[PATCH 0/6 (v2)] Detecting HEAD more reliably while cloning

[PATCH 0/6 (v2)] Detecting HEAD more reliably while cloning

[Junio C Hamano]

This is another approach to the same problem that a repository cloned from
another repository whose default branch is not 'master' can use 'master'
as the default.

The current code has to guess where the HEAD in the original repository
points at, because the original protocol tells what object each ref points
at but does not talk about which other ref a symbolic ref points at.  The
implication of this is that if you prepare another branch that points at
your master, like this:

	$ git checkout -b another master

and keep that other branch checked out (and in sync with 'master'), a
clone made from such a repository may incorrectly have its HEAD pointing
at 'master', not 'another'.

Instead of introducing a full-fledged protocol extension, this round hides
the new information in the same place as the server capabilities list that
is used to implement protocol extension is hidden from older clients.
This way, it does not have to work around the code structure imposed by
the transport API, does not have to introduce an extra round trip, and
does not have to trigger an annoying (but harmless) error message when an
older client contacts a new uploader.

  [1/6] get_remote_heads(): refactor code to read "server capabilities"
  [2/6] connect.c::read_extra_info(): prepare to receive more than server
        capabilities
  [3/6] connect.c::read_extra_info(): find where HEAD points at
  [4/6] clone: find the current branch more explicitly
  [5/6] upload-pack: send the HEAD information
  [6/6] clone: test the new HEAD detection logic

The first four are the client side, the fifth one is the uploader side,
and the last one is the test.  After storing these patches in separate
files, you would build this history (on top of 'master'):

    git am 1 2 3 4
    git reset --hard HEAD~4    5---------------M---6
    git am 5                  /               /
    git merge HEAD@{2}        ---1---2---3---4
    git am 6

 builtin-clone.c  |   24 +++++++++++++++++++-----
 connect.c        |   47 +++++++++++++++++++++++++++++++++++++++++++----
 t/t5601-clone.sh |   11 +++++++++++
 upload-pack.c    |   14 +++++++++++---
 4 files changed, 84 insertions(+), 12 deletions(-)

[PATCH 1/6 (v2)] get_remote_heads(): refactor code to read "server capabilities"

[Junio C Hamano]

This just moves the code to read the server capability list that is hidden
after "40-byte hex object name, SP, refname, NUL" on the information the
uploading end initially sends.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 connect.c |   21 +++++++++++++++++----
 1 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/connect.c b/connect.c
index 584e04c..932c503 100644
--- a/connect.c
+++ b/connect.c
@@ -49,6 +49,21 @@ static void add_extra_have(struct extra_have_objects *extra, unsigned char *sha1
 }
 
 /*
+ * "line" points at the byte just after "40-byte hex, SP, refname,
+ * NUL".  "len" is the remaining number of bytes.  The caller knows
+ * that the original packet contains more information than that.
+ */
+static void read_extra_info(char *line, int len)
+{
+	/*
+	 * The first such "extra" piece of information is the list of
+	 * server capabilities.
+	 */
+	free(server_capabilities);
+	server_capabilities = xstrdup(line);
+}
+
+/*
  * Read all the refs from the other end
  */
 struct ref **get_remote_heads(int in, struct ref **list,
@@ -78,10 +93,8 @@ struct ref **get_remote_heads(int in, struct ref **list,
 		name = buffer + 41;
 
 		name_len = strlen(name);
-		if (len != name_len + 41) {
-			free(server_capabilities);
-			server_capabilities = xstrdup(name + name_len + 1);
-		}
+		if (len != name_len + 41)
+			read_extra_info(name + name_len + 1, len - (name_len + 41));
 
 		if (extra_have &&
 		    name_len == 5 && !memcmp(".have", name, 5)) {
-- 
1.6.0.4.864.g0f47a

[PATCH 2/6 (v2)] connect.c::read_extra_info(): prepare to receive more than server capabilities

[Junio C Hamano]

This still does not actually parse and accept anything more than the list
of server capabilities, but prepares for the uploader that sends more
"extra" information than that.

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

diff --git a/connect.c b/connect.c
index 932c503..114d691 100644
--- a/connect.c
+++ b/connect.c
@@ -59,8 +59,11 @@ static void read_extra_info(char *line, int len)
 	 * The first such "extra" piece of information is the list of
 	 * server capabilities.
 	 */
+	int infolen = strlen(line) + 1;
+
 	free(server_capabilities);
-	server_capabilities = xstrdup(line);
+	server_capabilities = xmalloc(infolen);
+	memcpy(server_capabilities, line, infolen);
 }
 
 /*
-- 
1.6.0.4.864.g0f47a

[PATCH 3/6 (v2)] connect.c::read_extra_info(): find where HEAD points at

[Junio C Hamano]

This actually implements the protocol extension to receive the HEAD symref
information that is hidden after the server capabilities list.  Nobody
uses the information yet, though.

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

diff --git a/connect.c b/connect.c
index 114d691..aa4757a 100644
--- a/connect.c
+++ b/connect.c
@@ -53,7 +53,7 @@ static void add_extra_have(struct extra_have_objects *extra, unsigned char *sha1
  * NUL".  "len" is the remaining number of bytes.  The caller knows
  * that the original packet contains more information than that.
  */
-static void read_extra_info(char *line, int len)
+static void read_extra_info(char *line, int len, char **hpa)
 {
 	/*
 	 * The first such "extra" piece of information is the list of
@@ -64,6 +64,20 @@ static void read_extra_info(char *line, int len)
 	free(server_capabilities);
 	server_capabilities = xmalloc(infolen);
 	memcpy(server_capabilities, line, infolen);
+	if (infolen == len)
+		return;
+	/*
+	 * The uploader can optionally tell us where the HEAD pointer
+	 * points at after that.
+	 */
+	line += infolen;
+	len -= infolen;
+
+	infolen = strlen(line) + 1;
+
+	free(*hpa);
+	*hpa = xmalloc(infolen);
+	memcpy(*hpa, line, infolen);
 }
 
 /*
@@ -74,6 +88,9 @@ struct ref **get_remote_heads(int in, struct ref **list,
 			      unsigned int flags,
 			      struct extra_have_objects *extra_have)
 {
+	struct ref **ref_list = list;
+	char *head_points_at = NULL;
+
 	*list = NULL;
 	for (;;) {
 		struct ref *ref;
@@ -97,7 +114,8 @@ struct ref **get_remote_heads(int in, struct ref **list,
 
 		name_len = strlen(name);
 		if (len != name_len + 41)
-			read_extra_info(name + name_len + 1, len - (name_len + 41));
+			read_extra_info(name + name_len + 1, len - (name_len + 41),
+					&head_points_at);
 
 		if (extra_have &&
 		    name_len == 5 && !memcmp(".have", name, 5)) {
@@ -114,6 +132,11 @@ struct ref **get_remote_heads(int in, struct ref **list,
 		*list = ref;
 		list = &ref->next;
 	}
+	if (head_points_at) {
+		struct ref *head = find_ref_by_name(*ref_list, "HEAD");
+		if (head)
+			head->symref = head_points_at;
+	}
 	return list;
 }
 
-- 
1.6.0.4.864.g0f47a

[PATCH 4/6 (v2)] clone: find the current branch more explicitly

[Junio C Hamano]

This makes "git clone" use the HEAD information sent by an updated
uploader to point the resulting HEAD at the current branch that is not
'master' but happens to point at the same commit as 'master'.

IOW, immediately after doing:

    $ git checkout -b another master

a clone made out of that repository will check out 'another' branch, not
'master'.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-clone.c |   24 +++++++++++++++++++-----
 1 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/builtin-clone.c b/builtin-clone.c
index 2feac9c..a8b8d56 100644
--- a/builtin-clone.c
+++ b/builtin-clone.c
@@ -299,13 +299,27 @@ static const struct ref *locate_head(const struct ref *refs,
 	const struct ref *remote_head = NULL;
 	const struct ref *remote_master = NULL;
 	const struct ref *r;
-	for (r = refs; r; r = r->next)
-		if (!strcmp(r->name, "HEAD"))
+
+	for (r = refs; r; r = r->next) {
+		if (!strcmp(r->name, "HEAD")) {
 			remote_head = r;
+			break;
+		}
+	}
 
-	for (r = mapped_refs; r; r = r->next)
-		if (!strcmp(r->name, "refs/heads/master"))
-			remote_master = r;
+	if (remote_head && remote_head->symref) {
+		for (r = mapped_refs; r; r = r->next)
+			if (!strcmp(r->name, remote_head->symref)) {
+				remote_master = r;
+				break;
+			}
+	}
+
+	if (!remote_master) {
+		for (r = mapped_refs; r; r = r->next)
+			if (!strcmp(r->name, "refs/heads/master"))
+				remote_master = r;
+	}
 
 	if (remote_head_p)
 		*remote_head_p = remote_head;
-- 
1.6.0.4.864.g0f47a

[PATCH 5/6 (v2)] upload-pack: send the HEAD information

[Junio C Hamano]

This implements the server side of protocol extension to show which branch
the HEAD points at.  The information is sent after the terminating NUL
that comes after the server capabilities list, to cause older clients to
ignore it, while allowing newer clients to make use of that information

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 upload-pack.c |   14 +++++++++++---
 1 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/upload-pack.c b/upload-pack.c
index e5adbc0..4aa444a 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -579,9 +579,17 @@ static int send_ref(const char *refname, const unsigned char *sha1, int flag, vo
 	if (!o)
 		die("git upload-pack: cannot find object %s:", sha1_to_hex(sha1));
 
-	if (capabilities)
-		packet_write(1, "%s %s%c%s\n", sha1_to_hex(sha1), refname,
-			0, capabilities);
+	if (capabilities) {
+		unsigned char dummy[20];
+		const char *target = resolve_ref("HEAD", dummy, 0, NULL);
+
+		if (!target)
+			packet_write(1, "%s %s%c%s\n", sha1_to_hex(sha1), refname,
+				     0, capabilities);
+		else
+			packet_write(1, "%s %s%c%s%c%s\n", sha1_to_hex(sha1), refname,
+				     0, capabilities, 0, target);
+	}
 	else
 		packet_write(1, "%s %s\n", sha1_to_hex(sha1), refname);
 	capabilities = NULL;
-- 
1.6.0.4.864.g0f47a

[PATCH 6/6 (v2)] 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 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 78a3fa6..6d4665b 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -125,4 +125,15 @@ test_expect_success 'clone to destination with extra trailing /' '
 
 '
 
+test_expect_success 'clone from a repository with two identical branches' '
+
+	(
+		cd src &&
+		git checkout -b another master
+	) &&
+	git clone src target-3 &&
+	test "z$( cd target-3 && git symbolic-ref HEAD )" = zrefs/heads/another
+
+'
+
 test_done
-- 
1.6.0.4.864.g0f47a

Re: [PATCH 5/6 (v2)] upload-pack: send the HEAD information

[Jakub Narebski]

Junio C Hamano wrote:

> This implements the server side of protocol extension to show which branch
> the HEAD points at.  The information is sent after the terminating NUL
> that comes after the server capabilities list, to cause older clients to
> ignore it, while allowing newer clients to make use of that information

By the way, is negotiating protocol extensions, and supported protocol
extensions documented somewhere in Documentation/technical/ ?

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Re: [PATCH 5/6 (v2)] upload-pack: send the HEAD information

[Shawn O. Pearce]

Junio C Hamano <gitster@pobox.com> wrote:
> This implements the server side of protocol extension to show which branch
> the HEAD points at.  The information is sent after the terminating NUL
> that comes after the server capabilities list, to cause older clients to
> ignore it, while allowing newer clients to make use of that information

Ok, not to paint the bikeshed another color or anything ... but
can we do something to make this slightly more extensible?  I like
the "lets hide it behind the NUL" bit a lot better than the prior
iteration, but I wonder if we shouldn't do something slightly
different.

Maybe we put on the first capability line a flag that lets the
client know we have symref data in the advertised list, and then
instead of sticking only HEAD into that first ref we put the names
of the symrefs after the ref they point to.

So we might see something like:

  xxxx......................... refs/heads/boo\0with-symref\0
  xxxx......................... refs/heads/master\0HEAD\0
  xxxx......................... refs/remotes/origin/HEAD\0refs/remotes/origin/master\0

etc.  Its probably harder to produce the output for, but it permits
advertising all of the symrefs on the remote side, which may be good
for --mirror, among other uses.  It also should make it easier to put
multiple symrefs down pointing at the same real ref, they could just
be a space delimited list stored after the ref name, and if its the
first ref in the stream, after the other capability advertisement.

Actually, since the capability line is space delimited and space is
not valid in a ref name, we could just include into the capability
line like "symref=HEAD", but I still like the idea of listing it
after each ref, to reduce the risk of running into pkt-line length
limitations.

-- 
Shawn.

Re: [PATCH 5/6 (v2)] upload-pack: send the HEAD information

[Junio C Hamano]

"Shawn O. Pearce" <spearce@spearce.org> writes:

> Maybe we put on the first capability line a flag that lets the
> client know we have symref data in the advertised list, and then
> instead of sticking only HEAD into that first ref we put the names
> of the symrefs after the ref they point to.
>
> So we might see something like:
>
>   xxxx......................... refs/heads/boo\0with-symref\0
>   xxxx......................... refs/heads/master\0HEAD\0
>   xxxx......................... refs/remotes/origin/HEAD\0refs/remotes/origin/master\0
>
> etc.  Its probably harder to produce the output for, but it permits
> advertising all of the symrefs on the remote side, which may be good
> for --mirror, among other uses.  It also should make it easier to put
> multiple symrefs down pointing at the same real ref, they could just
> be a space delimited list stored after the ref name, and if its the
> first ref in the stream, after the other capability advertisement.

It certainly is possible, and I think the arrangement v2 code makes
already keeps that option to talk about symrefs other than HEAD open.  If
you want to send all the symref information, you would show something
like:

    xxxx... HEAD\0<caps>\0refs/heads/master\n
    xxxx... refs/heads/master\n
    xxxx... refs/remotes/origin/HEAD\0<caps>\0refs/remotes/origin/master\n
    xxxx... refs/remotes/origin/master\n

But in this round I am not interested in giving any "random symref"
information but "HEAD", so I omitted it from the code.  

Notice that you need to repeat the capabilities list on each and every
line that describes a symbolic ref for that to work (and you do not need
"with-symref"), though.  See what ll. 80-84 in connect.c does.

	if (len != name_len + 41) {
		free(server_capabilities);
		server_capabilities = xstrdup(name + name_len + 1);
	}

Historical accident mandates that the first hidden piece of information on
each and all of these lines _must_ be the capabilities list.

Re: [PATCH 5/6 (v2)] upload-pack: send the HEAD information

[Jeff King]

On Mon, Dec 01, 2008 at 06:12:54AM -0800, Junio C Hamano wrote:

> +			packet_write(1, "%s %s%c%s%c%s\n", sha1_to_hex(sha1), refname,
> +				     0, capabilities, 0, target);

Yuck. My two complaints are:

  (1) this implicitly handles only the HEAD symref. I don't think any
      others are in common use, but the rest of git handles arbitrary
      symrefs just fine. It would be a shame to needlessly limit the
      protocol. Can we at least make it <ref>:<ref> to allow later
      expansion to other symrefs?

      (1a) As a follow-on to that, because the client is not requesting
      anything, how would we ask for other symrefs if we want to do so
      later?  I think it would be nice to eventually allow copying of
      arbitrary symrefs within the refs/* hierarchy (e.g.,
      project-specific branch aliases). Sending all symrefs right off
      the bat is potentially large and wasteful.

  (2) You've used up the first such expansion slot forever. Now it's "if
      I want to tell you the symref, there is an extra slot, and
      otherwise none". But if we ever want to use the _next_ slot, then
      you will always have to send this slot (blank, I guess?). It gets
      even more complicated if you ever want to an arbitrary number of
      symref mappings. Maybe a short header to say "this slot contains a
      symref target"?

So (1) and (2) together would make it something like:

   <capabilities>\0
   symref HEAD:refs/heads/master\0
   symref refs/heads/alias:refs/heads/branch\n

which would make adding any new features in the expansion slots easier.
But that still doesn't address (1a). I really like the other proposal a
lot better.

-Peff

Re: [PATCH 5/6 (v2)] upload-pack: send the HEAD information

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Dec 01, 2008 at 06:12:54AM -0800, Junio C Hamano wrote:
>
>> +			packet_write(1, "%s %s%c%s%c%s\n", sha1_to_hex(sha1), refname,
>> +				     0, capabilities, 0, target);
>
> Yuck. My two complaints are:
>
>   (1) this implicitly handles only the HEAD symref.

But this information *is* on the "40-hex name" line that describes the
HEAD ;-)

You can trivially extend it to add this to other symbolic refs if you are
interested.  I wasn't.

Re: [PATCH 5/6 (v2)] upload-pack: send the HEAD information

[Jeff King]

On Mon, Dec 01, 2008 at 05:31:16PM -0800, Junio C Hamano wrote:

> > Yuck. My two complaints are:
> >
> >   (1) this implicitly handles only the HEAD symref.
> 
> But this information *is* on the "40-hex name" line that describes the
> HEAD ;-)

Ahhh. OK. I see, I misunderstood the protocol a bit. So let me revise my
complaints. ;)

  (1) If there are multiple symrefs to report, we have to keep
      re-sending the server capabilities (I think you mentioned this in
      the subthread with Shawn).

  (2) Similar to (2) before. If we ever add a _third_ extra slot, we
      will be stuck sending the symref field each time we want to use
      that third slot (though if the client properly treats an empty
      slot the same as non-existent, this only wastes a single byte for
      the NUL).

  (3) If HEAD doesn't point to a valid object, we have no place to put
      the symref. Or is it kosher to say

        0000000000000000000000000000000000000000 HEAD\0...\0refs/heads/master

      ? I think it would be nice eventually to be able to clone a HEAD
      pointing to yet-to-be-born branch.

-Peff

Re: [PATCH 5/6 (v2)] upload-pack: send the HEAD information

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>   (1) If there are multiple symrefs to report, we have to keep
>       re-sending the server capabilities (I think you mentioned this in
>       the subthread with Shawn).

This is nothing new, so there is nothing to complain about here.

The ref advertisement section is not very extensible to begin with and we
should consider ourselves lucky that Sergey Vlasov found the hole
(cf. $gmane/10710) we are exploiting to carry "server capabilities"; I do
not think we should nor can stuff any more than absolute minimum in the
section.

>   (3) If HEAD doesn't point to a valid object, we have no place to put
>       the symref. Or is it kosher to say
>
>         0000000000000000000000000000000000000000 HEAD\0...\0refs/heads/master
>
>       ? I think it would be nice eventually to be able to clone a HEAD
>       pointing to yet-to-be-born branch.

I think sending 0{40} would break older clients, but older clients cannot
clone from an empty repository anyway, so that should not be so bad.  I
however do not think it is such a big thing to be able to clone void
anyway.

You just have to train yourself to announce that your repository is
clonable _after_ making it actually clonable.

Re: [PATCH 5/6 (v2)] upload-pack: send the HEAD information

[Jeff King]

On Mon, Dec 01, 2008 at 06:20:52PM -0800, Junio C Hamano wrote:

> I think sending 0{40} would break older clients, but older clients cannot
> clone from an empty repository anyway, so that should not be so bad.  I
> however do not think it is such a big thing to be able to clone void
> anyway.
> 
> You just have to train yourself to announce that your repository is
> clonable _after_ making it actually clonable.

I disagree. 99% of the time we see people complain about this, it is not
"I tried to announce my repo to people but it didn't have any commits"
but rather "why must I hack, init remote, then push, instead of init
remote, hack, push"[1].

That is, some segment of people (myself included) want to say first "I'm
starting a new project, and so I'm going to create a spot for it on my
server". Of course we can train ourselves to do it in the other order,
but it is an unnecessary complication.

-Peff

[1] Actually, it is more than just arbitrary preference. It is less
typing to do:

    ssh remote 'mkdir foo && cd foo && git init --bare'
    git clone remote:foo
    hack hack hack; commit
    git push

than

    git init
    hack hack hack; commit
    ssh remote 'mkdir foo && cd foo && git init --bare'
    git push
    git remote add -m refs/heads/master origin remote:foo

Re: [PATCH 0/6 (v2)] Detecting HEAD more reliably while cloning

[Johannes Sixt]

Junio C Hamano schrieb:
> Instead of introducing a full-fledged protocol extension, this round hides
> the new information in the same place as the server capabilities list that
> is used to implement protocol extension is hidden from older clients.

Not that it makes a lot of difference, but why do you want to *hide* the
information? Can't we just have a capability-with-parameter:

 ... shallow no-progress include-tag head=refs/heads/foo\ bar ...

(with spaces and backslashes escaped)?

-- Hannes

Re: [PATCH 0/6 (v2)] Detecting HEAD more reliably while cloning

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> Junio C Hamano schrieb:
>> Instead of introducing a full-fledged protocol extension, this round hides
>> the new information in the same place as the server capabilities list that
>> is used to implement protocol extension is hidden from older clients.
>
> Not that it makes a lot of difference, but why do you want to *hide* the
> information? Can't we just have a capability-with-parameter:
>
>  ... shallow no-progress include-tag head=refs/heads/foo\ bar ...
>
> (with spaces and backslashes escaped)?

The ref namespace is reasonably tight (most importantly I do not think you
can have space) so there is no need for quoting.  If we were to go that
route of making them extended "capabilities", the right syntax would be

    ... symref-HEAD=refs/heads/master symref-refs/remotes/origin/HEAD=refs/remotes/origin/master ...

or something like that.