[PATCH 0/3] Unconfuse git clone when two branches at are HEAD.

[PATCH 0/3] Unconfuse git clone when two branches at are HEAD.

[Andreas Krey]

Ok, here are some patches that make git actually
check out the current remote branch when cloning.

Up to now this failed when there were two branches that pointed to 
the HEAD commit of the remote repo, and git clone would sometimes
choose the wrong one as the HEAD ref isn't transmitted in all
transport.

Stuff the HEAD ref into the capability list (assuming refs are clean 
enough to do that w/o escaping), and read them out on the other
side. All other things were thankfully already in place.

Two of the patches have Junio in the From as they are essentially his.

Andreas

[PATCH 1/3] upload-pack: send the HEAD information

[Andreas Krey]

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

This implements the server side of protocol extension to show which branch
the HEAD points at.  The information is sent as a capability symref=<target>.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Andreas Krey <a.krey@gmx.de>
---
 upload-pack.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/upload-pack.c b/upload-pack.c
index 127e59a..390d1ec 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -745,13 +745,17 @@ static int send_ref(const char *refname, const unsigned char *sha1, int flag, vo
 	if (mark_our_ref(refname, sha1, flag, cb_data))
 		return 0;
 
-	if (capabilities)
-		packet_write(1, "%s %s%c%s%s%s agent=%s\n",
+	if (capabilities) {
+		unsigned char dummy[20];
+		const char *target = resolve_ref_unsafe("HEAD", dummy, 0, NULL);
+		packet_write(1, "%s %s%c%s%s%s%s%s agent=%s\n",
 			     sha1_to_hex(sha1), refname_nons,
 			     0, capabilities,
 			     allow_tip_sha1_in_want ? " allow-tip-sha1-in-want" : "",
 			     stateless_rpc ? " no-done" : "",
+			     target ? " symref=" : "", target ? target : 0,
 			     git_user_agent_sanitized());
+	}
 	else
 		packet_write(1, "%s %s\n", sha1_to_hex(sha1), refname_nons);
 	capabilities = NULL;
-- 
1.8.3.1.485.g9704416.dirty

[PATCH 2/3] connect.c: save symref info from server capabilities

[Andreas Krey]

Signed-off-by: Andreas Krey <a.krey@gmx.de>
---
 connect.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/connect.c b/connect.c
index a0783d4..98c4868 100644
--- a/connect.c
+++ b/connect.c
@@ -72,8 +72,8 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
 	for (;;) {
 		struct ref *ref;
 		unsigned char old_sha1[20];
-		char *name;
-		int len, name_len;
+		char *name, *symref;
+		int len, name_len, symref_len;
 		char *buffer = packet_buffer;
 
 		len = packet_read(in, &src_buf, &src_len,
@@ -94,9 +94,12 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
 		name = buffer + 41;
 
 		name_len = strlen(name);
+		symref = 0;
 		if (len != name_len + 41) {
 			free(server_capabilities);
 			server_capabilities = xstrdup(name + name_len + 1);
+			symref = parse_feature_value(server_capabilities,
+						     "symref", &symref_len);
 		}
 
 		if (extra_have &&
@@ -108,6 +111,10 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
 		if (!check_ref(name, name_len, flags))
 			continue;
 		ref = alloc_ref(buffer + 41);
+		if (symref) {
+			ref->symref = xcalloc(symref_len + 1, 1);
+			strncpy(ref->symref, symref, symref_len);
+		}
 		hashcpy(ref->old_sha1, old_sha1);
 		*list = ref;
 		list = &ref->next;
-- 
1.8.3.1.485.g9704416.dirty

[PATCH 3/3] clone: test the new HEAD detection logic

[Andreas Krey]

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

Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Andreas Krey <a.krey@gmx.de>
---
 t/t5601-clone.sh | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 0629149..ccda6df 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -285,4 +285,15 @@ test_expect_success NOT_MINGW,NOT_CYGWIN 'clone local path foo:bar' '
 	git clone "./foo:bar" foobar
 '
 
+test_expect_success 'clone from a repository with two identical branches' '
+
+	(
+		cd src &&
+		git checkout -b another master
+	) &&
+	git clone src target-11 &&
+	test "z$( cd target-11 && git symbolic-ref HEAD )" = zrefs/heads/another
+
+'
+
 test_done
-- 
1.8.3.1.485.g9704416.dirty

Re: [PATCH 0/3] Unconfuse git clone when two branches at are HEAD.

[Philip Oakley]

From: "Andreas Krey" <a.krey@gmx.de>
> Ok, here are some patches that make git actually
> check out the current remote branch when cloning.
>
> Up to now this failed when there were two branches that pointed to
> the HEAD commit of the remote repo, and git clone would sometimes
> choose the wrong one as the HEAD ref isn't transmitted in all
> transport.
>
> Stuff the HEAD ref into the capability list (assuming refs are clean
> enough to do that w/o escaping), and read them out on the other
> side. All other things were thankfully already in place.
>
> Two of the patches have Junio in the From as they are essentially his.
>
> Andreas
> --

Does this have any impact on the alleged bug in `git bundle --all` 
(which can then be cloned from) where the current HEAD ref wasn't 
included in the bundle? Or am I mis-remembering?

Philip 

Re: [PATCH 1/3] upload-pack: send the HEAD information

[Junio C Hamano]

Andreas Krey <a.krey@gmx.de> writes:

> From: Junio C Hamano <gitster@pobox.com>
>
> This implements the server side of protocol extension to show which branch
> the HEAD points at.  The information is sent as a capability symref=<target>.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Andreas Krey <a.krey@gmx.de>
> ---
>  upload-pack.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/upload-pack.c b/upload-pack.c
> index 127e59a..390d1ec 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -745,13 +745,17 @@ static int send_ref(const char *refname, const unsigned char *sha1, int flag, vo
>  	if (mark_our_ref(refname, sha1, flag, cb_data))
>  		return 0;
>  
> -	if (capabilities)
> -		packet_write(1, "%s %s%c%s%s%s agent=%s\n",
> +	if (capabilities) {
> +		unsigned char dummy[20];
> +		const char *target = resolve_ref_unsafe("HEAD", dummy, 0, NULL);
> +		packet_write(1, "%s %s%c%s%s%s%s%s agent=%s\n",
>  			     sha1_to_hex(sha1), refname_nons,
>  			     0, capabilities,
>  			     allow_tip_sha1_in_want ? " allow-tip-sha1-in-want" : "",
>  			     stateless_rpc ? " no-done" : "",
> +			     target ? " symref=" : "", target ? target : 0,
>  			     git_user_agent_sanitized());
> +	}
>  	else
>  		packet_write(1, "%s %s\n", sha1_to_hex(sha1), refname_nons);
>  	capabilities = NULL;

I think it is perfectly fine to expose _only_ HEAD now, and wait
until we find a good reason that we should send this information for
other symbolic refs in the repository.

However, because we already anticipate that we may find such a good
reason later, on-the-wire format should be prepared to support such
later enhancement.  I think sending

	symref=HEAD:refs/heads/master

is probably one good way to do so, as Peff suggested in that old
thread ($gmane/102070; note that back then this wasn't suggested as
a proper capability so the exact format he suggests in the message
is different).  Then we could later add advertisements for other
symbolic refs if we find it necessary to do so, e.g.

	symref=HEAD:refs/heads/master
        symref=refs/remotes/origin/HEAD:refs/remotes/origin/master

(all on one line together with other capabilities separated with a
SP in between).

Re: [PATCH 2/3] connect.c: save symref info from server capabilities

[Junio C Hamano]

Andreas Krey <a.krey@gmx.de> writes:

> Signed-off-by: Andreas Krey <a.krey@gmx.de>
> ---
>  connect.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/connect.c b/connect.c
> index a0783d4..98c4868 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -72,8 +72,8 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
>  	for (;;) {
>  		struct ref *ref;
>  		unsigned char old_sha1[20];
> -		char *name;
> -		int len, name_len;
> +		char *name, *symref;
> +		int len, name_len, symref_len;
>  		char *buffer = packet_buffer;
>  
>  		len = packet_read(in, &src_buf, &src_len,
> @@ -94,9 +94,12 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
>  		name = buffer + 41;
>  
>  		name_len = strlen(name);
> +		symref = 0;
>  		if (len != name_len + 41) {
>  			free(server_capabilities);
>  			server_capabilities = xstrdup(name + name_len + 1);
> +			symref = parse_feature_value(server_capabilities,
> +						     "symref", &symref_len);
>  		}
>  		if (extra_have &&
> @@ -108,6 +111,10 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
>  		if (!check_ref(name, name_len, flags))
>  			continue;
>  		ref = alloc_ref(buffer + 41);
> +		if (symref) {
> +			ref->symref = xcalloc(symref_len + 1, 1);
> +			strncpy(ref->symref, symref, symref_len);
> +		}
>  		hashcpy(ref->old_sha1, old_sha1);
>  		*list = ref;
>  		list = &ref->next;


This looks utterly wrong.  HEAD may happen to be the first ref that
is advertised and hence capability list typically comes on it, but
that does not necessarily have to be the case from the protocol's
correctness point of view.

I think this function should do this instead.

    - inside the loop, collect the "symref=..." capabilities;

    - after the loop, look at the "symref=..." capabilities, and
      among the refs the loop added to the *list, find the "HEAD"
      ref and set its ->symref to point at an appropirate ref.

Re: [PATCH 0/3] Unconfuse git clone when two branches at are HEAD.

[Junio C Hamano]

"Philip Oakley" <philipoakley@iee.org> writes:

> Does this have any impact on the alleged bug in `git bundle --all`
> (which can then be cloned from) where the current HEAD ref wasn't
> included in the bundle? Or am I mis-remembering?

Not "current HEAD ref", but "git clone" will fail to check out from
a bundle that does not include HEAD ref (it is easy to just say
"reset --hard master" or whatever after it, though).

I think I suggested to update "git bundle" to include HEAD when
there is no HEAD specified some time ago, but I do not think anybody
was interested, so this may be a non-issue.

Re: [PATCH 2/3] connect.c: save symref info from server capabilities

[Andreas Krey]

On Fri, 06 Sep 2013 10:56:51 +0000, Junio C Hamano wrote:
> Andreas Krey <a.krey@gmx.de> writes:
> 
...
> > +		if (symref) {
> > +			ref->symref = xcalloc(symref_len + 1, 1);
> > +			strncpy(ref->symref, symref, symref_len);
> > +		}
...
> 
> This looks utterly wrong.  HEAD may happen to be the first ref that
> is advertised and hence capability list typically comes on it, but
> that does not necessarily have to be the case from the protocol's
> correctness point of view.

Ok, then I misunderstood that part. I thought we'd be going to
put the symref (if any) into 'capabilities' on the respective ref,
but putting them all in one capability list sounds saner all in all.

> I think this function should do this instead.
> 
>     - inside the loop, collect the "symref=..." capabilities;
> 
>     - after the loop, look at the "symref=..." capabilities, and
>       among the refs the loop added to the *list, find the "HEAD"
>       ref and set its ->symref to point at an appropirate ref.

Fair game. There goes the parse_feature_value; will have to iterate
another way (or look them ("symref=#{name}:") up instead of collecting
them into a hash beforehand).

Can I assume that the only capability list is always on the
first ref sent (as it is now)?

(And besides, is there something more suitable for the 
 xcalloc/strncpy combination?)

Andreas

-- 
"Totally trivial. Famous last words."
From: Linus Torvalds <torvalds@*.org>
Date: Fri, 22 Jan 2010 07:29:21 -0800

Re: [PATCH 1/3] upload-pack: send the HEAD information

[Andreas Krey]

On Fri, 06 Sep 2013 10:46:24 +0000, Junio C Hamano wrote:
> Andreas Krey <a.krey@gmx.de> writes:
> 
...
> reason later, on-the-wire format should be prepared to support such
> later enhancement.  I think sending
> 
> 	symref=HEAD:refs/heads/master

Mirco-bikeshed: Should that possibly be

  symref:HEAD=refs/heads/master

as then 'symref:HEAD' is a regular capability key?

Andreas

-- 
"Totally trivial. Famous last words."
From: Linus Torvalds <torvalds@*.org>
Date: Fri, 22 Jan 2010 07:29:21 -0800

Re: [PATCH 2/3] connect.c: save symref info from server capabilities

[Junio C Hamano]

Andreas Krey <a.krey@gmx.de> writes:

> Can I assume that the only capability list is always on the
> first ref sent (as it is now)?

The capability list _could_ be sent more than once, and the
receiving end is prepared to accept such a stream.  Everything
learned from an older capability list needs to be forgot and only
the last one is honored, I think.

Re: [PATCH 1/3] upload-pack: send the HEAD information

[Junio C Hamano]

Andreas Krey <a.krey@gmx.de> writes:

> On Fri, 06 Sep 2013 10:46:24 +0000, Junio C Hamano wrote:
>> Andreas Krey <a.krey@gmx.de> writes:
>> 
> ...
>> reason later, on-the-wire format should be prepared to support such
>> later enhancement.  I think sending
>> 
>> 	symref=HEAD:refs/heads/master
>
> Mirco-bikeshed: Should that possibly be
>
>   symref:HEAD=refs/heads/master
>
> as then 'symref:HEAD' is a regular capability key?

I doubt that is a good change.

We haven't needed (and have refrained from adding) any capability
with an unknown name.  The variable part should go to the value
portion of the token.

Re: [PATCH 0/3] Unconfuse git clone when two branches at are HEAD.

[Philip Oakley]

From: "Junio C Hamano" <gitster@pobox.com>
> "Philip Oakley" <philipoakley@iee.org> writes:
>
>> Does this have any impact on the alleged bug in `git bundle --all`
>> (which can then be cloned from) where the current HEAD ref wasn't
>> included in the bundle? Or am I mis-remembering?
>
> Not "current HEAD ref", but "git clone" will fail to check out from
> a bundle that does not include HEAD ref (it is easy to just say
> "reset --hard master" or whatever after it, though).
>
> I think I suggested to update "git bundle" to include HEAD when
> there is no HEAD specified some time ago, but I do not think anybody
> was interested, so this may be a non-issue.
>
Just had a quick look at a very quick test repo (10 objects, 2 branches) 
and the bundle file does contain the HEAD ref, but again it has the two 
ref/heads/* are better than one problem, in that the clone from the 
bundle checks out master, whilst the source repo has feature checked 
out.

Philip 

Re: [PATCH 0/3] Unconfuse git clone when two branches at are HEAD.

[Junio C Hamano]

"Philip Oakley" <philipoakley@iee.org> writes:

> From: "Junio C Hamano" <gitster@pobox.com>
>> "Philip Oakley" <philipoakley@iee.org> writes:
>>
>>> Does this have any impact on the alleged bug in `git bundle --all`
>>> (which can then be cloned from) where the current HEAD ref wasn't
>>> included in the bundle? Or am I mis-remembering?
>>
>> Not "current HEAD ref", but "git clone" will fail to check out from
>> a bundle that does not include HEAD ref (it is easy to just say
>> "reset --hard master" or whatever after it, though).
>>
>> I think I suggested to update "git bundle" to include HEAD when
>> there is no HEAD specified some time ago, but I do not think anybody
>> was interested, so this may be a non-issue.
>>
> Just had a quick look at a very quick test repo (10 objects, 2
> branches) and the bundle file does contain the HEAD ref, but again it
> has the two ref/heads/* are better than one problem, in that the clone
> from the bundle checks out master, whilst the source repo has feature
> checked out.

I do not think the bundle header records symref any differently from
other refs, so a HEAD that points at a commit that is at the tip of
more than one ref needs to be guessed at the extraction end, just
like the network-transfer case discussed in this thread.

But this thread is not about updating the current bundle format to a
new one, so any of the updates proposed in these patches will not
affect it.

Re: [PATCH 0/3] Unconfuse git clone when two branches at are HEAD.

[Philip Oakley]

From: "Junio C Hamano" <gitster@pobox.com>
Sent: Saturday, September 07, 2013 4:50 PM
> "Philip Oakley" <philipoakley@iee.org> writes:
>> From: "Junio C Hamano" <gitster@pobox.com>
>>> "Philip Oakley" <philipoakley@iee.org> writes:
>>>
>>>> Does this have any impact on the alleged bug in `git bundle --all`
>>>> (which can then be cloned from) where the current HEAD ref wasn't
>>>> included in the bundle? Or am I mis-remembering?
>>>
>>> Not "current HEAD ref", but "git clone" will fail to check out from
>>> a bundle that does not include HEAD ref (it is easy to just say
>>> "reset --hard master" or whatever after it, though).
>>>
>>> I think I suggested to update "git bundle" to include HEAD when
>>> there is no HEAD specified some time ago, but I do not think anybody
>>> was interested, so this may be a non-issue.
>>>
>> Just had a quick look at a very quick test repo (10 objects, 2
>> branches) and the bundle file does contain the HEAD ref, but again it
>> has the two ref/heads/* are better than one problem, in that the 
>> clone
>> from the bundle checks out master, whilst the source repo has feature
>> checked out.
>
> I do not think the bundle header records symref any differently from
> other refs, so a HEAD that points at a commit that is at the tip of
> more than one ref needs to be guessed at the extraction end, just
> like the network-transfer case discussed in this thread.
>
> But this thread is not about updating the current bundle format to a
> new one, so any of the updates proposed in these patches will not
> affect it.
> --
I was having a quick look at the different bundle/clone routes and tried 
out (on 1.8.1.msysgit.1) the following script to see the differences 
(probably word wrap damaged):

---
cd /c/  # if on Windows to be at the top of c:/
mkdir gitBundleTest1
cd gitBundleTest1
git init
echo AAA >a.txt
git add a.txt
git commit -mfirst
git checkout -b feature
git checkout -b zulu # does this, alphabetically after master, change 
anything?
git status # observe on 'feature' branch
# one repo, one file, one commit, two branches

# test the bundle - clone transfer
git bundle create Repo.bundle --all
git clone Repo.bundle ../gitBundleTest2
cd ../gitBundleTest2
git status  # observe on wrong branch

# back to original repo
cd ../gitBundleTest1
# test the direct clone transfer
git clone . ../gitBundleTest3
cd ../gitBundleTest3
git status  # observe on wrong branch again

# back to top level (wherever that is on Msys Windows ;-)
cd ..
pwd
# test the git protocol clone transfer
# it's file:// followed by abolute path /path/to/dir so ...
# but note msys windows /c/
git clone file:///c/gitBundleTest1 ./gitBundleTest4
cd ./gitBundleTest4
git status  # observe on wrong branch again

cd ~  # return home
---

What I observed was that all the clones had the same HEAD problem, which 
I think comes from clone.c: guess_remote_head().

When I looked in the Repo.bundle file I saw the refs/heads/* listed in 
alphabetic order followed by HEAD, all with the same sha1 (in this 
case), followed by PACK and then the binary data.

My quick look at clone.c suggested to me that there would be a lot of 
commonality between the bundle data stream and the transport streams 
(identical?), and it was just a case of adding into the bundle data the 
same HEAD symref indication that would solve the normal clone problem 
(including backward compatibility). Is that a reasonable assesssment?

Philip

Re: [PATCH 1/3] upload-pack: send the HEAD information

[Jeff King]

On Fri, Sep 06, 2013 at 10:46:24AM -0700, Junio C Hamano wrote:

> I think it is perfectly fine to expose _only_ HEAD now, and wait
> until we find a good reason that we should send this information for
> other symbolic refs in the repository.

Yeah, I agree with that.

> However, because we already anticipate that we may find such a good
> reason later, on-the-wire format should be prepared to support such
> later enhancement.  I think sending
> 
> 	symref=HEAD:refs/heads/master
> 
> is probably one good way to do so, as Peff suggested in that old
> thread ($gmane/102070; note that back then this wasn't suggested as
> a proper capability so the exact format he suggests in the message
> is different).  Then we could later add advertisements for other
> symbolic refs if we find it necessary to do so, e.g.
> 
> 	symref=HEAD:refs/heads/master
>         symref=refs/remotes/origin/HEAD:refs/remotes/origin/master
> 
> (all on one line together with other capabilities separated with a
> SP in between).

It somehow feels a little weird to me that we would output the
information about refs/foo on the HEAD line. A few possible issues (and
I am playing devil's advocate to some degree here):

  1. What if we have a large number of symrefs? Would we run afoul of
     pkt-line length limits?

  2. What's the impact of having to display all symrefs on the first
     line, before we output other refs? Right now we can just stream out
     refs as we read them, but we would have to make two passes (and/or
     cache them all) to find all of the symrefs before we start
     outputting. Will the extra latency ever matter?

What do you think about teaching git to read extra data after "\0" for
_every_ ref line? And then ref advertisement might look something like:

  <sha1> HEAD\0multi_ack thin-pack ... symref=refs/heads/master\n
  <sha1> refs/heads/master\n
  <sha1> refs/heads/my-alias\0symref=refs/heads/master

That would leave us future room for more ref annotations if we should
want them, and I think (but haven't tested) that existing receivers
should ignore everything after the NUL.

-Peff

Re: [PATCH 1/3] upload-pack: send the HEAD information

[Jeff King]

On Sun, Sep 08, 2013 at 03:13:59AM -0400, Jeff King wrote:

> What do you think about teaching git to read extra data after "\0" for
> _every_ ref line? And then ref advertisement might look something like:
> 
>   <sha1> HEAD\0multi_ack thin-pack ... symref=refs/heads/master\n
>   <sha1> refs/heads/master\n
>   <sha1> refs/heads/my-alias\0symref=refs/heads/master
> 
> That would leave us future room for more ref annotations if we should
> want them, and I think (but haven't tested) that existing receivers
> should ignore everything after the NUL.

Meh, elsewhere you said:

  The capability list _could_ be sent more than once, and the
  receiving end is prepared to accept such a stream.  Everything
  learned from an older capability list needs to be forgot and only
  the last one is honored, I think.

and I think you are right. We simply keep a copy of the string the
server sent, and when we see a new one, we free the old and replace it.
So each subsequent ref would have to re-send the whole capability string
(only if it is a symref, but still, it is somewhat ugly).

-Peff

Re: [PATCH 1/3] upload-pack: send the HEAD information

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> It somehow feels a little weird to me that we would output the
> information about refs/foo on the HEAD line.

I see that you realized why the above is not the case in the
downthread; the capability list is not about describing HEAD. The
list happens to be on the line for HEAD merely because HEAD is the
first ref.

Unfortunately, the "read and replace capability if we see one" (not
"read and update capability with newly discovered one on a second
and subsequent capability list") is a restriction imposed by existing
reader side code that are deployed on the wild, so we need to stick
to it until we revamp the protocol in a backward incompatible way
(which has been discussed a few times; websearch for "who speaks
first" in the list archive).

Re: [PATCH 0/3] Unconfuse git clone when two branches at are HEAD.

[Junio C Hamano]

"Philip Oakley" <philipoakley@iee.org> writes:

> What I observed was that all the clones had the same HEAD problem,
> which I think comes from clone.c: guess_remote_head().

Yes.  They share "having to guess" property because their data
source does not tell them.

> My quick look at clone.c suggested to me that there would be a lot of
> commonality between the bundle data stream and the transport streams
> (identical?), and it was just a case of adding into the bundle data
> the same HEAD symref indication that would solve the normal clone
> problem (including backward compatibility). Is that a reasonable
> assesssment?

You need to find a hole in the existing readers to stick the new
information in a way that do not break existing readers but allow
updated readers to extract that information.  That is exactly what
we did when we added the protocol capability.  I do not offhand
think an equivalent hole exists in the bundle file format.

Re: [PATCH 0/3] Unconfuse git clone when two branches at are HEAD.

[Philip Oakley]

From: "Junio C Hamano" <gitster@pobox.com>
Sent: Sunday, September 08, 2013 6:35 PM
> "Philip Oakley" <philipoakley@iee.org> writes:
>
>> What I observed was that all the clones had the same HEAD problem,
>> which I think comes from clone.c: guess_remote_head().
>
> Yes.  They share "having to guess" property because their data
> source does not tell them.
>
>> My quick look at clone.c suggested to me that there would be a lot of
>> commonality between the bundle data stream and the transport streams
>> (identical?), and it was just a case of adding into the bundle data
>> the same HEAD symref indication that would solve the normal clone
>> problem (including backward compatibility). Is that a reasonable
>> assesssment?
>
> You need to find a hole in the existing readers to stick the new
> information in a way that do not break existing readers but allow
> updated readers to extract that information.  That is exactly what
> we did when we added the protocol capability.  I do not offhand
> think an equivalent hole exists in the bundle file format.
> --

I've been rummaging about as to options.

One is to extend the ref format such that
  <sha1> refs/heads/Test:HEAD
would be considered a valid indicator of a symref relationship (i.e. 
using the typical 'colon' style). It would be appended after the regular 
refs, so all the existing refs are still transported.

The point is that while it produces an error, it doesn't stop the 
cloning, and the error message
 "error: * Ignoring funny ref 'refs/remotes/origin/Test:HEAD' locally"
gives a pretty clear statement of intent to those with older versions of 
git.

Another alternative is to add an additional name space (e.g.)
   <sha1> refs/remotes/origin/HEAD/Test
which would simply be an extra directory layer that reflects where the 
HEAD should have been. Though this namespace example has the D/F 
conflict.

Philip

Re: [PATCH 0/3] Unconfuse git clone when two branches at are HEAD.

[Junio C Hamano]

"Philip Oakley" <philipoakley@iee.org> writes:

> One is to extend the ref format such that
>  <sha1> refs/heads/Test:HEAD
> would be considered a valid indicator of a symref relationship
> (i.e. using the typical 'colon' style). It would be appended after the
> regular refs, so all the existing refs are still transported.
>
> The point is that while it produces an error, it doesn't stop the
> cloning, and the error message
> "error: * Ignoring funny ref 'refs/remotes/origin/Test:HEAD' locally"
> gives a pretty clear statement of intent to those with older versions
> of git.

Cute.  If it does not stop any of these:

        git ls-remote such.bundle
	git clone such.bundle
        git fetch such.bundle
	git fetch such.bundle master ;# if 'master' branch is in it
        git ls-remote such.bundle
        git ls-remote such.bundle master ;# if 'master' branch is in it

even if some of them may give error messages, I think that may be a
workable escape hatch.

> Another alternative is to add an additional name space (e.g.)
>   <sha1> refs/remotes/origin/HEAD/Test
> which would simply be an extra directory layer that reflects where the
> HEAD should have been. Though this namespace example has the D/F
> conflict.

I'd rather not go this route.  Allowing refs/heads/master and local
branches that forked from it in refs/heads/master/{a,b,c,...} could
be a potentially useful future enhancement, and this approach will
close the door for it.

Re: [PATCH 0/3] Unconfuse git clone when two branches at are HEAD.

[Andreas Krey]

On Mon, 09 Sep 2013 07:44:04 +0000, Junio C Hamano wrote:
...
> I'd rather not go this route.  Allowing refs/heads/master and local
> branches that forked from it in refs/heads/master/{a,b,c,...} could
> be a potentially useful future enhancement,

Want! We're currently going the route of naming the branches

  master
  feature/master
  feature/subfeature/master

to allow feature/subfeature/topic, and feature/subfeature in the first place.

(Other hierarchy separator candidates were ugly, shell-unwieldy, already
commonly used within branch names, or illegal.)

Andreas

-- 
"Totally trivial. Famous last words."
From: Linus Torvalds <torvalds@*.org>
Date: Fri, 22 Jan 2010 07:29:21 -0800

Re: [PATCH 0/3] Unconfuse git clone when two branches at are HEAD.

[Philip Oakley]

From: "Junio C Hamano" <gitster@pobox.com>
Sent: Monday, September 09, 2013 3:44 PM
> "Philip Oakley" <philipoakley@iee.org> writes:
>
>> One is to extend the ref format such that
>>  <sha1> refs/heads/Test:HEAD
>> would be considered a valid indicator of a symref relationship
>> (i.e. using the typical 'colon' style). It would be appended after 
>> the
>> regular refs, so all the existing refs are still transported.
>>
>> The point is that while it produces an error, it doesn't stop the
>> cloning, and the error message
>> "error: * Ignoring funny ref 'refs/remotes/origin/Test:HEAD' locally"
>> gives a pretty clear statement of intent to those with older versions
>> of git.
>
> Cute.  If it does not stop any of these:
>
>        git ls-remote such.bundle
> git clone such.bundle
>        git fetch such.bundle
> git fetch such.bundle master ;# if 'master' branch is in it
>        git ls-remote such.bundle
>        git ls-remote such.bundle master ;# if 'master' branch is in it
>
> even if some of them may give error messages, I think that may be a
> workable escape hatch.
>
These look to work OK. Not sure If I've properly covered all the 
options.

A nice feature is that ls-remote will find the fake ref !

$ git ls-remote /c/gitBundleTest1/RepoHEADnomaster.bundle Test:HEAD
41ccb18028d1cb6516251e94cef1cd5cb3f0bcb5        refs/heads/Test:HEAD

It's only the clone that barfs (so far) (which could be 'fixed').

Obviously, if it can be made to work, one check would be that the two 
refs (HEAD and refs/heads/wherever) point to the came commit before 
generating the HEAD symref.

Philip 

Re: [PATCH 0/3] Unconfuse git clone when two branches at are HEAD.

[Junio C Hamano]

"Philip Oakley" <philipoakley@iee.org> writes:

> These look to work OK. Not sure If I've properly covered all the
> options.
>
> A nice feature is that ls-remote will find the fake ref !
>
> $ git ls-remote /c/gitBundleTest1/RepoHEADnomaster.bundle Test:HEAD
> 41ccb18028d1cb6516251e94cef1cd5cb3f0bcb5        refs/heads/Test:HEAD
>
> It's only the clone that barfs (so far) (which could be 'fixed').

You cannot 'fix' the ones deployed in the wild, but I think saying
"funny ref" and not aborting is a good compromise.  The updated
documentation for bundle can mention that error message and explain
that the older version of "clone" will say that in what situation.

I didn't notice it the first time, but I think the above would read
better and match what has been discussed on the on-the-wire format
to swap the order, i.e. use "HEAD:refs/heads/Test" to mean "HEAD is
a symref that points at the Test branch".

> Obviously, if it can be made to work, one check would be that the two
> refs (HEAD and refs/heads/wherever) point to the came commit before
> generating the HEAD symref.

Yes, that is a sensible check.

[PATCH v2 0/6] Removing the guesswork of HEAD in "clone"

[Junio C Hamano]

This reworks the old idea from 2008 ($gmane/102039) to teach
upload-pack to say where symbolic refs are pointing at in the
initial ref advertisement as a new capability "sym", and allow
"git clone" to take advantage of that knowledge when deciding what
branch to point at with the HEAD of the newly created repository.

Thanks go to Andreas Krey for reigniting the ember in a patch series
a few weeks ago.

I did not do anything more than just compile it once; all the bugs
in this round are mine (it is all new code after all).


Junio C Hamano (6):
  upload-pack.c: do not pass confusing cb_data to mark_our_ref()
  upload-pack: send symbolic ref information as capability
  upload-pack: send non-HEAD symbolic refs
  connect.c: make parse_feature_value() static
  connect: annotate refs with their symref information in get_remote_head()
  clone: test the new HEAD detection logic

 cache.h          |  1 -
 connect.c        | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 t/t5601-clone.sh | 11 ++++++++++
 upload-pack.c    | 51 +++++++++++++++++++++++++++++++++++++++------
 4 files changed, 118 insertions(+), 8 deletions(-)

-- 
1.8.4-585-g8d1dcaf

[PATCH v2 1/6] upload-pack.c: do not pass confusing cb_data to mark_our_ref()

[Junio C Hamano]

The callee does not use cb_data, and the caller is an intermediate
function in a callchain that later wants to use the cb_data for its
own use.  Clarify the code by breaking the dataflow explicitly by
not passing cb_data down to mark_our_ref().

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 upload-pack.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/upload-pack.c b/upload-pack.c
index 127e59a..a6e107f 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -742,7 +742,7 @@ static int send_ref(const char *refname, const unsigned char *sha1, int flag, vo
 	const char *refname_nons = strip_namespace(refname);
 	unsigned char peeled[20];
 
-	if (mark_our_ref(refname, sha1, flag, cb_data))
+	if (mark_our_ref(refname, sha1, flag, NULL))
 		return 0;
 
 	if (capabilities)
-- 
1.8.4-585-g8d1dcaf

[PATCH v2 2/6] upload-pack: send symbolic ref information as capability

[Junio C Hamano]

One long-standing flaw in the pack transfer protocol was that there
was no way to tell the other end which branch "HEAD" points at.
With a new "sym" capability (e.g. "sym=HEAD:refs/heads/master"; can
be repeated more than once to represent symbolic refs other than
HEAD, such as "refs/remotes/origin/HEAD").

Add an infrastructure to collect symbolic refs, format them as extra
capabilities and put it on the wire.  For now, just send information
on the "HEAD" and nothing else.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 upload-pack.c | 48 +++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 43 insertions(+), 5 deletions(-)

diff --git a/upload-pack.c b/upload-pack.c
index a6e107f..53958b9 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -734,6 +734,16 @@ static int mark_our_ref(const char *refname, const unsigned char *sha1, int flag
 	return 0;
 }
 
+static void format_symref_info(struct strbuf *buf, struct string_list *symref)
+{
+	struct string_list_item *item;
+
+	if (!symref->nr)
+		return;
+	for_each_string_list_item(item, symref)
+		strbuf_addf(buf, " sym=%s:%s", item->string, (char *)item->util);
+}
+
 static int send_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data)
 {
 	static const char *capabilities = "multi_ack thin-pack side-band"
@@ -745,32 +755,60 @@ static int send_ref(const char *refname, const unsigned char *sha1, int flag, vo
 	if (mark_our_ref(refname, sha1, flag, NULL))
 		return 0;
 
-	if (capabilities)
-		packet_write(1, "%s %s%c%s%s%s agent=%s\n",
+	if (capabilities) {
+		struct strbuf symref_info = STRBUF_INIT;
+
+		format_symref_info(&symref_info, cb_data);
+		packet_write(1, "%s %s%c%s%s%s%s agent=%s\n",
 			     sha1_to_hex(sha1), refname_nons,
 			     0, capabilities,
 			     allow_tip_sha1_in_want ? " allow-tip-sha1-in-want" : "",
 			     stateless_rpc ? " no-done" : "",
+			     symref_info.buf,
 			     git_user_agent_sanitized());
-	else
+		strbuf_release(&symref_info);
+	} else {
 		packet_write(1, "%s %s\n", sha1_to_hex(sha1), refname_nons);
+	}
 	capabilities = NULL;
 	if (!peel_ref(refname, peeled))
 		packet_write(1, "%s %s^{}\n", sha1_to_hex(peeled), refname_nons);
 	return 0;
 }
 
+static int find_symref(const char *refname, const unsigned char *sha1, int flag,
+		       void *cb_data)
+{
+	const char *symref_target;
+	struct string_list_item *item;
+	unsigned char unused[20];
+
+	if ((flag & REF_ISSYMREF) == 0)
+		return 0;
+	symref_target = resolve_ref_unsafe(refname, unused, 0, &flag);
+	if (!symref_target || (flag & REF_ISSYMREF) == 0)
+		die("'%s' is a symref but it is not?", refname);
+	item = string_list_append(cb_data, refname);
+	item->util = xstrdup(symref_target);
+	return 0;
+}
+
 static void upload_pack(void)
 {
+	struct string_list symref = STRING_LIST_INIT_DUP;
+
+	head_ref_namespaced(find_symref, &symref);
+
 	if (advertise_refs || !stateless_rpc) {
 		reset_timeout();
-		head_ref_namespaced(send_ref, NULL);
+		head_ref_namespaced(send_ref, &symref);
 		for_each_namespaced_ref(send_ref, NULL);
 		packet_flush(1);
 	} else {
-		head_ref_namespaced(mark_our_ref, NULL);
+		head_ref_namespaced(mark_our_ref, &symref);
 		for_each_namespaced_ref(mark_our_ref, NULL);
 	}
+	string_list_clear(&symref, 1);
 	if (advertise_refs)
 		return;
 
-- 
1.8.4-585-g8d1dcaf

[PATCH v2 3/6] upload-pack: send non-HEAD symbolic refs

[Junio C Hamano]

With the same mechanism as used to tell where "HEAD" points at to
the other end, we can tell the target of other symbolic refs as
well.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 upload-pack.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/upload-pack.c b/upload-pack.c
index 53958b9..7ca6154 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -798,6 +798,7 @@ static void upload_pack(void)
 	struct string_list symref = STRING_LIST_INIT_DUP;
 
 	head_ref_namespaced(find_symref, &symref);
+	for_each_namespaced_ref(find_symref, &symref);
 
 	if (advertise_refs || !stateless_rpc) {
 		reset_timeout();
-- 
1.8.4-585-g8d1dcaf

[PATCH v2 4/6] connect.c: make parse_feature_value() static

[Junio C Hamano]

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 cache.h   | 1 -
 connect.c | 3 ++-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/cache.h b/cache.h
index 85b544f..2c853ba 100644
--- a/cache.h
+++ b/cache.h
@@ -1098,7 +1098,6 @@ extern struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
 extern int server_supports(const char *feature);
 extern int parse_feature_request(const char *features, const char *feature);
 extern const char *server_feature_value(const char *feature, int *len_ret);
-extern const char *parse_feature_value(const char *feature_list, const char *feature, int *len_ret);
 
 extern struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path);
 
diff --git a/connect.c b/connect.c
index a0783d4..e4c7ae6 100644
--- a/connect.c
+++ b/connect.c
@@ -8,6 +8,7 @@
 #include "url.h"
 
 static char *server_capabilities;
+static const char *parse_feature_value(const char *, const char *, int *);
 
 static int check_ref(const char *name, int len, unsigned int flags)
 {
@@ -116,7 +117,7 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
 	return list;
 }
 
-const char *parse_feature_value(const char *feature_list, const char *feature, int *lenp)
+static const char *parse_feature_value(const char *feature_list, const char *feature, int *lenp)
 {
 	int len;
 
-- 
1.8.4-585-g8d1dcaf

[PATCH v2 5/6] connect: annotate refs with their symref information in get_remote_head()

[Junio C Hamano]

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 connect.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

diff --git a/connect.c b/connect.c
index e4c7ae6..a53ef6d 100644
--- a/connect.c
+++ b/connect.c
@@ -6,6 +6,7 @@
 #include "run-command.h"
 #include "remote.h"
 #include "url.h"
+#include "string-list.h"
 
 static char *server_capabilities;
 static const char *parse_feature_value(const char *, const char *, int *);
@@ -60,6 +61,61 @@ static void die_initial_contact(int got_at_least_one_head)
 		    "and the repository exists.");
 }
 
+static void parse_one_symref_info(struct string_list *symref, const char *val, int len)
+{
+	char *sym, *target;
+	struct string_list_item *item;
+
+	if (!len)
+		return; /* just "sym" */
+	/* e.g. "sym=HEAD:refs/heads/master" */
+	sym = xmalloc(len + 1);
+	memcpy(sym, val, len);
+	sym[len] = '\0';
+	target = strchr(sym, ':');
+	if (!target)
+		/* just "sym=something" */
+		goto reject;
+	*(target++) = '\0';
+	if (check_refname_format(sym, REFNAME_ALLOW_ONELEVEL) ||
+	    check_refname_format(target, REFNAME_ALLOW_ONELEVEL))
+		/* "sym=bogus:pair */
+		goto reject;
+	item = string_list_append(symref, sym);
+	item->util = target;
+	return;
+reject:
+	free(sym);
+	return;
+}
+
+static void annotate_refs_with_symref_info(struct ref *ref)
+{
+	struct string_list symref = STRING_LIST_INIT_DUP;
+	const char *feature_list = server_capabilities;
+
+	while (feature_list) {
+		int len;
+		const char *val;
+
+		val = parse_feature_value(feature_list, "sym", &len);
+		if (!val)
+			break;
+		parse_one_symref_info(&symref, val, len);
+		feature_list = val + 1;
+	}
+	sort_string_list(&symref);
+
+	for (; ref; ref = ref->next) {
+		struct string_list_item *item;
+		item = string_list_lookup(&symref, ref->name);
+		if (!item)
+			continue;
+		ref->symref = xstrdup((char *)item->util);
+	}
+	string_list_clear(&symref, 0);
+}
+
 /*
  * Read all the refs from the other end
  */
@@ -67,6 +123,7 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
 			      struct ref **list, unsigned int flags,
 			      struct extra_have_objects *extra_have)
 {
+	struct ref **orig_list = list;
 	int got_at_least_one_head = 0;
 
 	*list = NULL;
@@ -114,6 +171,9 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
 		list = &ref->next;
 		got_at_least_one_head = 1;
 	}
+
+	annotate_refs_with_symref_info(*orig_list);
+
 	return list;
 }
 
-- 
1.8.4-585-g8d1dcaf

[PATCH v2 6/6] clone: test the new HEAD detection logic

[Junio C Hamano]

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t5601-clone.sh | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 0629149..ccda6df 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -285,4 +285,15 @@ test_expect_success NOT_MINGW,NOT_CYGWIN 'clone local path foo:bar' '
 	git clone "./foo:bar" foobar
 '
 
+test_expect_success 'clone from a repository with two identical branches' '
+
+	(
+		cd src &&
+		git checkout -b another master
+	) &&
+	git clone src target-11 &&
+	test "z$( cd target-11 && git symbolic-ref HEAD )" = zrefs/heads/another
+
+'
+
 test_done
-- 
1.8.4-585-g8d1dcaf

Re: [PATCH v2 2/6] upload-pack: send symbolic ref information as capability

[Junio C Hamano]

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

>  static void upload_pack(void)
>  {
> +	struct string_list symref = STRING_LIST_INIT_DUP;
> +
> +	head_ref_namespaced(find_symref, &symref);
> +
>  	if (advertise_refs || !stateless_rpc) {
>  		reset_timeout();
> -		head_ref_namespaced(send_ref, NULL);
> +		head_ref_namespaced(send_ref, &symref);
>  		for_each_namespaced_ref(send_ref, NULL);

This one was trying to be too clever; HEAD may be pointing at an
unborn branch in which case head_ref_namespaced() will not emit
the capability bit, so the second line also needs to be

	for_each_namespaced_ref(send_ref, &symref);