v2, 6 partsDetecting HEAD more reliably while cloning

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(-)

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(-)

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(-)

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(-)

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(-)

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(-)

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

Junio C Hamano wrote:

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

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

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 O. Pearce" <spearce@spearce.org> writes:

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.

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

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

Jeff King <peff@peff.net> writes:

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.

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

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

Jeff King <peff@peff.net> writes:

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.

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.

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

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

Junio C Hamano schrieb:

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

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

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.