git remote --mirror bug?

git remote --mirror bug?

[Joakim Tjernlund]

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

Re: git remote --mirror bug?

[Joakim Tjernlund]

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

Re* git remote --mirror bug?

[Junio C Hamano]

Joakim Tjernlund <joakim.tjernlund@transmode.se> writes:

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

This is very unfortunate.  What's more unfortunate is that, adding insult
to injury, "git remote" reimplemented in C gives even worse nonsense:

    $ git remote show git.git
    fatal: * refusing to create funny ref 'refs/stash' locally

This is with my test repository sitting next to my primary git.git
repository with config entries like this:

    [remote "git.git"]
        url = ../git.git/
        fetch = refs/*:refs/*

The thing is that we obviously are _not_ attempting to create anything
when we say "git remote show".

Now, it happens that "stash" is purely a local matter, and propagating it
with fetch (even with mirroring) may not make much sense.  Even if the
mirroring is for backup purposes, its value is dubious (if you are backing
up because you are afraid of disk failure, you should back up properly
with disk back-up tools).

So in that sense, it could be argued that it is Ok not to propagate
refs/stash and that was partly the reason why we create the "stash" ref
directly underneath refs/ namespace.  Everything else that is propagatable
has at least two level refnames, e.g. heads/master, tags/v1.5.0, and the
plumbing layer has a way to enforce this, and "git remote" uses it.

But that is not an excuse to fail "git fetch" (which is the underlying
mechanism "git remote" uses).

I think this patch may work the issue around for recent enough git, but
I have to say that this is an interim fix.  With this, now you get a less
nonsense output, but it still is nonsense nevertheless:

    $ git remote show git.git
    * remote git.git
      URL: ../git.git/
      New remote branch (next fetch will store in remotes/git.git)
        refs/stash
      Tracked remote branches
        ar/sgid-bsd cb/mergetool cc/help cr/reset-parseopt db/diff...

Notice that it talks about storing refs/stash in remotes/git.git?

It won't, and it shouldn't, as we told it to mirror refs/stash to
refs/stash.

As a side note, I think remote, when doing "show" that does _not_ update
the local side of tracking refs, should tweak the error behaviour that it
can trigger when it calls get_fetch_map().  The callee may have originally
been written for "fetch" and had a hardcoded "we are fetching and storing,
so our behaviour when seeing a funny ref is to error out with "refusing to
create" message and that is fine" mentality.  Calling such a function when
it is not actually fetching, without modifying the callee's assumption to
suit the error behaviour for its needs, is sloppy and needs to be fixed.

The scripted version if git-parse-remote, which is being phased out (it
still is used somewhat by git-pull and git-clone) has similar logic to
parse fetch refspec mapping, but it does not understand the mirroring
layout (refs/*:refs/*) as you saw, so it fails way before it triggers
these issues.  What the C reimplementation does (or at least tries to do)
is an improvement in that sense, but because it is relatively a new
feature, bugs in there are to be expected.

We are going into 1.5.5-rc feature freeze tonight, and squashing these
bugs now are of the highest priority.  Please keep the bug reports and
fixes flowing.

Thanks.

---

 builtin-check-ref-format.c |    2 +-
 git-parse-remote.sh        |    9 +++++++--
 remote.c                   |   16 +++++++++++++---
 3 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/builtin-check-ref-format.c b/builtin-check-ref-format.c
index fe04be7..e2b4eef 100644
--- a/builtin-check-ref-format.c
+++ b/builtin-check-ref-format.c
@@ -10,5 +10,5 @@ int cmd_check_ref_format(int argc, const char **argv, const char *prefix)
 {
 	if (argc != 2)
 		usage("git-check-ref-format refname");
-	return !!check_ref_format(argv[1]);
+	return (-check_ref_format(argv[1]));
 }
diff --git a/git-parse-remote.sh b/git-parse-remote.sh
index 695a409..19d00e7 100755
--- a/git-parse-remote.sh
+++ b/git-parse-remote.sh
@@ -156,8 +156,13 @@ canon_refs_list_for_fetch () {
 
 		if local_ref_name=$(expr "z$local" : 'zrefs/\(.*\)')
 		then
-		   git check-ref-format "$local_ref_name" ||
-		   die "* refusing to create funny ref '$local_ref_name' locally"
+		   git check-ref-format "$local_ref_name"
+		   case "$?" in
+		   0 | 2) ;;
+		   *)
+			die "* refusing to create funny ref '$local_ref_name' locally"
+			;;
+		   esac
 		fi
 		echo "${dot_prefix}${force}${remote}:${local}"
 	done
diff --git a/remote.c b/remote.c
index f3f7375..6a95b1e 100644
--- a/remote.c
+++ b/remote.c
@@ -1007,9 +1007,19 @@ int get_fetch_map(const struct ref *remote_refs,
 	}
 
 	for (rm = ref_map; rm; rm = rm->next) {
-		if (rm->peer_ref && check_ref_format(rm->peer_ref->name + 5))
-			die("* refusing to create funny ref '%s' locally",
-			    rm->peer_ref->name);
+		if (rm->peer_ref) {
+			switch (check_ref_format(rm->peer_ref->name + 5)) {
+			default:
+				die("* refusing to create funny ref '%s' locally",
+				    rm->peer_ref->name);
+				break;
+			case CHECK_REF_FORMAT_ONELEVEL:
+				/* allow "refs/stash" to be propagated */
+				break;
+			case CHECK_REF_FORMAT_OK:
+				break;
+			}
+		}
 	}
 
 	if (ref_map)

remote/clone bug: Stale tracking branch HEAD

[Teemu Likonen]

Junio C Hamano kirjoitti:

> We are going into 1.5.5-rc feature freeze tonight, and squashing
> these bugs now are of the highest priority.  Please keep the bug
> reports and fixes flowing.

I hope this bug won't find it's way to the release:

http://article.gmane.org/gmane.comp.version-control.git/77188


In short: After 'git clone' the command 'git remote show origin' shows a 
stale tracking branch HEAD:

  Stale tracking branch (use 'git remote prune')
      HEAD

'git remote prune origin' removes branch 'master'; you can see this 
with "git remote show origin":

  New remote branch (next fetch will store in remotes/origin)
      master

'git fetch' fetches 'master' again but the stale tracking branch HEAD 
appears again.

On fetch refspecs and wildcards

[Junio C Hamano]

I was looking at t5505 tests and noticed something funny.

This is a design level question, so I am cc'ing Daniel whose remote.c is
heavily involved in the new implementation.

What should this config do:

    [remote "origin"]
        url = ../one/.git
        fetch = +refs/heads/*:refs/remotes/origin/*
        fetch = refs/heads/master:refs/heads/upstream

when the other repository (../one/.git) has branches "master" and "side2"?

I am not sure if the original implementation used to copy master to both
refs/remotes/origin/master and refs/heads/upstream, but I think that is
what the users would expect.  

I think the current one excludes any source that has an explicit
destination from the wildcard matches.  It is probably Ok as long as we
reject if the same source has more than one destinations (or matches more
than one wildcards, for that matter) like this as a configuration error:

    [remote "origin"]
        url = ../one/.git
        fetch = refs/heads/master:refs/heads/upstream
        fetch = refs/heads/master:refs/heads/another

If it doesn't, it does feel somewhat inconsistent.

Fortunately or unfortunately, Documentation/pull-fetch-param.txt does not
talk about wildcard refspecs (not even the syntax, let alone the
semantics), so we can define whatever we want right now, and I think both

    (1) allow duplicated destinations, including wildcard matches; and

    (2) refuse duplicated destinations for explicit ones, and more than
        one wildcard patterns that match the same ref, but omit explicitly
        specified ones from wildcard matches;

are viable options.  I suspect the current code does not do either.  We
should pick one semantics, make sure the implementation matches that, and
document it.

Another topic is what the semantics should be for mirroring configuration,
like this:

    [remote "origin"]
        url = ../one/.git
        fetch = refs/*:refs/*

or

    [remote "origin"]
        url = ../one/.git
        fetch = refs/*:refs/remotes/one/*

The issues are:

 (1) get_fetch_map() currently insists on refname to be check_ref_format()
     clean; it even rejects CHECK_REF_FORMAT_ONELEVEL, which means that
     refs/stash would not be considered Ok and the code will die().

 (2) "git remote prune" seems to cull refs/remotes/one/HEAD if exists.

Currently we do not have a way to determine where HEAD at the remote
points at at the protocol level (I've sent a patch to the list earlier for
the necessary protocol extension on the upload-pack side, but receiver
side never got implemented in remotes.c).  So we cannot propagate
refs/HEAD information correctly right now, but when we accept the protocol
extension to do so, issue (1) will matter also for HEAD.

I think there is no design level question on this one.  I think both are
bugs we currently have (and they may probably not be regressions).

Re: On fetch refspecs and wildcards

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> ...
> Fortunately or unfortunately, Documentation/pull-fetch-param.txt does not
> talk about wildcard refspecs (not even the syntax, let alone the
> semantics), so we can define whatever we want right now, and I think both
>
>     (1) allow duplicated destinations, including wildcard matches; and

Clarification.

"s/;/, but do honor requests to copy one thing to multiple destinations;/"
is what I meant here.

>     (2) refuse duplicated destinations for explicit ones, and more than
>         one wildcard patterns that match the same ref, but omit explicitly
>         specified ones from wildcard matches;
>
> are viable options.  I suspect the current code does not do either.  We
> should pick one semantics, make sure the implementation matches that, and
> document it.

Re: On fetch refspecs and wildcards

[Daniel Barkalow]

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

> I was looking at t5505 tests and noticed something funny.
> 
> This is a design level question, so I am cc'ing Daniel whose remote.c is
> heavily involved in the new implementation.
> 
> What should this config do:
> 
>     [remote "origin"]
>         url = ../one/.git
>         fetch = +refs/heads/*:refs/remotes/origin/*
>         fetch = refs/heads/master:refs/heads/upstream
> 
> when the other repository (../one/.git) has branches "master" and "side2"?
> 
> I am not sure if the original implementation used to copy master to both
> refs/remotes/origin/master and refs/heads/upstream, but I think that is
> what the users would expect.  
> 
> I think the current one excludes any source that has an explicit
> destination from the wildcard matches.  It is probably Ok as long as we
> reject if the same source has more than one destinations (or matches more
> than one wildcards, for that matter) like this as a configuration error:
> 
>     [remote "origin"]
>         url = ../one/.git
>         fetch = refs/heads/master:refs/heads/upstream
>         fetch = refs/heads/master:refs/heads/another
> 
> If it doesn't, it does feel somewhat inconsistent.
> 
> Fortunately or unfortunately, Documentation/pull-fetch-param.txt does not
> talk about wildcard refspecs (not even the syntax, let alone the
> semantics), so we can define whatever we want right now, and I think both
> 
>     (1) allow duplicated destinations, including wildcard matches; and
> 
>     (2) refuse duplicated destinations for explicit ones, and more than
>         one wildcard patterns that match the same ref, but omit explicitly
>         specified ones from wildcard matches;
> 
> are viable options.  I suspect the current code does not do either.  We
> should pick one semantics, make sure the implementation matches that, and
> document it.

Actually, I think the current code is close to (2). get_fetch_map() 
returns everything, ref_remove_duplicates() removes any exact matches and 
gives errors if there's the same destination for two different sources.

(Upon further consideration, there's one slight issue:

[remote "origin"]
	fetch = refs/heads/*:refs/remotes/origin/*
	fetch = +refs/heads/pu:refs/remotes/origin/pu

is not quite the same as:

[remote "origin"]
	fetch = +refs/heads/pu:refs/remotes/origin/pu
	fetch = refs/heads/*:refs/remotes/origin/*

in whether pu will be forced; the forcing flag on the first matching 
refspec is what matters.)

In any case, the implementation should be easy enough with any of these 
combinations.

> Another topic is what the semantics should be for mirroring configuration,
> like this:
> 
>     [remote "origin"]
>         url = ../one/.git
>         fetch = refs/*:refs/*
> 
> or
> 
>     [remote "origin"]
>         url = ../one/.git
>         fetch = refs/*:refs/remotes/one/*
> 
> The issues are:
> 
>  (1) get_fetch_map() currently insists on refname to be check_ref_format()
>      clean; it even rejects CHECK_REF_FORMAT_ONELEVEL, which means that
>      refs/stash would not be considered Ok and the code will die().

Yes, that's probably wrong. We probably do want to reject people whose 
servers send us "refs/heads/../../heads/master", but not "refs/stash".

>  (2) "git remote prune" seems to cull refs/remotes/one/HEAD if exists.
> 
> Currently we do not have a way to determine where HEAD at the remote
> points at at the protocol level (I've sent a patch to the list earlier for
> the necessary protocol extension on the upload-pack side, but receiver
> side never got implemented in remotes.c).  So we cannot propagate
> refs/HEAD information correctly right now, but when we accept the protocol
> extension to do so, issue (1) will matter also for HEAD.

There's the issue that "HEAD" isn't "refs/HEAD". I'm not at all sure how 
the user should communicate the desire to update things to match the 
remote HEAD. FWIW, I was considering moving the code to guess where the 
remote HEAD points from builtin-clone to remotes.c, until I realized that 
it's not clear what configuration should control this. I think it'd be 
necessary to have a special option to say "write HEAD here", but I may be 
wrong.

The implementation of whatever handles it is slightly non-trivial, since 
struct ref can't currently represent a symref, but that shouldn't be a 
major issue.

	-Daniel
*This .sig left intentionally blank*

Re: On fetch refspecs and wildcards

[Junio C Hamano]

Daniel Barkalow <barkalow@iabervon.org> writes:

> On Sun, 16 Mar 2008, Junio C Hamano wrote:
> ...
>> Fortunately or unfortunately, Documentation/pull-fetch-param.txt does not
>> talk about wildcard refspecs (not even the syntax, let alone the
>> semantics), so we can define whatever we want right now, and I think both
>> 
>>     (1) allow duplicated destinations, including wildcard matches; and
>> 
>>     (2) refuse duplicated destinations for explicit ones, and more than
>>         one wildcard patterns that match the same ref, but omit explicitly
>>         specified ones from wildcard matches;
>> 
>> are viable options.  I suspect the current code does not do either.  We
>> should pick one semantics, make sure the implementation matches that, and
>> document it.
>
> Actually, I think the current code is close to (2). get_fetch_map() 
> returns everything, ref_remove_duplicates() removes any exact matches and 
> gives errors if there's the same destination for two different sources.
>
> (Upon further consideration, there's one slight issue:
>
> [remote "origin"]
> 	fetch = refs/heads/*:refs/remotes/origin/*
> 	fetch = +refs/heads/pu:refs/remotes/origin/pu
>
> is not quite the same as:
>
> [remote "origin"]
> 	fetch = +refs/heads/pu:refs/remotes/origin/pu
> 	fetch = refs/heads/*:refs/remotes/origin/*
>
> in whether pu will be forced; the forcing flag on the first matching 
> refspec is what matters.)

Ok.

As I said, I think either one is valid, and I only mentioned (1) because I
thought refusing duplicates might be more work to get it right.  So if
your code does _most of_ (2), that is good.  Please document what it is
meant to do in Documentation/pull-fetch-param.txt, so that others can
report deviation from the defined semantics, if any, in the implementation
for us to fix.

>> The issues are:
>> 
>>  (1) get_fetch_map() currently insists on refname to be check_ref_format()
>>      clean; it even rejects CHECK_REF_FORMAT_ONELEVEL, which means that
>>      refs/stash would not be considered Ok and the code will die().
>
> Yes, that's probably wrong. We probably do want to reject people whose 
> servers send us "refs/heads/../../heads/master", but not "refs/stash".

The feeler patch I sent out would be Ok, then.  Can you test it, after
updating it with the die() -> error() and message rewording we discussed
in the other message, and send the result in?

>>  (2) "git remote prune" seems to cull refs/remotes/one/HEAD if exists.
>> 
>> Currently we do not have a way to determine where HEAD at the remote
>> points at at the protocol level (I've sent a patch to the list earlier for
>> the necessary protocol extension on the upload-pack side, but receiver
>> side never got implemented in remotes.c).  So we cannot propagate
>> refs/HEAD information correctly right now, but when we accept the protocol
>> extension to do so, issue (1) will matter also for HEAD.
>
> There's the issue that "HEAD" isn't "refs/HEAD". I'm not at all sure how 
> the user should communicate the desire to update things to match the 
> remote HEAD. FWIW, I was considering moving the code to guess where the 
> remote HEAD points from builtin-clone to remotes.c, until I realized that 
> it's not clear what configuration should control this.. I think it'd be 
> necessary to have a special option to say "write HEAD here", but I may be 
> wrong.

I tend to agree.  I'd propose the semantics for refs/remotes/<name>/HEAD
symref to be like this:

 * It is under _local_ control.  That means fetch should not update it, and
   "remote prune" should not prune it, nor even mention it is prunable.

 * It is the means for the user (i.e. the owner of the local repository)
   to express which branch from the remote he is most interested in.
   I.e., it exists solely to make "<name>" => "refs/remotes/<name>/HEAD"
   ref dwimming work as expected.

 * It is set up by "git clone" to point at the branch the remote had its
   HEAD pointing at when clone happened but that is merely a convenience
   feature.

 * We would probably want an explicit convenience subcommand "git remote
   something <name> <branch>" that switches refs/remotes/<name>/HEAD to
   point at a specific remote tracking branch, although you can do that
   yourself with symbolic-ref.

 * We may want to teach "git remote add <name>" to do the same HEAD
   discovery as done by "git clone" (earlier JBF had a patch for it to the
   scripted version), to have the same convenience feature as "git clone"
   has.

 * If we teach "git remote add" to set refs/remotes/<name>/HEAD, we may
   also want to teach it an explicit way to let the user say "I want
   <name> to mean refs/remotes/<name>/this", not whatever the remote side
   currently points at with its HEAD.

 * If we teach "git remote add" to do the HEAD discovery, we may also want
   to teach "git remote update" a way to let the user request "my
   refs/remotes/<name>/HEAD may not be pointing at the branch the remote
   currently points at with its HEAD.  Please update mine to match
   theirs".

When true mirroring configuration "refs/*:refs/*" is employed, neither
"refs/HEAD" nor "refs/heads/HEAD" is needed nor desired on the local side.

Re: On fetch refspecs and wildcards

[Daniel Barkalow]

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

> Daniel Barkalow <barkalow@iabervon.org> writes:
> 
> > On Sun, 16 Mar 2008, Junio C Hamano wrote:
> > ...
> >> Fortunately or unfortunately, Documentation/pull-fetch-param.txt does not
> >> talk about wildcard refspecs (not even the syntax, let alone the
> >> semantics), so we can define whatever we want right now, and I think both
> >> 
> >>     (1) allow duplicated destinations, including wildcard matches; and
> >> 
> >>     (2) refuse duplicated destinations for explicit ones, and more than
> >>         one wildcard patterns that match the same ref, but omit explicitly
> >>         specified ones from wildcard matches;
> >> 
> >> are viable options.  I suspect the current code does not do either.  We
> >> should pick one semantics, make sure the implementation matches that, and
> >> document it.
> >
> > Actually, I think the current code is close to (2). get_fetch_map() 
> > returns everything, ref_remove_duplicates() removes any exact matches and 
> > gives errors if there's the same destination for two different sources.
> >
> > (Upon further consideration, there's one slight issue:
> >
> > [remote "origin"]
> > 	fetch = refs/heads/*:refs/remotes/origin/*
> > 	fetch = +refs/heads/pu:refs/remotes/origin/pu
> >
> > is not quite the same as:
> >
> > [remote "origin"]
> > 	fetch = +refs/heads/pu:refs/remotes/origin/pu
> > 	fetch = refs/heads/*:refs/remotes/origin/*
> >
> > in whether pu will be forced; the forcing flag on the first matching 
> > refspec is what matters.)
> 
> Ok.
> 
> As I said, I think either one is valid, and I only mentioned (1) because I
> thought refusing duplicates might be more work to get it right.  So if
> your code does _most of_ (2), that is good.  Please document what it is
> meant to do in Documentation/pull-fetch-param.txt, so that others can
> report deviation from the defined semantics, if any, in the implementation
> for us to fix.

Oh, sorry, I got the cases backwards. The current code does (1): for each 
refspec, it collects all of the matches for that refspec into one big 
list, and then it (a) removes items where src and dst match and (b) gives 
errors if dst matches but src doesn't (which is obviously a problem: two 
refspecs will try to write different things to the same place).

> >> The issues are:
> >> 
> >>  (1) get_fetch_map() currently insists on refname to be check_ref_format()
> >>      clean; it even rejects CHECK_REF_FORMAT_ONELEVEL, which means that
> >>      refs/stash would not be considered Ok and the code will die().
> >
> > Yes, that's probably wrong. We probably do want to reject people whose 
> > servers send us "refs/heads/../../heads/master", but not "refs/stash".
> 
> The feeler patch I sent out would be Ok, then.  Can you test it, after
> updating it with the die() -> error() and message rewording we discussed
> in the other message, and send the result in?

Sure.

> >>  (2) "git remote prune" seems to cull refs/remotes/one/HEAD if exists.
> >> 
> >> Currently we do not have a way to determine where HEAD at the remote
> >> points at at the protocol level (I've sent a patch to the list earlier for
> >> the necessary protocol extension on the upload-pack side, but receiver
> >> side never got implemented in remotes.c).  So we cannot propagate
> >> refs/HEAD information correctly right now, but when we accept the protocol
> >> extension to do so, issue (1) will matter also for HEAD.
> >
> > There's the issue that "HEAD" isn't "refs/HEAD". I'm not at all sure how 
> > the user should communicate the desire to update things to match the 
> > remote HEAD. FWIW, I was considering moving the code to guess where the 
> > remote HEAD points from builtin-clone to remotes.c, until I realized that 
> > it's not clear what configuration should control this.. I think it'd be 
> > necessary to have a special option to say "write HEAD here", but I may be 
> > wrong.
> 
> I tend to agree.  I'd propose the semantics for refs/remotes/<name>/HEAD
> symref to be like this:
> 
>  * It is under _local_ control.  That means fetch should not update it, and
>    "remote prune" should not prune it, nor even mention it is prunable.

Ah, interesting. Not at all what I'd expected, but much more useful, I 
think, than having it controlled by the remote.

>  * It is the means for the user (i.e. the owner of the local repository)
>    to express which branch from the remote he is most interested in.
>    I.e., it exists solely to make "<name>" => "refs/remotes/<name>/HEAD"
>    ref dwimming work as expected.
> 
>  * It is set up by "git clone" to point at the branch the remote had its
>    HEAD pointing at when clone happened but that is merely a convenience
>    feature.

In other words, a reasonable default since you haven't yet configured 
anything.

>  * We would probably want an explicit convenience subcommand "git remote
>    something <name> <branch>" that switches refs/remotes/<name>/HEAD to
>    point at a specific remote tracking branch, although you can do that
>    yourself with symbolic-ref.

The command would mainly be useful to users as a way of making it clear 
that it's theirs to control, rather than as a method for controlling it.

>  * We may want to teach "git remote add <name>" to do the same HEAD
>    discovery as done by "git clone" (earlier JBF had a patch for it to the
>    scripted version), to have the same convenience feature as "git clone"
>    has.

Right.

>  * If we teach "git remote add" to set refs/remotes/<name>/HEAD, we may
>    also want to teach it an explicit way to let the user say "I want
>    <name> to mean refs/remotes/<name>/this", not whatever the remote side
>    currently points at with its HEAD.

And likewise clone, which is probably more relevant, actually, because 
clone will often also create a local branch based on it and check that 
out, which can waste some time if you actually want to use some other 
branch.

>  * If we teach "git remote add" to do the HEAD discovery, we may also want
>    to teach "git remote update" a way to let the user request "my
>    refs/remotes/<name>/HEAD may not be pointing at the branch the remote
>    currently points at with its HEAD.  Please update mine to match
>    theirs".

Right.

> When true mirroring configuration "refs/*:refs/*" is employed, neither
> "refs/HEAD" nor "refs/heads/HEAD" is needed nor desired on the local side.

Sure.

	-Daniel
*This .sig left intentionally blank*

Re: Re* git remote --mirror bug?

[Johannes Schindelin]

Hi,

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

> Joakim Tjernlund <joakim.tjernlund@transmode.se> writes:
> 
> >> git remote show os2kernel 
> >> * remote os2kernel
> >>   URL: /usr/local/src/os2kernel
> >> Warning: unrecognized mapping in remotes.os2kernel.fetch: +refs/*:refs/*
> 
> This is very unfortunate.
>
> [...]
>
>  builtin-check-ref-format.c |    2 +-
>  git-parse-remote.sh        |    9 +++++++--
>  remote.c                   |   16 +++++++++++++---
>  3 files changed, 21 insertions(+), 6 deletions(-)

Thanks for the fix, and sorry for not being available to fix it myself.

Ciao,
Dscho

Re: Re* git remote --mirror bug?

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Sun, 16 Mar 2008, Junio C Hamano wrote:
>
>> Joakim Tjernlund <joakim.tjernlund@transmode.se> writes:
>> 
>> >> git remote show os2kernel 
>> >> * remote os2kernel
>> >>   URL: /usr/local/src/os2kernel
>> >> Warning: unrecognized mapping in remotes.os2kernel.fetch: +refs/*:refs/*
>> 
>> This is very unfortunate.
>>
>> [...]
>>
>>  builtin-check-ref-format.c |    2 +-
>>  git-parse-remote.sh        |    9 +++++++--
>>  remote.c                   |   16 +++++++++++++---
>>  3 files changed, 21 insertions(+), 6 deletions(-)
>
> Thanks for the fix,...

As I alluded to in the message, I do not think this was a fix.

Re: Re* git remote --mirror bug?

[Johannes Schindelin]

Hi,

On Tue, 18 Mar 2008, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > On Sun, 16 Mar 2008, Junio C Hamano wrote:
> >
> >> Joakim Tjernlund <joakim.tjernlund@transmode.se> writes:
> >> 
> >> >> git remote show os2kernel 
> >> >> * remote os2kernel
> >> >>   URL: /usr/local/src/os2kernel
> >> >> Warning: unrecognized mapping in remotes.os2kernel.fetch: +refs/*:refs/*
> >> 
> >> This is very unfortunate.
> >>
> >> [...]
> >>
> >>  builtin-check-ref-format.c |    2 +-
> >>  git-parse-remote.sh        |    9 +++++++--
> >>  remote.c                   |   16 +++++++++++++---
> >>  3 files changed, 21 insertions(+), 6 deletions(-)
> >
> > Thanks for the fix,...
> 
> As I alluded to in the message, I do not think this was a fix.

I am very sorry, as I read the mail under extreme time pressure, this must 
have slipped by.  I hope that my time management reverts to normal 
beginning tomorrow.

I looked into this issue, and I seem not to be able to reproduce with my 
current git (which is based on next).

Ciao,
Dscho

[PATCH/RFC] Allow "git remote --mirror" to mirror stashes

[Junio C Hamano]

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 <gitster@pobox.com>
---

 * 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;

Re: [PATCH/RFC] Allow "git remote --mirror" to mirror stashes

[Daniel Barkalow]

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*

Re: [PATCH/RFC] Allow "git remote --mirror" to mirror stashes

[Junio C Hamano]

Daniel Barkalow <barkalow@iabervon.org> 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.

Re: [PATCH/RFC] Allow "git remote --mirror" to mirror stashes

[Daniel Barkalow]

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

> Daniel Barkalow <barkalow@iabervon.org> 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?

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.

> > 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*