git remote --mirror bug?

Created a mirror like so:
 git --bare init 
 git remote add --mirror os2kernel /usr/local/src/os2kernel

Git fetch errors out
 git fetch os2kernel 
fatal: * refusing to create funny ref 'refs/stash' locally

Also 
git remote show os2kernel 
* remote os2kernel
  URL: /usr/local/src/os2kernel
Warning: unrecognized mapping in remotes.os2kernel.fetch: +refs/*:refs/*

git --version
git version 1.5.4.3

 Jocke

On Fri, 2008-03-14 at 14:05 +0100, Joakim Tjernlund wrote:

Created a mirror like so:
 git --bare init 
 git remote add --mirror os2kernel /usr/local/src/os2kernel

Git fetch errors out
 git fetch os2kernel 
fatal: * refusing to create funny ref 'refs/stash' locally

Also 
git remote show os2kernel 
* remote os2kernel
  URL: /usr/local/src/os2kernel
Warning: unrecognized mapping in remotes.os2kernel.fetch: +refs/*:refs/*

git --version
git version 1.5.4.3

 Jocke

Forgot to mention that clearing the stash with "git stash clear"
deletes the refs/stash file and then above commands succeeds.

This is a rather harmless bug, but if you are running the fetch command
in a cron job to backup your repo, it becomes more serious as one
will not see the failure.

 Jocke

When you have "remote.$there.fetch = refs/*:refs/*" and the remote has a
ref directly under refs/ (e.g. "stash"), "git fetch" still errored out
even with fixes in -rc1.

This should hopefully fix it.

Signed-off-by: Junio C Hamano <redacted>
---

 * Rather than failing, it would be better to allow "git fetch" to succeed
   by doing this, but on the other hand, stash is purely a local matter,
   so it might make more sense to avoid exposing it from the uploader.

 builtin-fetch-pack.c |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/builtin-fetch-pack.c b/builtin-fetch-pack.c
index 65350ca..472bad5 100644
--- a/builtin-fetch-pack.c
+++ b/builtin-fetch-pack.c

@@ -363,10 +363,17 @@ static void filter_refs(struct ref **refs, int nr_match, char **match)
 		return_refs = NULL;
 
 	for (ref = *refs; ref; ref = next) {
+		int trash = 0;
+
 		next = ref->next;
-		if (!memcmp(ref->name, "refs/", 5) &&
-		    check_ref_format(ref->name + 5))
-			; /* trash */
+		if (!memcmp(ref->name, "refs/", 5)) {
+			trash = check_ref_format(ref->name + 5);
+			if (trash == CHECK_REF_FORMAT_ONELEVEL)
+				trash = 0;
+		}
+
+		if (trash)
+			; /* this is trash */
 		else if (args.fetch_all &&
 			 (!args.depth || prefixcmp(ref->name, "refs/tags/") )) {
 			*newtail = ref;

On Thu, 27 Mar 2008, Junio C Hamano wrote:

When you have "remote.$there.fetch = refs/*:refs/*" and the remote has a
ref directly under refs/ (e.g. "stash"), "git fetch" still errored out
even with fixes in -rc1.

In particular, it would fail to request "refs/stash", and then be 
surprised that it didn't get the object that points to. (This would be a 
helpful thing to mention in the commit message)

This should hopefully fix it.

Maybe it shouldn't do any filtering here, and instead do it in 
cmd_fetch_pack? If the transport code gets to this point and anything gets 
filtered out by this function, the transport code or builtin-fetch will 
have to be terribly confused and fail with a mysterious error message, 
AFAICT.

 * Rather than failing, it would be better to allow "git fetch" to succeed
   by doing this, but on the other hand, stash is purely a local matter,
   so it might make more sense to avoid exposing it from the uploader.

This is also true, although I'm not too sure that we won't want to do 
things like having "refs/default" in a public repository be the 
repository's suggestion for the default branch (to replace "HEAD", 
because, in a world where people use lots of branches, the "current 
branch" idea and the "default branch" idea aren't really the same idea, 
although there's no technical conflict since only one of these ideas is 
really important in any given repository). So we probably want a whitelist 
or blacklist for refs to serve when we avoid exposing things in the 
uploader, rather than using the level, in which case it's definitely 
important to have fetch-pack just ignore stuff.

	-Daniel
*This .sig left intentionally blank*

Daniel Barkalow [off-list ref] writes:

On Thu, 27 Mar 2008, Junio C Hamano wrote:

Maybe it shouldn't do any filtering here, and instead do it in 
cmd_fetch_pack?

I dunno.  How would the code look like?

This is also true, although I'm not too sure that we won't want to do 
things like having "refs/default" in a public repository be the 
repository's suggestion for the default branch (to replace "HEAD", 
because, in a world where people use lots of branches, the "current 
branch" idea and the "default branch" idea aren't really the same idea, 

In a public repository with many branches to serve people with different
interests, I do not think a single refs/default in addition to HEAD would
help that much.  We would _not_ want to have more magic refs like HEAD.

Quite the opposite.  In such a repository, HEAD means even less, and
instead of giving an extra layer of indirection, you tell people which
branches are what in your repository.  "If you are interested in only the
bugfixes without any new features since the last feature lease no matter
how solid and tested they are, use 'maint' branch.  If you want solid and
tested features, and do not mind new features, use 'master'.  Etc.".

And just like a good API names its functions sensibly, you give meaningful
names to your branches, so that you do not _need_ that extra layer of
indirection refs/default would incur.

On Sun, 30 Mar 2008, Junio C Hamano wrote:

Daniel Barkalow [off-list ref] writes:

quoted

On Thu, 27 Mar 2008, Junio C Hamano wrote:

Maybe it shouldn't do any filtering here, and instead do it in 
cmd_fetch_pack?

I dunno.  How would the code look like?

Actually, I don't see any reason to call check_ref_format. The point of 
filter_refs is to make sure that we don't fetch anything we didn't ask 
for. We shouldn't care at all about the name of the refs we're considering 
except whether there's in the list to fetch, and if the user requests the 
objects for a ref named 'refs/*^&+' and the server offers such a ref, 
there's no reason for us not to get the objects. (Sure, we shouldn't 
create the ref with that name, but this code path doesn't go on the create 
refs based on these names, except when it's already checking their format 
for that purpose anyway.)

So I'd say just drop the first "if" in that sequence entirely. The only 
thing that could be a problem we'd want to stop here is something that 
would break the packet protocol, and we've already gotten these values 
over the packet protocol anyway.

quoted

This is also true, although I'm not too sure that we won't want to do 
things like having "refs/default" in a public repository be the 
repository's suggestion for the default branch (to replace "HEAD", 
because, in a world where people use lots of branches, the "current 
branch" idea and the "default branch" idea aren't really the same idea, 

In a public repository with many branches to serve people with different
interests, I do not think a single refs/default in addition to HEAD would
help that much.  We would _not_ want to have more magic refs like HEAD.

Quite the opposite.  In such a repository, HEAD means even less, and
instead of giving an extra layer of indirection, you tell people which
branches are what in your repository.  "If you are interested in only the
bugfixes without any new features since the last feature lease no matter
how solid and tested they are, use 'maint' branch.  If you want solid and
tested features, and do not mind new features, use 'master'.  Etc.".

It's not a particularly *useful* default, but "git-clone" presumably 
should initially check out *something* given a repository with multiple 
branches and no local user guidance. And, if this is the git.git 
repository, and you briefly check out each branch in turn to build it when 
you push new changes, then HEAD is usually master but briefly, rarely, and 
irrelevantly other things.

I'm not convinced that it's something worth actually implementing. But I 
think it's a plausible enough idea that we shouldn't exclude the 
possibility of one-level public refs. There are various usues people find 
for this sort of low-semantics pointer on FTP sites, so it could be useful 
in git as well.

	-Daniel
*This .sig left intentionally blank*