git clone (ssh://) skips detached HEAD

git clone (ssh://) skips detached HEAD

[Dmitry Ivankov]

For some reason git doesn't try to fetch detached HEAD object if it's
not needed by needed heads.
It could be just work on top of detached HEAD or checked out remote branch.

Steps to reproduce:
git init test && cd test
touch 1 && git add 1 && git commit -m 123
touch 2 && git add 2 && git commit -m 345
git reset --hard HEAD^
git checkout HEAD@{1}

cd ../
git clone ssh://127.0.0.1/`pwd`/test test2

remote: Counting objects: 3, done.
remote: Total 3 (delta 0), reused 0 (delta 0)
Receiving objects: 100% (3/3), done.
error: Trying to write ref HEAD with nonexistant object
91dbc2403853783f637744c31036f94a66084286
fatal: Cannot update the ref 'HEAD'.

HEAD seems to be treated in a special way in various places, so
haven't found any easy patch to fix this.

And by the way, clone --no-hardlinks test test2 still goes to is_local
codepath and avoids the bug.

Re: git clone (ssh://) skips detached HEAD

[Jeff King]

On Thu, Jun 02, 2011 at 03:30:37AM +0600, Dmitry Ivankov wrote:

> For some reason git doesn't try to fetch detached HEAD object if it's
> not needed by needed heads.

The reason is that the default refspec on clone is:

  $ git config remote.origin.fetch
  +refs/heads/*:refs/remotes/origin/*

And HEAD is not under refs/heads/.

So you can do:

  git init test2 && cd test2
  git fetch ../test HEAD

if you want. So I don't think it's a bug that we don't fetch it, but...

> Steps to reproduce:
> git init test && cd test
> touch 1 && git add 1 && git commit -m 123
> touch 2 && git add 2 && git commit -m 345
> git reset --hard HEAD^
> git checkout HEAD@{1}
> 
> cd ../
> git clone ssh://127.0.0.1/`pwd`/test test2
> remote: Counting objects: 3, done.
> remote: Total 3 (delta 0), reused 0 (delta 0)
> Receiving objects: 100% (3/3), done.
> error: Trying to write ref HEAD with nonexistant object
> 91dbc2403853783f637744c31036f94a66084286
> fatal: Cannot update the ref 'HEAD'.

This is quite bad behavior. In addition to the ugly error messages, it
actually aborts the clone. So it is impossible to clone a repo with a
detached HEAD that is not otherwise referenced.

We basically have two choices:

  1. Fetch objects for HEAD on clone.

  2. Don't checkout a detached HEAD if we don't have the object (or
     possibly, don't checkout a detached HEAD at all; we already do
     something similar for the case of a HEAD that points to a bogus
     branch).

I think (2) is more consistent with the refspec we set up, but (1) is
probably more convenient to users (and better matches the case where the
remote is on a detached HEAD that _does_ point to something we have).

> HEAD seems to be treated in a special way in various places, so
> haven't found any easy patch to fix this.

I think it should just be a matter of adding HEAD to the list of things
we fetch, although we don't actually want to write it to a ref. I'll
take a look.

> And by the way, clone --no-hardlinks test test2 still goes to is_local
> codepath and avoids the bug.

Yeah. --no-hardlinks still avoids the git-transport; it just copies
instead of hardlinking. I saw you used ssh://localhost above to do your
test. You can also use file://$PWD/test to accomplish the same thing
without having to run an ssh daemon.

-Peff

Re: git clone (ssh://) skips detached HEAD

[Dmitry Ivankov]

>> git clone ssh://127.0.0.1/`pwd`/test test2
>> remote: Counting objects: 3, done.
>> remote: Total 3 (delta 0), reused 0 (delta 0)
>> Receiving objects: 100% (3/3), done.
>> error: Trying to write ref HEAD with nonexistant object
>> 91dbc2403853783f637744c31036f94a66084286
>> fatal: Cannot update the ref 'HEAD'.
>
> This is quite bad behavior. In addition to the ugly error messages, it
> actually aborts the clone. So it is impossible to clone a repo with a
> detached HEAD that is not otherwise referenced.
Which could be evil without resumable clone on a big repo.
git clone --bare will work though.

> We basically have two choices:
>
>  1. Fetch objects for HEAD on clone.
>
>  2. Don't checkout a detached HEAD if we don't have the object (or
>     possibly, don't checkout a detached HEAD at all; we already do
>     something similar for the case of a HEAD that points to a bogus
>     branch).
>
> I think (2) is more consistent with the refspec we set up, but (1) is
> probably more convenient to users (and better matches the case where the
> remote is on a detached HEAD that _does_ point to something we have).
I like (2) a bit more too, and give a hint about explicit HEAD fetching.

> I think it should just be a matter of adding HEAD to the list of things
> we fetch, although we don't actually want to write it to a ref. I'll
> take a look.
I tried to hack it through via adding HEAD:HEADtmp static const
refspec to remote.{h,c}
and hit "Ignoring funny ref 'HEAD' locally" in remote.c:get_fetch_map,
commented out that ignore and got:

error: unable to resolve reference HEAD: No such file or directory
fatal: Cannot lock the ref 'HEAD'.

from builtin/clone.c:
...
                        const char *head = skip_prefix(our_head_points_at->name,
                                                       "refs/heads/");

                        update_ref(reflog_msg.buf, "HEAD",
                                   our_head_points_at->old_sha1,
                                   NULL, 0, DIE_ON_ERR);
...
afaik from gdb HEAD ended up being a symlink to "" ref or so.

Re: git clone (ssh://) skips detached HEAD

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> We basically have two choices:
>
>   1. Fetch objects for HEAD on clone.
>
>   2. Don't checkout a detached HEAD if we don't have the object (or
>      possibly, don't checkout a detached HEAD at all; we already do
>      something similar for the case of a HEAD that points to a bogus
>      branch).
>
> I think (2) is more consistent with the refspec we set up, but (1) is
> probably more convenient to users (and better matches the case where the
> remote is on a detached HEAD that _does_ point to something we have).

Probably. As HEAD is usually visible via ls-remote exchange, the usual
security concern would not come into the picture even if we do (1), even
though it feels somewhat wrong to do.

Re: git clone (ssh://) skips detached HEAD

[Jakub Narebski]

Jeff King <peff@peff.net> writes:

> On Thu, Jun 02, 2011 at 03:30:37AM +0600, Dmitry Ivankov wrote:
> 
> > For some reason git doesn't try to fetch detached HEAD object if it's
> > not needed by needed heads.
> 
> The reason is that the default refspec on clone is:
> 
>   $ git config remote.origin.fetch
>   +refs/heads/*:refs/remotes/origin/*
> 
> And HEAD is not under refs/heads/.

Hmmm... HEAD is a bit of special case, as HEAD should really land in
refs/remotes/origin/HEAD from what I understand.

[...] 
> > Steps to reproduce:
> > git init test && cd test
> > touch 1 && git add 1 && git commit -m 123
> > touch 2 && git add 2 && git commit -m 345
> > git reset --hard HEAD^
> > git checkout HEAD@{1}
> > 
> > cd ../
> > git clone ssh://127.0.0.1/`pwd`/test test2
> > remote: Counting objects: 3, done.
> > remote: Total 3 (delta 0), reused 0 (delta 0)
> > Receiving objects: 100% (3/3), done.
> > error: Trying to write ref HEAD with nonexistant object
> > 91dbc2403853783f637744c31036f94a66084286
> > fatal: Cannot update the ref 'HEAD'.
> 
> This is quite bad behavior. In addition to the ugly error messages, it
> actually aborts the clone. So it is impossible to clone a repo with a
> detached HEAD that is not otherwise referenced.

It _might_ be the case that the fact that git protocol doesn't have
mechanism to transfer information about symref, and ends up guessing
where HEAD points to, bites use here.

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: git clone (ssh://) skips detached HEAD

[Jeff King]

On Wed, Jun 01, 2011 at 03:22:09PM -0700, Junio C Hamano wrote:

> > We basically have two choices:
> >
> >   1. Fetch objects for HEAD on clone.
> >
> >   2. Don't checkout a detached HEAD if we don't have the object (or
> >      possibly, don't checkout a detached HEAD at all; we already do
> >      something similar for the case of a HEAD that points to a bogus
> >      branch).
> >
> > I think (2) is more consistent with the refspec we set up, but (1) is
> > probably more convenient to users (and better matches the case where the
> > remote is on a detached HEAD that _does_ point to something we have).
> 
> Probably. As HEAD is usually visible via ls-remote exchange, the usual
> security concern would not come into the picture even if we do (1), even
> though it feels somewhat wrong to do.

Yeah, one can always just do:

  git fetch origin HEAD && git checkout FETCH_HEAD

immediately afterwards. But I think given that we make some effort to
propagate detached-ness across a clone in cases where we can, we should
just do the fetch.

I wrote some tests that document what I think _should_ happen. In
addition to this bug, there's one other (the second in my list below).

I'm done for the day, but may take a look at actually fixing these
tomorrow.

-- >8 --
Subject: [PATCH] t: add tests for cloning remotes with detached HEAD

We didn't test this setup at all, and doing so reveals a few
bugs:

  1. Cloning a repository with an orphaned detached HEAD
     (i.e., one that points to history that is not
     referenced by any ref) will fail.

  2. Cloning a repository with a detached HEAD that points
     to a tag will cause us to write a bogus "refs/tags/..."
     ref into the HEAD symbolic ref. We should probably
     detach instead.

  3. Cloning a repository with a detached HEAD that points
     to a branch will cause us to checkout that branch. This
     is a known limitation of the git protocol (we have to
     guess at HEAD's destination, since the symref contents
     aren't shown to us). This test serves to document the
     desired behavior, which can only be achieved once the
     git protocol learns to share symref information.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t5707-clone-detached.sh |   76 +++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 76 insertions(+), 0 deletions(-)
 create mode 100755 t/t5707-clone-detached.sh

diff --git a/t/t5707-clone-detached.sh b/t/t5707-clone-detached.sh
new file mode 100755
index 0000000..fca8609
--- /dev/null
+++ b/t/t5707-clone-detached.sh
@@ -0,0 +1,76 @@
+#!/bin/sh
+
+test_description='test cloning a repository with detached HEAD'
+. ./test-lib.sh
+
+head_is_detached() {
+	git --git-dir=$1/.git rev-parse --verify HEAD &&
+	test_must_fail git --git-dir=$1/.git symbolic-ref HEAD
+}
+
+test_expect_success 'setup' '
+	echo one >file &&
+	git add file &&
+	git commit -m one &&
+	echo two >file &&
+	git commit -a -m two &&
+	git tag two &&
+	echo three >file &&
+	git commit -a -m three
+'
+
+test_expect_success 'clone repo (detached HEAD points to branch)' '
+	git checkout --detach master &&
+	git clone "file://$PWD" detached-branch
+'
+test_expect_success 'cloned HEAD matches' '
+	echo three >expect &&
+	git --git-dir=detached-branch/.git log -1 --format=%s >actual &&
+	test_cmp expect actual
+'
+test_expect_failure 'cloned HEAD is detached' '
+	head_is_detached detached-branch
+'
+
+test_expect_success 'clone repo (detached HEAD points to tag)' '
+	git checkout --detach two &&
+	git clone "file://$PWD" detached-tag
+'
+test_expect_success 'cloned HEAD matches' '
+	echo two >expect &&
+	git --git-dir=detached-tag/.git log -1 --format=%s >actual &&
+	test_cmp expect actual
+'
+test_expect_failure 'cloned HEAD is detached' '
+	head_is_detached detached-tag
+'
+
+test_expect_success 'clone repo (detached HEAD points to history)' '
+	git checkout --detach two^ &&
+	git clone "file://$PWD" detached-history
+'
+test_expect_success 'cloned HEAD matches' '
+	echo one >expect &&
+	git --git-dir=detached-history/.git log -1 --format=%s >actual &&
+	test_cmp expect actual
+'
+test_expect_success 'cloned HEAD is detached' '
+	head_is_detached detached-history
+'
+
+test_expect_failure 'clone repo (orphan detached HEAD)' '
+	git checkout --detach master &&
+	echo four >file &&
+	git commit -a -m four &&
+	git clone "file://$PWD" detached-orphan
+'
+test_expect_failure 'cloned HEAD matches' '
+	echo four >expect &&
+	git --git-dir=detached-orphan/.git log -1 --format=%s >actual &&
+	test_cmp expect actual
+'
+test_expect_failure 'cloned HEAD is detached' '
+	head_is_detached detached-orphan
+'
+
+test_done
-- 
1.7.4.4.23.g5df3c

Re: git clone (ssh://) skips detached HEAD

[Jeff King]

On Wed, Jun 01, 2011 at 03:42:58PM -0700, Jakub Narebski wrote:

> > The reason is that the default refspec on clone is:
> > 
> >   $ git config remote.origin.fetch
> >   +refs/heads/*:refs/remotes/origin/*
> > 
> > And HEAD is not under refs/heads/.
> 
> Hmmm... HEAD is a bit of special case, as HEAD should really land in
> refs/remotes/origin/HEAD from what I understand.

No, that is always supposed to be a symbolic ref. Making it a real ref
would be confusing. I don't think fetch should look at HEAD at all; it's
outside its refspec. However, clone does treat HEAD specially, and
should probably convert the remote's detached HEAD into a local detached
HEAD (we already do if it's part of referenced history).

> > > git clone ssh://127.0.0.1/`pwd`/test test2
> > > remote: Counting objects: 3, done.
> > > remote: Total 3 (delta 0), reused 0 (delta 0)
> > > Receiving objects: 100% (3/3), done.
> > > error: Trying to write ref HEAD with nonexistant object
> > > 91dbc2403853783f637744c31036f94a66084286
> > > fatal: Cannot update the ref 'HEAD'.
> > 
> > This is quite bad behavior. In addition to the ugly error messages, it
> > actually aborts the clone. So it is impossible to clone a repo with a
> > detached HEAD that is not otherwise referenced.
> 
> It _might_ be the case that the fact that git protocol doesn't have
> mechanism to transfer information about symref, and ends up guessing
> where HEAD points to, bites use here.

No, we already handle that case. If there is a valid HEAD and no ref
matches, then we assume it's a detached HEAD. Which makes sense, and the
guess is right in this case. The problem is that we didn't fetch the
objects for HEAD.

-Peff

Re: git clone (ssh://) skips detached HEAD

[Jeff King]

On Wed, Jun 01, 2011 at 06:47:54PM -0400, Jeff King wrote:

> > Probably. As HEAD is usually visible via ls-remote exchange, the usual
> > security concern would not come into the picture even if we do (1), even
> > though it feels somewhat wrong to do.
> 
> Yeah, one can always just do:
> 
>   git fetch origin HEAD && git checkout FETCH_HEAD
> 
> immediately afterwards. But I think given that we make some effort to
> propagate detached-ness across a clone in cases where we can, we should
> just do the fetch.

Re-reading what I wrote, it's horribly unclear. What I meant was:

  We already make some effort to propagate detached-ness across a clone
  in cases where the detached commit is found in referenced history. To
  not do so for the case where the detached commit is not an ancestor of
  any ref seems unnecessarily inconsistent. Therefore we should solve
  the problem not by refusing to handle the detached HEAD, but by
  correctly fetching the objects.

-Peff

Re: git clone (ssh://) skips detached HEAD

[Jakub Narebski]

On Thu, 2 Jun 2011, Jeff King wrote:
> On Wed, Jun 01, 2011 at 03:42:58PM -0700, Jakub Narebski wrote:
> 
> > > The reason is that the default refspec on clone is:
> > > 
> > >   $ git config remote.origin.fetch
> > >   +refs/heads/*:refs/remotes/origin/*
> > > 
> > > And HEAD is not under refs/heads/.
> > 
> > Hmmm... HEAD is a bit of special case, as HEAD should really land in
> > refs/remotes/origin/HEAD from what I understand.
> 
> No, that is always supposed to be a symbolic ref. Making it a real ref
> would be confusing. I don't think fetch should look at HEAD at all; it's
> outside its refspec. However, clone does treat HEAD specially, and
> should probably convert the remote's detached HEAD into a local detached
> HEAD (we already do if it's part of referenced history).

Hmmm... in ordinary case (not on detached HEAD) "git fetch" would never
modify my local branches nor my local HEAD.  On the other hand IIRC
origin/HEAD do not follow switching branches at origin, and is staying
at the value at clone (or "git remote set-head"), isn't it?  

So what to do when HEAD on origin is detached?  Modify .git/HEAD,
or maybe .git/refs/remotes/origin/HEAD?

-- 
Jakub Narebski
Poland

Re: git clone (ssh://) skips detached HEAD

[Jeff King]

On Thu, Jun 02, 2011 at 10:02:31PM +0200, Jakub Narebski wrote:

> > No, that is always supposed to be a symbolic ref. Making it a real ref
> > would be confusing. I don't think fetch should look at HEAD at all; it's
> > outside its refspec. However, clone does treat HEAD specially, and
> > should probably convert the remote's detached HEAD into a local detached
> > HEAD (we already do if it's part of referenced history).
> 
> Hmmm... in ordinary case (not on detached HEAD) "git fetch" would never
> modify my local branches nor my local HEAD.

Right. Fetch should never touch those things. Nor should it ever touch
refs/remotes/$remote/HEAD, as that is not really "where is the remote
HEAD pointing" but rather "what would I like the alias $remote to point
to"? Clone sets it to the remote HEAD, as that is the most obvious
choice; but if the user changes it, we should leave their choice in
place.

> On the other hand IIRC origin/HEAD do not follow switching branches at
> origin, and is staying at the value at clone (or "git remote
> set-head"), isn't it?

Right.

> So what to do when HEAD on origin is detached?  Modify .git/HEAD, or
> maybe .git/refs/remotes/origin/HEAD?

I don't think fetch should do _anything_ with the remote HEAD. It's
outside of its refspec scope. This is purely about what clone should do,
and that's an easier problem, because it's not "modify" but rather
"create".

-Peff

Re: git clone (ssh://) skips detached HEAD

[Jeff King]

On Wed, Jun 01, 2011 at 06:47:54PM -0400, Jeff King wrote:

> I wrote some tests that document what I think _should_ happen. In
> addition to this bug, there's one other (the second in my list below).
> 
> I'm done for the day, but may take a look at actually fixing these
> tomorrow.
> 
> -- >8 --
> Subject: [PATCH] t: add tests for cloning remotes with detached HEAD

So this turned out to be a tiny amount of code, which is nice. And all
of the "clone should fetch HEAD" bits stay inside builtin/clone.c, which
gives me warm fuzzies that my change will not accidentally break regular
fetches.

Unfortunately, there's a test failure in t5800; apparently our
git-remote-testgit helper does not like us fetching HEAD. I haven't
figured out if it's a bug in the helper, or if this will be a problem in
general with remote helpers. More details in 3/3.

  [1/3]: t: add tests for cloning remotes with detached HEAD
  [2/3]: consider only branches in guess_remote_head
  [3/3]: clone: always fetch remote HEAD

-Peff

[PATCH 1/3] t: add tests for cloning remotes with detached HEAD

[Jeff King]

We didn't test this setup at all, and doing so reveals a few
bugs:

  1. Cloning a repository with an orphaned detached HEAD
     (i.e., one that points to history that is not
     referenced by any ref) will fail.

  2. Cloning a repository with a detached HEAD that points
     to a tag will cause us to write a bogus "refs/tags/..."
     ref into the HEAD symbolic ref. We should probably
     detach instead.

  3. Cloning a repository with a detached HEAD that points
     to a branch will cause us to checkout that branch. This
     is a known limitation of the git protocol (we have to
     guess at HEAD's destination, since the symref contents
     aren't shown to us). This test serves to document the
     desired behavior, which can only be achieved once the
     git protocol learns to share symref information.

Signed-off-by: Jeff King <peff@peff.net>
---
Just a repost of my earlier patch.

 t/t5707-clone-detached.sh |   76 +++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 76 insertions(+), 0 deletions(-)
 create mode 100755 t/t5707-clone-detached.sh

diff --git a/t/t5707-clone-detached.sh b/t/t5707-clone-detached.sh
new file mode 100755
index 0000000..fca8609
--- /dev/null
+++ b/t/t5707-clone-detached.sh
@@ -0,0 +1,76 @@
+#!/bin/sh
+
+test_description='test cloning a repository with detached HEAD'
+. ./test-lib.sh
+
+head_is_detached() {
+	git --git-dir=$1/.git rev-parse --verify HEAD &&
+	test_must_fail git --git-dir=$1/.git symbolic-ref HEAD
+}
+
+test_expect_success 'setup' '
+	echo one >file &&
+	git add file &&
+	git commit -m one &&
+	echo two >file &&
+	git commit -a -m two &&
+	git tag two &&
+	echo three >file &&
+	git commit -a -m three
+'
+
+test_expect_success 'clone repo (detached HEAD points to branch)' '
+	git checkout --detach master &&
+	git clone "file://$PWD" detached-branch
+'
+test_expect_success 'cloned HEAD matches' '
+	echo three >expect &&
+	git --git-dir=detached-branch/.git log -1 --format=%s >actual &&
+	test_cmp expect actual
+'
+test_expect_failure 'cloned HEAD is detached' '
+	head_is_detached detached-branch
+'
+
+test_expect_success 'clone repo (detached HEAD points to tag)' '
+	git checkout --detach two &&
+	git clone "file://$PWD" detached-tag
+'
+test_expect_success 'cloned HEAD matches' '
+	echo two >expect &&
+	git --git-dir=detached-tag/.git log -1 --format=%s >actual &&
+	test_cmp expect actual
+'
+test_expect_failure 'cloned HEAD is detached' '
+	head_is_detached detached-tag
+'
+
+test_expect_success 'clone repo (detached HEAD points to history)' '
+	git checkout --detach two^ &&
+	git clone "file://$PWD" detached-history
+'
+test_expect_success 'cloned HEAD matches' '
+	echo one >expect &&
+	git --git-dir=detached-history/.git log -1 --format=%s >actual &&
+	test_cmp expect actual
+'
+test_expect_success 'cloned HEAD is detached' '
+	head_is_detached detached-history
+'
+
+test_expect_failure 'clone repo (orphan detached HEAD)' '
+	git checkout --detach master &&
+	echo four >file &&
+	git commit -a -m four &&
+	git clone "file://$PWD" detached-orphan
+'
+test_expect_failure 'cloned HEAD matches' '
+	echo four >expect &&
+	git --git-dir=detached-orphan/.git log -1 --format=%s >actual &&
+	test_cmp expect actual
+'
+test_expect_failure 'cloned HEAD is detached' '
+	head_is_detached detached-orphan
+'
+
+test_done
-- 
1.7.6.rc0.36.g5a0d

[PATCH 2/3] consider only branches in guess_remote_head

[Jeff King]

The guess_remote_head function tries to figure out where a
remote's HEAD is pointing by comparing the sha1 of the
remote's HEAD with the sha1 of various refs found on the
remote. However, we were too liberal in matching refs, and
would match tags or remote tracking branches, even though
these things could not possibly be referenced by the HEAD
symbolic ref (since git will detach when checking them out).

As a result, a clone of a remote repository with a detached
HEAD might write "refs/tags/*" into our local HEAD, which is
bogus. The resulting HEAD should be detached.

The other related code path is remote.c's get_head_names()
(which is used for, among other things, "set-head -a"). This was
not affected, however, as that function feeds only refs from
refs/heads to guess_remote_head.

Signed-off-by: Jeff King <peff@peff.net>
---
 remote.c                  |    4 +++-
 t/t5707-clone-detached.sh |    2 +-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/remote.c b/remote.c
index ca42a12..f073b1e 100644
--- a/remote.c
+++ b/remote.c
@@ -1667,7 +1667,9 @@ struct ref *guess_remote_head(const struct ref *head,
 
 	/* Look for another ref that points there */
 	for (r = refs; r; r = r->next) {
-		if (r != head && !hashcmp(r->old_sha1, head->old_sha1)) {
+		if (r != head &&
+		    !prefixcmp(r->name, "refs/heads/") &&
+		    !hashcmp(r->old_sha1, head->old_sha1)) {
 			*tail = copy_ref(r);
 			tail = &((*tail)->next);
 			if (!all)
diff --git a/t/t5707-clone-detached.sh b/t/t5707-clone-detached.sh
index fca8609..db4af95 100755
--- a/t/t5707-clone-detached.sh
+++ b/t/t5707-clone-detached.sh
@@ -41,7 +41,7 @@ test_expect_success 'cloned HEAD matches' '
 	git --git-dir=detached-tag/.git log -1 --format=%s >actual &&
 	test_cmp expect actual
 '
-test_expect_failure 'cloned HEAD is detached' '
+test_expect_success 'cloned HEAD is detached' '
 	head_is_detached detached-tag
 '
 
-- 
1.7.6.rc0.36.g5a0d

[PATCH 3/3] clone: always fetch remote HEAD

[Jeff King]

In most cases, fetching the remote HEAD explicitly is
unnecessary. It's just a symref pointing to a branch which
we are already fetching, so we will already ask for its sha1.

However, if the remote has a detached HEAD, things are less
certain. We do not ask for HEAD's sha1, but we do try to
write it into a local detached HEAD. In most cases this is
fine, as the remote HEAD is pointing to some part of the
history graph that we will fetch via the refs.

But if the remote HEAD points to an "orphan" commit (one
which was is not an ancestor of any refs), then we will not
have the object, and update_ref will complain when we try to
write the detached HEAD, aborting the whole clone.

This patch makes clone always explicitly ask the remote for
the sha1 of its HEAD commit. In the non-detached case, this
is a no-op, as we were going to ask for that sha1 anyway. In
the regular detached case, this will add an extra "want" to
the protocol negotiation, but will not change the history
that gets sent. And in the detached orphan case, we will
fetch the orphaned history so that we can write it into our
local detached HEAD.

Signed-off-by: Jeff King <peff@peff.net>
---
[+cc Sverre and Jonathan for remote-helper advice]

This fails t5800, with:

  expecting success:
        git clone "testgit::${PWD}/server" localclone &&
        test_cmp public/file localclone/file

  Cloning into localclone...
  fatal: Got feature command 'relative-marks' after data command
  fast-import: dumping crash report to ...

So I guess it doesn't like us asking for HEAD. But the fact that it
sends weird data to fast-import instead of saying "hey, HEAD doesn't
exist" has me confused. I'm not sure if this is something one should not
be doing with remote helpers, or if the testgit helper is simply buggy
or incomplete.

 builtin/clone.c           |   10 +++++++---
 t/t5707-clone-detached.sh |    6 +++---
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index f579794..a99f1c8 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -343,8 +343,9 @@ static void remove_junk_on_signal(int signo)
 static struct ref *wanted_peer_refs(const struct ref *refs,
 		struct refspec *refspec)
 {
-	struct ref *local_refs = NULL;
-	struct ref **tail = &local_refs;
+	struct ref *head = get_remote_ref(refs, "HEAD");
+	struct ref *local_refs = head;
+	struct ref **tail = head ? &head->next : &local_refs;
 
 	get_fetch_map(refs, refspec, &tail, 0);
 	if (!option_mirror)
@@ -357,8 +358,11 @@ static void write_remote_refs(const struct ref *local_refs)
 {
 	const struct ref *r;
 
-	for (r = local_refs; r; r = r->next)
+	for (r = local_refs; r; r = r->next) {
+		if (!r->peer_ref)
+			continue;
 		add_extra_ref(r->peer_ref->name, r->old_sha1, 0);
+	}
 
 	pack_refs(PACK_REFS_ALL);
 	clear_extra_refs();
diff --git a/t/t5707-clone-detached.sh b/t/t5707-clone-detached.sh
index db4af95..82ecbce 100755
--- a/t/t5707-clone-detached.sh
+++ b/t/t5707-clone-detached.sh
@@ -58,18 +58,18 @@ test_expect_success 'cloned HEAD is detached' '
 	head_is_detached detached-history
 '
 
-test_expect_failure 'clone repo (orphan detached HEAD)' '
+test_expect_success 'clone repo (orphan detached HEAD)' '
 	git checkout --detach master &&
 	echo four >file &&
 	git commit -a -m four &&
 	git clone "file://$PWD" detached-orphan
 '
-test_expect_failure 'cloned HEAD matches' '
+test_expect_success 'cloned HEAD matches' '
 	echo four >expect &&
 	git --git-dir=detached-orphan/.git log -1 --format=%s >actual &&
 	test_cmp expect actual
 '
-test_expect_failure 'cloned HEAD is detached' '
+test_expect_success 'cloned HEAD is detached' '
 	head_is_detached detached-orphan
 '
 
-- 
1.7.6.rc0.36.g5a0d

Re: [PATCH 3/3] clone: always fetch remote HEAD

[Sverre Rabbelier]

Heya,

On Fri, Jun 3, 2011 at 00:18, Jeff King <peff@peff.net> wrote:
> So I guess it doesn't like us asking for HEAD. But the fact that it
> sends weird data to fast-import instead of saying "hey, HEAD doesn't
> exist" has me confused. I'm not sure if this is something one should not
> be doing with remote helpers, or if the testgit helper is simply buggy
> or incomplete.

Definitely the latter, quite possibly the former. I don't know if
asking for "HEAD" makes much sense in a remote-helper context though.
In Mercurial it does (e.g., tip), and in svn sort of, but I don't know
about other vcs-es. Perhaps it should be guarded by a capability?

-- 
Cheers,

Re: [PATCH 3/3] clone: always fetch remote HEAD

[Jeff King]

On Fri, Jun 03, 2011 at 12:36:50AM -0500, Sverre Rabbelier wrote:

> On Fri, Jun 3, 2011 at 00:18, Jeff King <peff@peff.net> wrote:
> > So I guess it doesn't like us asking for HEAD. But the fact that it
> > sends weird data to fast-import instead of saying "hey, HEAD doesn't
> > exist" has me confused. I'm not sure if this is something one should not
> > be doing with remote helpers, or if the testgit helper is simply buggy
> > or incomplete.
> 
> Definitely the latter, quite possibly the former. I don't know if
> asking for "HEAD" makes much sense in a remote-helper context though.
> In Mercurial it does (e.g., tip), and in svn sort of, but I don't know
> about other vcs-es. Perhaps it should be guarded by a capability?

Yeah, I think it depends on the helper. Definitely smart-http should be
able to do it. But protecting it with a capability does make sense, and
then remote helpers can opt into it.

-Peff

Re: [PATCH 3/3] clone: always fetch remote HEAD

[Jeff King]

On Fri, Jun 03, 2011 at 01:43:03AM -0400, Jeff King wrote:

> On Fri, Jun 03, 2011 at 12:36:50AM -0500, Sverre Rabbelier wrote:
> 
> > On Fri, Jun 3, 2011 at 00:18, Jeff King <peff@peff.net> wrote:
> > > So I guess it doesn't like us asking for HEAD. But the fact that it
> > > sends weird data to fast-import instead of saying "hey, HEAD doesn't
> > > exist" has me confused. I'm not sure if this is something one should not
> > > be doing with remote helpers, or if the testgit helper is simply buggy
> > > or incomplete.
> > 
> > Definitely the latter, quite possibly the former. I don't know if
> > asking for "HEAD" makes much sense in a remote-helper context though.
> > In Mercurial it does (e.g., tip), and in svn sort of, but I don't know
> > about other vcs-es. Perhaps it should be guarded by a capability?
> 
> Yeah, I think it depends on the helper. Definitely smart-http should be
> able to do it. But protecting it with a capability does make sense, and
> then remote helpers can opt into it.

Actually, I forgot that I did consider this when writing the original
patch. The clone code will only ask for HEAD if the remote side
advertises it. So there is no capability required; either the helper
advertises a HEAD ref or it doesn't. If it does, it should be prepared
for us to ask for its sha1.

-Peff

Re: git clone (ssh://) skips detached HEAD

[Junio C Hamano]

Thanks. Looks sane in a somewhat weird way; the series is internally
consistent, and that is why I said "sane".

An alternative would be not to checkout anything when HEAD points at an
object that does not exist, or point the HEAD at the default "master"
branch just like in the case when we cannot guess uniquely. That way, we
do not have to worry about having to fetch the orphaned detached HEAD,
which is an unlikely thing the publisher wanted to feed to its recipients
in the first place. I tend to prefer the former (i.e. resulting in no
paths in the working tree, possibly with a big warning message "the
repository does not suggest which branch to track---are you sure you
wanted to clone from there?").

Seriously.

We treat the symbolic-ref on the publishing end not as the "current"
branch at all. It is used as the "suggested primary branch to track".  So
allowing to fetch from a repository with detached HEAD is already a weird
setup. I am hoping that we are not setting up origin/HEAD to point at
anything in this case, as the remote is telling us that there is no
suggested primary branch for the clients to track by having a detached
HEAD to begin with.

Even if you are fetching your own (or your pal's) repository with a
working tree to transfer a work-in-progress, any work on detached HEAD
that is orphaned is too transitory, and it goes against my taste to let
people fetch from it.

But people are free to have bad taste ;-).

Re: [PATCH 3/3] clone: always fetch remote HEAD

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Actually, I forgot that I did consider this when writing the original
> patch. The clone code will only ask for HEAD if the remote side
> advertises it. So there is no capability required; either the helper
> advertises a HEAD ref or it doesn't. If it does, it should be prepared
> for us to ask for its sha1.

Sounds very sensible ;-).

Re: [PATCH 3/3] clone: always fetch remote HEAD

[Jeff King]

On Fri, Jun 03, 2011 at 12:36:50AM -0500, Sverre Rabbelier wrote:

> On Fri, Jun 3, 2011 at 00:18, Jeff King <peff@peff.net> wrote:
> > So I guess it doesn't like us asking for HEAD. But the fact that it
> > sends weird data to fast-import instead of saying "hey, HEAD doesn't
> > exist" has me confused. I'm not sure if this is something one should not
> > be doing with remote helpers, or if the testgit helper is simply buggy
> > or incomplete.
> 
> Definitely the latter, quite possibly the former. I don't know if
> asking for "HEAD" makes much sense in a remote-helper context though.
> In Mercurial it does (e.g., tip), and in svn sort of, but I don't know
> about other vcs-es. Perhaps it should be guarded by a capability?

I did some more digging. I don't think the problem is with HEAD at all,
but rather with asking for more than one ref at all.

If we want to fetch multiple refs, and the remote helper has the import
capability, we end up in transport-helper.c:fetch_with_import. This will
generate an "import %s" line for every ref we are interested in.

Meanwhile, the testgit remote helper will read each import line and call
do_import. This ends up in the git_remote_helper library's
exporter.export_repo function, which dumps fast-import data to stdout,
including some "feature" lines.

On the second import line, we have already sent some fast-import data,
but now we send more "feature" lines, which causes fast-import to
complain.

So which of the three (git's transport-helper code, the testgit remote
helper, or the remote helper library) is at fault?

Is it git itself? The fetch_with_import has to be able to ask for
multiple refs, right? Or should it simply say "import" once, and then
pick the refs it wants out of the alternate namespace?

Or is it the fault of testgit for calling exporter.export_repo twice? It
seems to ignore the ref argument it gets to import, and just dumps the
whole repo, albeit into an alternate namespace. Should we then be doing
something per-ref with the result? Or since our particular import
imports everything, should we simply ignore further imports after the
first one?

Or is it the fault of the exporter code, which should not assume it is
the first thing to generate fast-import data on stdout? I don't think
this is the case, since there would be no point to exporting again,
right? We didn't actually restrict the export to a particular ref in the
first place.

So I'm confused about who is supposed to be doing what with remote
helpers and "import". And I wasn't able to find any helpful
documentation or other examples. From reading the transport-helper code,
my guess is:

  1. We send the helper a bunch of import lines, one per ref. The helper
     can use that ref to generate fast-import data for just that ref. Or
     it can ignore it and just import everything (like testgit does).
     The result goes into an alternate namespace, like refs/testgit/.

  2. Git reads the sha1 values from the alternate namespace into its
     list of to-fetch refs. This puts us in the same state we would be
     in for other transports (which usually fill in the sha1 earlier,
     but in this case, we don't know it until the import is done).

  3. Git does the usual write-out of remote sha1 values into their
     matching local counterparts.

Does that make sense? If so, then I think the right fix is for testgit
ti ignore all imports after the first one (since the first one will have
done all available refs). And the patch is:

diff --git a/git-remote-testgit.py b/git-remote-testgit.py
index df9d512..7330753 100644
--- a/git-remote-testgit.py
+++ b/git-remote-testgit.py
@@ -111,13 +111,19 @@ def update_local_repo(repo):
     return repo
 
 
+did_import = False
 def do_import(repo, args):
     """Exports a fast-import stream from testgit for git to import.
     """
+    global did_import
 
     if len(args) != 1:
         die("Import needs exactly one ref")
 
+    if did_import:
+	return
+    did_import = True
+
     if not repo.gitdir:
         die("Need gitdir to import")
 

-Peff

Re: git clone (ssh://) skips detached HEAD

[Jeff King]

On Fri, Jun 03, 2011 at 09:11:19AM -0700, Junio C Hamano wrote:

> An alternative would be not to checkout anything when HEAD points at an
> object that does not exist, or point the HEAD at the default "master"
> branch just like in the case when we cannot guess uniquely. That way, we
> do not have to worry about having to fetch the orphaned detached HEAD,
> which is an unlikely thing the publisher wanted to feed to its recipients
> in the first place. I tend to prefer the former (i.e. resulting in no
> paths in the working tree, possibly with a big warning message "the
> repository does not suggest which branch to track---are you sure you
> wanted to clone from there?").

Yeah, I was tempted to go the "check out nothing if we don't have the
object" route. But my thinking was:

  1. We have already been creating local detached HEADs to match a
     remote detached HEAD since at least 8434c2f (Build in clone,
     2008-04-27). Should we keep doing that? If so, I think it
     introduces a somewhat confusing inconsistency from the user's
     perspective. Why is a sight-seeing detached HEAD pointing into
     history OK to checkout, but one with a commit on top is not OK?

  2. Similar to the above, we already do have the object for a local
     clone, which just copies the object db whole. So now there is an
     inconsistency that:

       git clone foo bar

     will checkout out such a HEAD, but neither of:

       git clone file://$PWD/foo bar

       git clone git://host/foo bar

     does.

  3. Fetching and checking it out just seems like the most friendly and
     the least surprising thing for the user. In 99% of cases, people
     are cloning bare repositories whose HEADs likely won't detach
     anyway (and if they did, they certainly wouldn't have made commits
     on them). But in the rare case that I _do_ clone a non-bare repo,
     what am I trying to accomplish?

     Most likely I'm trying to make a new workspace, either to work on
     the identical branch, or some other branch. In the former case,
     making a detached HEAD (whether it points to history or to a new
     commit) is what I would want. In the latter case, it doesn't really
     matter what we put in HEAD, as the user is going to switch it
     anyway. The only downside is that we may have transferred some
     extra objects; in practice, this is probably not a big deal due to
     deltas unless your detached commit is gigantic.

  4. We're guessing at whether the user will want the objects on the
     detached HEAD or not. Which means we're going to be wrong
     sometimes. But I would rather err on the side of copying the extra
     commits than not. The few extra bytes spent are a better downside
     than:

       $ git clone git://host/foo bar
       $ ssh host && rm -rf foo
       [oops, I actually wanted the detached commit!]

     Though to be fair, if we printed a warning about the detached HEAD
     during the first command, the user would hopefully not execute the
     second one.

> We treat the symbolic-ref on the publishing end not as the "current"
> branch at all. It is used as the "suggested primary branch to track".
> So allowing to fetch from a repository with detached HEAD is already a
> weird setup.

By that argument, we should stop checking out any detached HEAD at all.
Which I agree makes sense from the point of view that clone is just
"init + remote add + fetch + checkout". But I think it's probably more
helpful to the user (and doesn't cost us much) to just checkout the
detached HEAD than to refuse to check out anything and print a warning.
If we're right, they're happy. If we're wrong, the solution in both
cases is to "git checkout" what they did want.

Possibly we should warn that the cloned HEAD is detached (maybe even
with the regular detached HEAD warning).

> I am hoping that we are not setting up origin/HEAD to point at
> anything in this case, as the remote is telling us that there is no
> suggested primary branch for the clients to track by having a detached
> HEAD to begin with.

No, we don't set up an origin/HEAD at all in that case; the handling for
that and our local HEAD are separate (and I was careful to maintain this
with my patch by not setting a peer_ref to store the HEAD we fetch; it
stays in core until we write it to our HEAD).

> Even if you are fetching your own (or your pal's) repository with a
> working tree to transfer a work-in-progress, any work on detached HEAD
> that is orphaned is too transitory, and it goes against my taste to
> let people fetch from it.
> 
> But people are free to have bad taste ;-).

They can already fetch from it via "git fetch /your/pal HEAD". So this
is really just about clone. I think it is not even bad taste if your
workflow is:

  1. You're in the middle of a rebase on a detached HEAD. You make an
     amended commit, then continue. You get a conflict which is
     confusing, and inspection causes you to blame your coworker.

  2. You holler over the cubicle wall to your coworker, who runs "git
     clone ~bob/project bobs-project". They inspect your current state
     and try to cherry-pick the failed commit themselves. Either the
     merge conflicts tell them what to tell you to fix your problem, or
     perhaps they even resolve the conflicts themselves and let you
     fetch the state back.

So I think it is not a matter of taste, but of "in some (rare) cases
this is useful, and in many cases it is useless". I just think there is
no reason not to be helpful to the people in the rare cases, as it costs
so little in the other case (and remember that _without_ detached orphan
commits, this is literally a no-op).

-Peff

Re: [PATCH 3/3] clone: always fetch remote HEAD

[Sverre Rabbelier]

Heya,

On Fri, Jun 3, 2011 at 13:10, Jeff King <peff@peff.net> wrote:
> Or is it the fault of testgit for calling exporter.export_repo twice? It
> seems to ignore the ref argument it gets to import, and just dumps the
> whole repo, albeit into an alternate namespace. Should we then be doing
> something per-ref with the result? Or since our particular import
> imports everything, should we simply ignore further imports after the
> first one?

I never tested testgit with multiple import arguments, as I wasn't
sure how it was supposed to work in the first place.

> Does that make sense? If so, then I think the right fix is for testgit
> ti ignore all imports after the first one (since the first one will have
> done all available refs). And the patch is:

We can do that yes, although it would be even better if we could make
testgit implement importing only a limited set of branches, as I
assume that will be common. Nonetheless, this will do for now I think.

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 3/3] clone: always fetch remote HEAD

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I did some more digging. I don't think the problem is with HEAD at all,
> but rather with asking for more than one ref at all.
> ...
> Does that make sense? If so, then I think the right fix is for testgit
> ti ignore all imports after the first one (since the first one will have
> done all available refs). And the patch is:

After reading Sverre's answer, I think this makes sense as a short-term
workaround.  Shall we squah it in to 3/3?

Re: [PATCH 3/3] clone: always fetch remote HEAD

[Junio C Hamano]

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

> Jeff King <peff@peff.net> writes:
>
>> I did some more digging. I don't think the problem is with HEAD at all,
>> but rather with asking for more than one ref at all.
>> ...
>> Does that make sense? If so, then I think the right fix is for testgit
>> ti ignore all imports after the first one (since the first one will have
>> done all available refs). And the patch is:
>
> After reading Sverre's answer, I think this makes sense as a short-term
> workaround.  Shall we squah it in to 3/3?

I'll queue it separately as a tentative commit (below) and leave it in
'pu' for now. Please give it a better description so that we can move the
fix forward.

commit 2dbce8e7849279d9ef7d211390efd0da71e230ab
Author: Jeff King <peff@peff.net>
Date:   Fri Jun 3 14:10:52 2011 -0400

    remote-testgit: short-term fix to allow t5800 pass
    
    The test helper issues implements "import" but does not honor
    the branch argument; instead it imports everything and ends up
    not working well when more than one "import" is given.
    
    As a short-term workaround, just ignore second and later "import"
    commands.
    
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/git-remote-testgit.py b/git-remote-testgit.py
index df9d512..de644d2 100644
--- a/git-remote-testgit.py
+++ b/git-remote-testgit.py
@@ -111,13 +111,19 @@ def update_local_repo(repo):
     return repo
 
 
+did_import = False
 def do_import(repo, args):
     """Exports a fast-import stream from testgit for git to import.
     """
+    global did_import
 
     if len(args) != 1:
         die("Import needs exactly one ref")
 
+    if did_import:
+        return
+    did_import = True
+
     if not repo.gitdir:
         die("Need gitdir to import")
 

Re: [PATCH 3/3] clone: always fetch remote HEAD

[Sverre Rabbelier]

Heya,

On Mon, Jun 6, 2011 at 03:00, Junio C Hamano <gitster@pobox.com> wrote:
> I'll queue it separately as a tentative commit (below) and leave it in
> 'pu' for now. Please give it a better description so that we can move the
> fix forward.

Seems like an accurate description to me, what do you feel is missing?

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 3/3] clone: always fetch remote HEAD

[Junio C Hamano]

Sverre Rabbelier <srabbelier@gmail.com> writes:

> Heya,
>
> On Mon, Jun 6, 2011 at 03:00, Junio C Hamano <gitster@pobox.com> wrote:
>> I'll queue it separately as a tentative commit (below) and leave it in
>> 'pu' for now. Please give it a better description so that we can move the
>> fix forward.
>
> Seems like an accurate description to me, what do you feel is missing?

S-o-b to start with.

Re: [PATCH 3/3] clone: always fetch remote HEAD

[Jeff King]

On Fri, Jun 03, 2011 at 08:50:41PM -0500, Sverre Rabbelier wrote:

> > Does that make sense? If so, then I think the right fix is for testgit
> > ti ignore all imports after the first one (since the first one will have
> > done all available refs). And the patch is:
> 
> We can do that yes, although it would be even better if we could make
> testgit implement importing only a limited set of branches, as I
> assume that will be common. Nonetheless, this will do for now I think.

Yeah, I think that is a better model for showing people who are trying
to write their own helpers. We just have to read each "import" line,
saving the list of refs; when git disconnects from us, we know there are
no more, and we can start fast-import with the correct arguments.

-Peff

Re: [PATCH 3/3] clone: always fetch remote HEAD

[Jeff King]

On Sun, Jun 05, 2011 at 06:00:11PM -0700, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Jeff King <peff@peff.net> writes:
> >
> >> I did some more digging. I don't think the problem is with HEAD at all,
> >> but rather with asking for more than one ref at all.
> >> ...
> >> Does that make sense? If so, then I think the right fix is for testgit
> >> ti ignore all imports after the first one (since the first one will have
> >> done all available refs). And the patch is:
> >
> > After reading Sverre's answer, I think this makes sense as a short-term
> > workaround.  Shall we squah it in to 3/3?
> 
> I'll queue it separately as a tentative commit (below) and leave it in
> 'pu' for now. Please give it a better description so that we can move the
> fix forward.

I'll try the nicer fix Sverre mentioned and post a real patch. I wanted
to add a test for multiple refs to the test suite, but I got bogged down
by other breakages. Some of which I think are the fault of testgit, but
one of which I think may be a problem in the transport-helper code.

I gave up in frustration after many hours of dealing with it on Friday,
but I'll give it another go today. :)

-Peff

Re: [PATCH 3/3] clone: always fetch remote HEAD

[Sverre Rabbelier]

Heya,

On Mon, Jun 6, 2011 at 18:11, Jeff King <peff@peff.net> wrote:
> I'll try the nicer fix Sverre mentioned and post a real patch. I wanted
> to add a test for multiple refs to the test suite, but I got bogged down
> by other breakages. Some of which I think are the fault of testgit, but
> one of which I think may be a problem in the transport-helper code.

I'm not 100% the current remote-helper code allows that, since there
might be some interaction where the transport-helper code needs to
read from the helper before closing the connection. I don't think
that's the case, but I'm not sure of it either.

> I gave up in frustration after many hours of dealing with it on Friday,
> but I'll give it another go today. :)

Sounds familiar.

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 3/3] clone: always fetch remote HEAD

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> diff --git a/builtin/clone.c b/builtin/clone.c
> index f579794..a99f1c8 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -343,8 +343,9 @@ static void remove_junk_on_signal(int signo)
>  static struct ref *wanted_peer_refs(const struct ref *refs,
>  		struct refspec *refspec)
>  {
> -	struct ref *local_refs = NULL;
> -	struct ref **tail = &local_refs;
> +	struct ref *head = get_remote_ref(refs, "HEAD");

The rest of the patch looked quite sane but I wonder if this should be
using get_remote_ref() that calls find_ref_by_name_abbrev() which in turn
would hit "refs/heads/HEAD" if the remote side didn't give you "HEAD".
Shouldn't it be using find_ref_by_name() directly?

> @@ -357,8 +358,11 @@ static void write_remote_refs(const struct ref *local_refs)
>  {
>  	const struct ref *r;
>  
> -	for (r = local_refs; r; r = r->next)
> +	for (r = local_refs; r; r = r->next) {
> +		if (!r->peer_ref)
> +			continue;

As this is part of this patch, I presume this test reliably catch "HEAD"
and only "HEAD", but what is it that gives us this guarantee?  Is it that
in all three possible configurations (i.e. traditional no-separate remote
layout, separate remote layout, or mirrored layout), we never map anything
outside refs/heads/* and refs/tags/* namespace, hence things like HEAD
will never have peer_ref defined?

This is not a complaint but is an honest question. I am wondering how
future possible enhancements to "clone" (like the rumored "track only this
branch") will affect codepaths around this area.

Re: [PATCH 3/3] clone: always fetch remote HEAD

[Jeff King]

On Mon, Jun 06, 2011 at 01:31:54PM -0700, Junio C Hamano wrote:

> >  static struct ref *wanted_peer_refs(const struct ref *refs,
> >  		struct refspec *refspec)
> >  {
> > -	struct ref *local_refs = NULL;
> > -	struct ref **tail = &local_refs;
> > +	struct ref *head = get_remote_ref(refs, "HEAD");
> 
> The rest of the patch looked quite sane but I wonder if this should be
> using get_remote_ref() that calls find_ref_by_name_abbrev() which in turn
> would hit "refs/heads/HEAD" if the remote side didn't give you "HEAD".
> Shouldn't it be using find_ref_by_name() directly?

Ick, yeah, that was just me blindly cutting down what get_fetch_map was
doing to the bit that I wanted, and thinking get_remote_ref was it.  I
didn't even notice the fact that it was using the _abbrev form of
find_ref_by_name.

It should definitely be an exact match. I'll fix it in my re-roll.

> > @@ -357,8 +358,11 @@ static void write_remote_refs(const struct ref *local_refs)
> >  {
> >  	const struct ref *r;
> >  
> > -	for (r = local_refs; r; r = r->next)
> > +	for (r = local_refs; r; r = r->next) {
> > +		if (!r->peer_ref)
> > +			continue;
> 
> As this is part of this patch, I presume this test reliably catch "HEAD"
> and only "HEAD", but what is it that gives us this guarantee?  Is it that
> in all three possible configurations (i.e. traditional no-separate remote
> layout, separate remote layout, or mirrored layout), we never map anything
> outside refs/heads/* and refs/tags/* namespace, hence things like HEAD
> will never have peer_ref defined?

Until now, nobody fed us a ref without peer_ref mapped, as doing so
would segfault. So this gives a NULL peer ref the meaning "don't bother
saving this into a ref". Which is really the only sensible thing it can
mean, since the peer ref is the local bit that says where to put it.

> This is not a complaint but is an honest question. I am wondering how
> future possible enhancements to "clone" (like the rumored "track only this
> branch") will affect codepaths around this area.

I think it's a good question to ask. The answer is that if you are
cloning only a subset of the remote refs (or even a single ref), then
either:

  1. You will want to write that subset according to a refspec mapping
     (e.g., cloning "jk/*" to "remotes/origin/jk/*"). In that case you
     would have set the peer_ref to show where to put it.

  2. You will not want to write out these refs, because you are handling
     them separately. For example, in this case, we are already handling
     the detached HEAD case separately when we generally figure out what
     HEAD is. And in this case, the code does what you want by silently
     skipping.

     I tried to think of another case where you would want this, but I
     really couldn't come up with one. Even if you are cloning a "track
     only this branch" ref, you would still have its peer_ref pointing
     to its local counterpart (even if it's "refs/heads/master" both
     locally and on the remote).

-Peff

Re: [PATCH 3/3] clone: always fetch remote HEAD

[Jeff King]

On Mon, Jun 06, 2011 at 09:05:11PM +0200, Sverre Rabbelier wrote:

> On Mon, Jun 6, 2011 at 18:11, Jeff King <peff@peff.net> wrote:
> > I'll try the nicer fix Sverre mentioned and post a real patch. I wanted
> > to add a test for multiple refs to the test suite, but I got bogged down
> > by other breakages. Some of which I think are the fault of testgit, but
> > one of which I think may be a problem in the transport-helper code.
> 
> I'm not 100% the current remote-helper code allows that, since there
> might be some interaction where the transport-helper code needs to
> read from the helper before closing the connection. I don't think
> that's the case, but I'm not sure of it either.

No, I think it is how the import command was expected to run. The order
of the code in transport-helper.c is:

  1. Redirect the incoming pipe to fast-import.

  2. Print one or more "import $ref" lines.

  3. Print "\n", then close the outgoing pipe.

  4. Wait for fast-import to report completion.

So there really is no room for further communication.

-Peff

[PATCH 0/8] minor import/export remote helper fixes

[Jeff King]

On Mon, Jun 06, 2011 at 12:11:43PM -0400, Jeff King wrote:

> I'll try the nicer fix Sverre mentioned and post a real patch. I wanted
> to add a test for multiple refs to the test suite, but I got bogged down
> by other breakages. Some of which I think are the fault of testgit, but
> one of which I think may be a problem in the transport-helper code.
> 
> I gave up in frustration after many hours of dealing with it on Friday,
> but I'll give it another go today. :)

OK, I finally hit a point where I think all of my patches are doing sane
things. There are a few cleanups at the beginning, and then 4/8
demonstrates five different bugs. All but the last one are fixed by the
remaining patches (see the comments in 4/8 for why the fifth bug is
tricky).

  [1/8]: transport-helper: fix minor leak in push_refs_with_export
  [2/8]: git-remote-testgit: exit gracefully after push
  [3/8]: t5800: factor out some ref tests
  [4/8]: t5800: document some non-functional parts of remote helpers
  [5/8]: teach remote-testgit to import non-HEAD refs
  [6/8]: teach remote-testgit to import multiple refs
  [7/8]: transport-helper: don't feed bogus refs to export push
  [8/8]: git_remote_helpers: push all refs during a non-local export

-Peff

[PATCH 1/8] transport-helper: fix minor leak in push_refs_with_export

[Jeff King]

Signed-off-by: Jeff King <peff@peff.net>
---
 transport-helper.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index 660147f..b560b64 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -721,20 +721,21 @@ static int push_refs_with_export(struct transport *transport,
 		char *private;
 		unsigned char sha1[20];
 
 		if (!data->refspecs)
 			continue;
 		private = apply_refspecs(data->refspecs, data->refspec_nr, ref->name);
 		if (private && !get_sha1(private, sha1)) {
 			strbuf_addf(&buf, "^%s", private);
 			string_list_append(&revlist_args, strbuf_detach(&buf, NULL));
 		}
+		free(private);
 
 		string_list_append(&revlist_args, ref->name);
 
 	}
 
 	if (get_exporter(transport, &exporter,
 			 export_marks, import_marks, &revlist_args))
 		die("Couldn't run fast-export");
 
 	data->no_disconnect_req = 1;
-- 
1.7.6.rc0.35.gc40cb

[PATCH 2/8] git-remote-testgit: exit gracefully after push

[Jeff King]

The core of the helper script is a loop reading commands.
After most commands, we want to read more, until we get an
empty command that signals we're done. The "export" command
is different, though; all of the rest of the input goes to
fast-import, and there is nothing left for us to read.  This
caused us to print a warning that we got EOF, even though it
is expected in this case.

This was just a warning, so it didn't cause any tests to
fail, but the testgit helper is supposed to act as a
reference implementation. We should try to set a good
example.

This patch lets each command handler tell the main loop
whether it should expect more commands or not; the handler
exits gracefully if no more are expected.

Signed-off-by: Jeff King <peff@peff.net>
---
 git-remote-testgit.py |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/git-remote-testgit.py b/git-remote-testgit.py
index df9d512..937b858 100644
--- a/git-remote-testgit.py
+++ b/git-remote-testgit.py
@@ -74,6 +74,7 @@ def do_capabilities(repo, args):
     print "refspec refs/heads/*:%s*" % repo.prefix
 
     print # end capabilities
+    return True
 
 
 def do_list(repo, args):
@@ -96,6 +97,7 @@ def do_list(repo, args):
         print "@refs/heads/master HEAD"
 
     print # end list
+    return True
 
 
 def update_local_repo(repo):
@@ -123,6 +125,7 @@ def do_import(repo, args):
 
     repo = update_local_repo(repo)
     repo.exporter.export_repo(repo.gitdir)
+    return True
 
 
 def do_export(repo, args):
@@ -148,6 +151,7 @@ def do_export(repo, args):
     update_local_repo(repo)
     repo.importer.do_import(repo.gitdir)
     repo.non_local.push(repo.gitdir)
+    return False
 
 
 def do_gitdir(repo, args):
@@ -158,6 +162,7 @@ def do_gitdir(repo, args):
         die("gitdir needs an argument")
 
     repo.gitdir = ' '.join(args)
+    return True
 
 
 COMMANDS = {
@@ -203,7 +208,8 @@ def read_one_line(repo):
         die("Unknown command, %s", cmd)
 
     func = COMMANDS[cmd]
-    func(repo, cmdline)
+    if not func(repo, cmdline):
+	    return False
     sys.stdout.flush()
 
     return True
-- 
1.7.6.rc0.35.gc40cb

Re: [PATCH 3/3] clone: always fetch remote HEAD

[Sverre Rabbelier]

Heya,

On Tue, Jun 7, 2011 at 19:10, Jeff King <peff@peff.net> wrote:
> So there really is no room for further communication.

Thanks for checking. In that case it's just a matter of me not having
implemented this particular feature properly.

-- 
Cheers,

Sverre Rabbelier

[PATCH 3/8] t5800: factor out some ref tests

[Jeff King]

These are a little hard to read, and I'm about to add more
just like them. Plus the failure output is nicer if we use
test_cmp than a comparison with "test".

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t5800-remote-helpers.sh |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/t/t5800-remote-helpers.sh b/t/t5800-remote-helpers.sh
index 1fb6380..3a37ad0 100755
--- a/t/t5800-remote-helpers.sh
+++ b/t/t5800-remote-helpers.sh
@@ -17,6 +17,12 @@ then
 	test_set_prereq PYTHON_24
 fi
 
+compare_refs() {
+	git --git-dir="$1/.git" rev-parse --verify $2 >expect &&
+	git --git-dir="$3/.git" rev-parse --verify $4 >actual &&
+	test_cmp expect actual
+}
+
 test_expect_success PYTHON_24 'setup repository' '
 	git init --bare server/.git &&
 	git clone server public &&
@@ -59,8 +65,7 @@ test_expect_success PYTHON_24 'pushing to local repo' '
 	echo content >>file &&
 	git commit -a -m three &&
 	git push) &&
-	HEAD=$(git --git-dir=localclone/.git rev-parse --verify HEAD) &&
-	test $HEAD = $(git --git-dir=server/.git rev-parse --verify HEAD)
+	compare_refs localclone HEAD server HEAD
 '
 
 test_expect_success PYTHON_24 'synch with changes from localclone' '
@@ -73,8 +78,7 @@ test_expect_success PYTHON_24 'pushing remote local repo' '
 	echo content >>file &&
 	git commit -a -m four &&
 	git push) &&
-	HEAD=$(git --git-dir=clone/.git rev-parse --verify HEAD) &&
-	test $HEAD = $(git --git-dir=server/.git rev-parse --verify HEAD)
+	compare_refs clone HEAD server HEAD
 '
 
 test_done
-- 
1.7.6.rc0.35.gc40cb

[PATCH 4/8] t5800: document some non-functional parts of remote helpers

[Jeff King]

These are all things one might expect to work in a helper
that is capable of handling multiple branches (which our
testgit helper in theory should be able to do, as it is
backed by git). All of these bugs are specific to the
import/export codepaths, so they don't affect helpers like
git-remote-curl that use fetch/push commands.

The first and fourth tests are about fetching and pushing
new refs, and demonstrate bugs in the git_remote_helpers
library (so they would be most likely to impact helpers for
other VCSs which import/export git).

The second test is about importing multiple refs; it
demonstrates a bug in git-remote-testgit, which is mostly
for exercising the test code. Therefore it probably doesn't
affect anyone in practice.

The third test demonstrates a bug in git's side of the
helper code when the upstream has added new refs without us.
This could impact git users who use remote helpers to access
foreign VCSs.

All of those bugs have fixes later in this series.

The fifth test is the most complex, and does not have a fix
in this series. It tests pushing a ref via the export
mechanism to a new name on the remote side (i.e.,
"git push $remote old:new").

The problem is that we push all of the work of generating
the export stream onto fast-export, but we have no way of
communicating to fast-export that this name mapping is
happening. So we tell fast-export to generate a stream with
the commits for "old", but we can't tell it to label them
all as "new".

There are two possible solutions:

  1. Indicate the mapping to fast-export, so that it can
     generate the "mapped" names. Unfortunately, this is
     somewhat difficult due to the way fast-import is
     implemented. It feeds its revision parameters to the
     regular rev-walking machinery, asking the revision code
     to mark the "source" of each commit. So if fast-export
     sees that we want commits from "refs/heads/old", but
     also that we want to call commits from old as
     "refs/heads/new", it can't distinguish between the
     case of "I want _only_ new, using the commits of old"
     and "I want _both_ old and new, which happen to point
     to the same commits".

     And it's important to distinguish those cases, because
     in one, the remote will update master, and in the
     other, it will not.

     This isn't an insurmountable obstacle, but it's going
     to mean either fast-export has to take on more of the
     revision argument parsing and revwalking
     responsibility, or the revision walker's show_source
     code will have to learn about mapping names.

  2. Indicate the mapping to the remote helper, who can then
     apply the mapping to the results of the export. Right
     now the export remote helper command has no arguments;
     it simply says "hey, I'm going to dump a fast-export
     stream on you". And the remote-helper looks through the
     results of the stream to decide which refs are being
     pushed. This could be extended instead to something
     like:

       export refs/heads/foo
       export refs/heads/old:refs/heads/new
       <blank line>

     which would allow the remote helper to map names as
     appropriate. This a reasonably elegant solution; the
     downside is that it breaks compatibility with existing
     remote helpers.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t5800-remote-helpers.sh |   47 +++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 47 insertions(+), 0 deletions(-)

diff --git a/t/t5800-remote-helpers.sh b/t/t5800-remote-helpers.sh
index 3a37ad0..a10a48d 100755
--- a/t/t5800-remote-helpers.sh
+++ b/t/t5800-remote-helpers.sh
@@ -81,4 +81,51 @@ test_expect_success PYTHON_24 'pushing remote local repo' '
 	compare_refs clone HEAD server HEAD
 '
 
+test_expect_failure PYTHON_24 'fetch new branch' '
+	(cd public &&
+	 git checkout -b new &&
+	 echo content >>file &&
+	 git commit -a -m five &&
+	 git push origin new
+	) &&
+	(cd localclone &&
+	 git fetch origin new
+	) &&
+	compare_refs public HEAD localclone FETCH_HEAD
+'
+
+test_expect_failure PYTHON_24 'fetch multiple branches' '
+	(cd localclone &&
+	 git fetch
+	) &&
+	compare_refs server master localclone refs/remotes/origin/master &&
+	compare_refs server new localclone refs/remotes/origin/new
+'
+
+test_expect_failure PYTHON_24 'push when remote has extra refs' '
+	(cd clone &&
+	 echo content >>file &&
+	 git commit -a -m six &&
+	 git push
+	) &&
+	compare_refs clone master server master
+'
+
+test_expect_failure PYTHON_24 'push new branch by name' '
+	(cd clone &&
+	 git checkout -b new-name  &&
+	 echo content >>file &&
+	 git commit -a -m seven &&
+	 git push origin new-name
+	) &&
+	compare_refs clone HEAD server refs/heads/new-name
+'
+
+test_expect_failure PYTHON_24 'push new branch with old:new refspec' '
+	(cd clone &&
+	 git push origin new-name:new-refspec
+	) &&
+	compare_refs clone HEAD server refs/heads/new-refspec
+'
+
 test_done
-- 
1.7.6.rc0.35.gc40cb

[PATCH 5/8] teach remote-testgit to import non-HEAD refs

[Jeff King]

Upon receiving an "import" command, the testgit remote
helper would ignore the ref asked for by git and generate a
fast-export stream based on HEAD. Instead, we should
actually give git the ref it asked for.

This requires adding a new parameter to the export_repo
method in the remote-helpers python library, which may be
used by code outside of git.git. We use a default parameter
so that callers without the new parameter will get the same
behavior as before.

Signed-off-by: Jeff King <peff@peff.net>
---
 git-remote-testgit.py              |    2 +-
 git_remote_helpers/git/exporter.py |    3 ++-
 t/t5800-remote-helpers.sh          |    2 +-
 3 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/git-remote-testgit.py b/git-remote-testgit.py
index 937b858..e2ad98e 100644
--- a/git-remote-testgit.py
+++ b/git-remote-testgit.py
@@ -124,7 +124,7 @@ def do_import(repo, args):
         die("Need gitdir to import")
 
     repo = update_local_repo(repo)
-    repo.exporter.export_repo(repo.gitdir)
+    repo.exporter.export_repo(repo.gitdir, args)
     return True
 
 
diff --git a/git_remote_helpers/git/exporter.py b/git_remote_helpers/git/exporter.py
index f40f9d6..1855c6a 100644
--- a/git_remote_helpers/git/exporter.py
+++ b/git_remote_helpers/git/exporter.py
@@ -15,7 +15,7 @@ class GitExporter(object):
 
         self.repo = repo
 
-    def export_repo(self, base):
+    def export_repo(self, base, refs = ["HEAD"]):
         """Exports a fast-export stream for the given directory.
 
         Simply delegates to git fast-epxort and pipes it through sed
@@ -38,6 +38,7 @@ class GitExporter(object):
         sys.stdout.flush()
 
         args = ["git", "--git-dir=" + self.repo.gitpath, "fast-export", "--export-marks=" + path]
+	args.extend(refs)
 
         if os.path.exists(path):
             args.append("--import-marks=" + path)
diff --git a/t/t5800-remote-helpers.sh b/t/t5800-remote-helpers.sh
index a10a48d..562edf4 100755
--- a/t/t5800-remote-helpers.sh
+++ b/t/t5800-remote-helpers.sh
@@ -81,7 +81,7 @@ test_expect_success PYTHON_24 'pushing remote local repo' '
 	compare_refs clone HEAD server HEAD
 '
 
-test_expect_failure PYTHON_24 'fetch new branch' '
+test_expect_success PYTHON_24 'fetch new branch' '
 	(cd public &&
 	 git checkout -b new &&
 	 echo content >>file &&
-- 
1.7.6.rc0.35.gc40cb

[PATCH 6/8] teach remote-testgit to import multiple refs

[Jeff King]

When git wants to fetch multiple refs from a remote helper,
it will issue multiple "import" lines. In this case, testgit
would then run multiple fast-exports, one per commit. Not
only is this inefficient (since the refs may have shared
history), but the data stream will appear bogus to
fast-import, which sees the output of the exports
concatenated (specifically, each export starts with
"feature" lines, which are not allowed to come after "data"
lines).

Instead, the helper needs to collect the list of refs to be
imported, and then run a single fast-import with all of the
refs (once git has signalled to us the list is done by
hanging up).

Signed-off-by: Jeff King <peff@peff.net>
---
 git-remote-testgit.py     |   24 +++++++++++++++++++++---
 t/t5800-remote-helpers.sh |    2 +-
 2 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/git-remote-testgit.py b/git-remote-testgit.py
index e2ad98e..c04f1b3 100644
--- a/git-remote-testgit.py
+++ b/git-remote-testgit.py
@@ -113,9 +113,13 @@ def update_local_repo(repo):
     return repo
 
 
+to_import = []
 def do_import(repo, args):
-    """Exports a fast-import stream from testgit for git to import.
+    """Collect a set of refs to import; we must do the final
+       import at the end, since we only want to exec fast-export
+       once.
     """
+    global to_import
 
     if len(args) != 1:
         die("Import needs exactly one ref")
@@ -123,11 +127,24 @@ def do_import(repo, args):
     if not repo.gitdir:
         die("Need gitdir to import")
 
-    repo = update_local_repo(repo)
-    repo.exporter.export_repo(repo.gitdir, args)
+    to_import.append(args[0])
     return True
 
 
+def finalize_import(repo):
+    """Exports a fast-import stream from testgit for git to import;
+       we should have collected the list of refs already in
+       to_import.
+    """
+    global to_import
+
+    if len(to_import) == 0:
+	    return
+
+    repo = update_local_repo(repo)
+    repo.exporter.export_repo(repo.gitdir, to_import)
+
+
 def do_export(repo, args):
     """Imports a fast-import stream from git to testgit.
     """
@@ -199,6 +216,7 @@ def read_one_line(repo):
     cmdline = cmdline.strip().split()
     if not cmdline:
         # Blank line means we're about to quit
+        finalize_import(repo)
         return False
 
     cmd = cmdline.pop(0)
diff --git a/t/t5800-remote-helpers.sh b/t/t5800-remote-helpers.sh
index 562edf4..b28f2b3 100755
--- a/t/t5800-remote-helpers.sh
+++ b/t/t5800-remote-helpers.sh
@@ -94,7 +94,7 @@ test_expect_success PYTHON_24 'fetch new branch' '
 	compare_refs public HEAD localclone FETCH_HEAD
 '
 
-test_expect_failure PYTHON_24 'fetch multiple branches' '
+test_expect_success PYTHON_24 'fetch multiple branches' '
 	(cd localclone &&
 	 git fetch
 	) &&
-- 
1.7.6.rc0.35.gc40cb

[PATCH 7/8] transport-helper: don't feed bogus refs to export push

[Jeff King]

When we want to push to a remote helper that has the
"export" capability, we collect all of the refs we want to
push and then feed them to fast-export.

However, the list of refs is actually a list of remote refs,
not local refs. The mapped local refs are included via the
peer_ref pointer. So when we add an argument to our
fast-export command line, we must be sure to use the local
peer_ref name (and if there is no local name, it is because
we are not actually sending that ref, or we may not even
have the ref at all).

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t5800-remote-helpers.sh |    2 +-
 transport-helper.c        |    3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/t/t5800-remote-helpers.sh b/t/t5800-remote-helpers.sh
index b28f2b3..a6cc43b 100755
--- a/t/t5800-remote-helpers.sh
+++ b/t/t5800-remote-helpers.sh
@@ -102,7 +102,7 @@ test_expect_success PYTHON_24 'fetch multiple branches' '
 	compare_refs server new localclone refs/remotes/origin/new
 '
 
-test_expect_failure PYTHON_24 'push when remote has extra refs' '
+test_expect_success PYTHON_24 'push when remote has extra refs' '
 	(cd clone &&
 	 echo content >>file &&
 	 git commit -a -m six &&
diff --git a/transport-helper.c b/transport-helper.c
index b560b64..34d18aa 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -730,7 +730,8 @@ static int push_refs_with_export(struct transport *transport,
 		}
 		free(private);
 
-		string_list_append(&revlist_args, ref->name);
+		if (ref->peer_ref)
+			string_list_append(&revlist_args, ref->peer_ref->name);
 
 	}
 
-- 
1.7.6.rc0.35.gc40cb

[PATCH 8/8] git_remote_helpers: push all refs during a non-local export

[Jeff King]

When a remote helper exports to a non-local git repo, the
steps are roughly:

  1. fast-export into a local staging area; the set of
     interesting refs is defined by what is in the fast-export
     stream

  2. git push from the staging area to the non-local repo

In the second step, we should explicitly push all refs, not
just matching ones. This will let us push refs that do not
yet exist in the remote repo.

Signed-off-by: Jeff King <peff@peff.net>
---
 git_remote_helpers/git/non_local.py |    2 +-
 t/t5800-remote-helpers.sh           |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/git_remote_helpers/git/non_local.py b/git_remote_helpers/git/non_local.py
index f27389b..c53e074 100644
--- a/git_remote_helpers/git/non_local.py
+++ b/git_remote_helpers/git/non_local.py
@@ -63,7 +63,7 @@ class NonLocalGit(object):
         if not os.path.exists(path):
             die("could not find repo at %s", path)
 
-        args = ["git", "--git-dir=" + path, "push", "--quiet", self.repo.gitpath]
+        args = ["git", "--git-dir=" + path, "push", "--quiet", self.repo.gitpath, "--all"]
         child = subprocess.Popen(args)
         if child.wait() != 0:
             raise CalledProcessError
diff --git a/t/t5800-remote-helpers.sh b/t/t5800-remote-helpers.sh
index a6cc43b..682f813 100755
--- a/t/t5800-remote-helpers.sh
+++ b/t/t5800-remote-helpers.sh
@@ -111,7 +111,7 @@ test_expect_success PYTHON_24 'push when remote has extra refs' '
 	compare_refs clone master server master
 '
 
-test_expect_failure PYTHON_24 'push new branch by name' '
+test_expect_success PYTHON_24 'push new branch by name' '
 	(cd clone &&
 	 git checkout -b new-name  &&
 	 echo content >>file &&
-- 
1.7.6.rc0.35.gc40cb

Re: [PATCH 3/8] t5800: factor out some ref tests

[Sverre Rabbelier]

Heya,

On Tue, Jun 7, 2011 at 19:20, Jeff King <peff@peff.net> wrote:
> These are a little hard to read, and I'm about to add more
> just like them. Plus the failure output is nicer if we use
> test_cmp than a comparison with "test".

Ah, nice :)

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 4/8] t5800: document some non-functional parts of remote helpers

[Sverre Rabbelier]

Heya,

On Tue, Jun 7, 2011 at 19:20, Jeff King <peff@peff.net> wrote:
> There are two possible solutions:

3. As mentioned in a different series, I feel it would be preferable
if fast-import/fast-export did not actually modify any refs, rather
the remote-helper should specify which refs should be updated to what
value explicitly. I think the same should/could apply in this case.
Does that make sense?

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 4/8] t5800: document some non-functional parts of remote helpers

[Jeff King]

On Tue, Jun 07, 2011 at 07:25:49PM +0200, Sverre Rabbelier wrote:

> Heya,
> 
> On Tue, Jun 7, 2011 at 19:20, Jeff King <peff@peff.net> wrote:
> > There are two possible solutions:
> 
> 3. As mentioned in a different series, I feel it would be preferable
> if fast-import/fast-export did not actually modify any refs, rather
> the remote-helper should specify which refs should be updated to what
> value explicitly. I think the same should/could apply in this case.
> Does that make sense?

Isn't that the case already with import? The helper writes into a
staging area (like refs/testgit), and then we let git pick the results
out of there.

For export, that would be the same as my (2). The problem is that we
don't feed the helper _any_ information about which refs were desired.
All it gets is the fast-export stream.

-Peff

Re: [PATCH 7/8] transport-helper: don't feed bogus refs to export push

[Sverre Rabbelier]

Heya,

On Tue, Jun 7, 2011 at 19:21, Jeff King <peff@peff.net> wrote:
> When we want to push to a remote helper that has the
> "export" capability, we collect all of the refs we want to
> push and then feed them to fast-export.

Whoah! I think this fixes the bug that's been preventing me from using
git-remote-hg for pushing! If it is, I can finally send it out for
review!

Thanks for spending time on this Peff!

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 8/8] git_remote_helpers: push all refs during a non-local export

[Sverre Rabbelier]

Heya,

On Tue, Jun 7, 2011 at 19:21, Jeff King <peff@peff.net> wrote:
> When a remote helper exports to a non-local git repo, the
> steps are roughly:
>
>  1. fast-export into a local staging area; the set of
>     interesting refs is defined by what is in the fast-export
>     stream
>
>  2. git push from the staging area to the non-local repo
>
> In the second step, we should explicitly push all refs, not
> just matching ones. This will let us push refs that do not
> yet exist in the remote repo.

That repo is re-used though. Wouldn't this break for:

$ git push branch-a
$ # make commits on branch-a that shouldn't be pushed yet
$ git push branch-b

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 4/8] t5800: document some non-functional parts of remote helpers

[Sverre Rabbelier]

Heya,

On Tue, Jun 7, 2011 at 19:28, Jeff King <peff@peff.net> wrote:
> Isn't that the case already with import? The helper writes into a
> staging area (like refs/testgit), and then we let git pick the results
> out of there.

The staging area solution is sub-optimal, see [0] for the thread I was
referring to.

[0] http://thread.gmane.org/gmane.comp.version-control.git/174693/focus=174787

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 8/8] git_remote_helpers: push all refs during a non-local export

[Jeff King]

On Tue, Jun 07, 2011 at 07:32:22PM +0200, Sverre Rabbelier wrote:

> On Tue, Jun 7, 2011 at 19:21, Jeff King <peff@peff.net> wrote:
> > When a remote helper exports to a non-local git repo, the
> > steps are roughly:
> >
> >  1. fast-export into a local staging area; the set of
> >     interesting refs is defined by what is in the fast-export
> >     stream
> >
> >  2. git push from the staging area to the non-local repo
> >
> > In the second step, we should explicitly push all refs, not
> > just matching ones. This will let us push refs that do not
> > yet exist in the remote repo.
> 
> That repo is re-used though. Wouldn't this break for:
> 
> $ git push branch-a
> $ # make commits on branch-a that shouldn't be pushed yet
> $ git push branch-b

No, because our fast-export from git will not include branch-a, and this
is about pushing from the exported staging area.

If the remote rewound their branch-a, we would actually fast-forward it
back again. This is no different than the current behavior, though,
which uses matching[1]. It would also update branch-a because it exists
upstream.

A related problem is branch deletion. I don't know that I can delete via
a fast-export stream. So once a branch exists in the staging area, it
will never go away.

So I think the right solution is really that the "export" command needs
to communicate the ref pairs to the remote helper so that it can do the
right thing (which would include renaming or deleting).

-Peff

[1] Actually, isn't the default push behavior due to change soon? That
would further break the existing code.

Re: [PATCH 8/8] git_remote_helpers: push all refs during a non-local export

[Sverre Rabbelier]

Heya,

On Tue, Jun 7, 2011 at 19:42, Jeff King <peff@peff.net> wrote:
> No, because our fast-export from git will not include branch-a, and this
> is about pushing from the exported staging area.
>
> If the remote rewound their branch-a, we would actually fast-forward it
> back again. This is no different than the current behavior, though,
> which uses matching[1]. It would also update branch-a because it exists
> upstream.

Ah, you are right :).

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 2/8] git-remote-testgit: exit gracefully after push

[Sverre Rabbelier]

Heya,

On Tue, Jun 7, 2011 at 19:19, Jeff King <peff@peff.net> wrote:
> This patch lets each command handler tell the main loop
> whether it should expect more commands or not; the handler
> exits gracefully if no more are expected.

I have a (much) better solution to this queued locally: the 'done'
feature to fast-import. I've sent the series out once, but I never
followed up on the comments it received. Patch series forthcoming.

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 4/8] t5800: document some non-functional parts of remote helpers

[Jeff King]

On Tue, Jun 07, 2011 at 07:34:04PM +0200, Sverre Rabbelier wrote:

> On Tue, Jun 7, 2011 at 19:28, Jeff King <peff@peff.net> wrote:
> > Isn't that the case already with import? The helper writes into a
> > staging area (like refs/testgit), and then we let git pick the results
> > out of there.
> 
> The staging area solution is sub-optimal, see [0] for the thread I was
> referring to.
> 
> [0] http://thread.gmane.org/gmane.comp.version-control.git/174693/focus=174787

Thanks. I'm not sure of some details, though. In particular, we also use
the staging area during push to know which parts of the history we
_don't_ have to send. So where will the remote helper store that state
if not in this staging repo?

-Peff

Re: [PATCH 4/8] t5800: document some non-functional parts of remote helpers

[Sverre Rabbelier]

Heya,

On Tue, Jun 7, 2011 at 19:51, Jeff King <peff@peff.net> wrote:
> Thanks. I'm not sure of some details, though. In particular, we also use
> the staging area during push to know which parts of the history we
> _don't_ have to send. So where will the remote helper store that state
> if not in this staging repo?

Oh, I didn't mean the staging _repo_. I meant the refs/testgit namespace.

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 4/8] t5800: document some non-functional parts of remote helpers

[Jeff King]

On Tue, Jun 07, 2011 at 07:53:01PM +0200, Sverre Rabbelier wrote:

> On Tue, Jun 7, 2011 at 19:51, Jeff King <peff@peff.net> wrote:
> > Thanks. I'm not sure of some details, though. In particular, we also use
> > the staging area during push to know which parts of the history we
> > _don't_ have to send. So where will the remote helper store that state
> > if not in this staging repo?
> 
> Oh, I didn't mean the staging _repo_. I meant the refs/testgit namespace.

Oh. :) Then yeah, I think that would be more elegant.

-Peff

Re: [PATCH 3/3] clone: always fetch remote HEAD

[Jeff King]

On Mon, Jun 06, 2011 at 06:08:21PM -0400, Jeff King wrote:

> On Mon, Jun 06, 2011 at 01:31:54PM -0700, Junio C Hamano wrote:
> 
> > >  static struct ref *wanted_peer_refs(const struct ref *refs,
> > >  		struct refspec *refspec)
> > >  {
> > > -	struct ref *local_refs = NULL;
> > > -	struct ref **tail = &local_refs;
> > > +	struct ref *head = get_remote_ref(refs, "HEAD");
> > 
> > The rest of the patch looked quite sane but I wonder if this should be
> > using get_remote_ref() that calls find_ref_by_name_abbrev() which in turn
> > would hit "refs/heads/HEAD" if the remote side didn't give you "HEAD".
> > Shouldn't it be using find_ref_by_name() directly?
> 
> Ick, yeah, that was just me blindly cutting down what get_fetch_map was
> doing to the bit that I wanted, and thinking get_remote_ref was it.  I
> didn't even notice the fact that it was using the _abbrev form of
> find_ref_by_name.
> 
> It should definitely be an exact match. I'll fix it in my re-roll.

OK, here is the re-roll. I've omitted the first two patches, which are
the same as before. So this replaces the old 3/3. When merged with (or
rebase on top of) the remote-helpers fixes I posted yesterday, all tests
pass.

  [1/2]: make copy_ref globally available
  [2/2]: clone: always fetch remote HEAD

-Peff

[PATCH 1/2] make copy_ref globally available

[Jeff King]

This is a useful function, and we have already made the
similar alloc_ref and copy_ref_list available.

Signed-off-by: Jeff King <peff@peff.net>
---
 remote.c |    2 +-
 remote.h |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/remote.c b/remote.c
index f073b1e..b8ecfa5 100644
--- a/remote.c
+++ b/remote.c
@@ -896,7 +896,7 @@ struct ref *alloc_ref(const char *name)
 	return alloc_ref_with_prefix("", 0, name);
 }
 
-static struct ref *copy_ref(const struct ref *ref)
+struct ref *copy_ref(const struct ref *ref)
 {
 	struct ref *cpy;
 	size_t len;
diff --git a/remote.h b/remote.h
index 888d7c1..9a30a9d 100644
--- a/remote.h
+++ b/remote.h
@@ -70,7 +70,7 @@ struct refspec {
 extern const struct refspec *tag_refspec;
 
 struct ref *alloc_ref(const char *name);
-
+struct ref *copy_ref(const struct ref *ref);
 struct ref *copy_ref_list(const struct ref *ref);
 
 int check_ref_type(const struct ref *ref, int flags);
-- 
1.7.6.rc0.39.g6b091.dirty

[PATCH 2/2] clone: always fetch remote HEAD

[Jeff King]

In most cases, fetching the remote HEAD explicitly is
unnecessary. It's just a symref pointing to a branch which
we are already fetching, so we will already ask for its sha1.

However, if the remote has a detached HEAD, things are less
certain. We do not ask for HEAD's sha1, but we do try to
write it into a local detached HEAD. In most cases this is
fine, as the remote HEAD is pointing to some part of the
history graph that we will fetch via the refs.

But if the remote HEAD points to an "orphan" commit (one
which was is not an ancestor of any refs), then we will not
have the object, and update_ref will complain when we try to
write the detached HEAD, aborting the whole clone.

This patch makes clone always explicitly ask the remote for
the sha1 of its HEAD commit. In the non-detached case, this
is a no-op, as we were going to ask for that sha1 anyway. In
the regular detached case, this will add an extra "want" to
the protocol negotiation, but will not change the history
that gets sent. And in the detached orphan case, we will
fetch the orphaned history so that we can write it into our
local detached HEAD.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/clone.c           |   10 +++++++---
 t/t5707-clone-detached.sh |    6 +++---
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index f579794..f134ff8 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -343,8 +343,9 @@ static void remove_junk_on_signal(int signo)
 static struct ref *wanted_peer_refs(const struct ref *refs,
 		struct refspec *refspec)
 {
-	struct ref *local_refs = NULL;
-	struct ref **tail = &local_refs;
+	struct ref *head = copy_ref(find_ref_by_name(refs, "HEAD"));
+	struct ref *local_refs = head;
+	struct ref **tail = head ? &head->next : &local_refs;
 
 	get_fetch_map(refs, refspec, &tail, 0);
 	if (!option_mirror)
@@ -357,8 +358,11 @@ static void write_remote_refs(const struct ref *local_refs)
 {
 	const struct ref *r;
 
-	for (r = local_refs; r; r = r->next)
+	for (r = local_refs; r; r = r->next) {
+		if (!r->peer_ref)
+			continue;
 		add_extra_ref(r->peer_ref->name, r->old_sha1, 0);
+	}
 
 	pack_refs(PACK_REFS_ALL);
 	clear_extra_refs();
diff --git a/t/t5707-clone-detached.sh b/t/t5707-clone-detached.sh
index db4af95..82ecbce 100755
--- a/t/t5707-clone-detached.sh
+++ b/t/t5707-clone-detached.sh
@@ -58,18 +58,18 @@ test_expect_success 'cloned HEAD is detached' '
 	head_is_detached detached-history
 '
 
-test_expect_failure 'clone repo (orphan detached HEAD)' '
+test_expect_success 'clone repo (orphan detached HEAD)' '
 	git checkout --detach master &&
 	echo four >file &&
 	git commit -a -m four &&
 	git clone "file://$PWD" detached-orphan
 '
-test_expect_failure 'cloned HEAD matches' '
+test_expect_success 'cloned HEAD matches' '
 	echo four >expect &&
 	git --git-dir=detached-orphan/.git log -1 --format=%s >actual &&
 	test_cmp expect actual
 '
-test_expect_failure 'cloned HEAD is detached' '
+test_expect_success 'cloned HEAD is detached' '
 	head_is_detached detached-orphan
 '
 
-- 
1.7.6.rc0.39.g6b091.dirty

Re: [PATCH 3/3] clone: always fetch remote HEAD

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> It should definitely be an exact match. I'll fix it in my re-roll.
>
> OK, here is the re-roll. I've omitted the first two patches, which are
> the same as before. So this replaces the old 3/3.

Looked sane. Thanks; will replace.

Re: [PATCH 4/8] t5800: document some non-functional parts of remote helpers

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> The third test demonstrates a bug in git's side of the
> helper code when the upstream has added new refs without us.

without us knowing, you mean?

Re: [PATCH 5/8] teach remote-testgit to import non-HEAD refs

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> diff --git a/git_remote_helpers/git/exporter.py b/git_remote_helpers/git/exporter.py
> index f40f9d6..1855c6a 100644
> --- a/git_remote_helpers/git/exporter.py
> +++ b/git_remote_helpers/git/exporter.py
> @@ -15,7 +15,7 @@ class GitExporter(object):
>  
>          self.repo = repo
>  
> -    def export_repo(self, base):
> +    def export_repo(self, base, refs = ["HEAD"]):

This seems like an accident waiting to happen, even though it is Ok with
the current code (because this method does not modify refs in any way), to
specify a mutable object as the default value for an optional parameter.

> @@ -38,6 +38,7 @@ class GitExporter(object):
>          sys.stdout.flush()
>  
>          args = ["git", "--git-dir=" + self.repo.gitpath, "fast-export", "--export-marks=" + path]
> +	args.extend(refs)
>  
>          if os.path.exists(path):
>              args.append("--import-marks=" + path)

Hmm, am I looking at the right version of this file?

I see args.append("HEAD") after this --import-marks in the existing code,
which this patch does not remove.

An existing script that does not pass the new argument would end up
feeding HEAD twice, and if a new script does pass the new argument that
does not have "HEAD", it would still feed HEAD, to the underlying
fast-export.

I suspect that duplicated HEAD would not hurt for existing callers, but I
am not sure feeding HEAD unconditionally is a right thing to do.

Re: [PATCH 4/8] t5800: document some non-functional parts of remote helpers

[Jeff King]

On Wed, Jun 08, 2011 at 04:19:29PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > The third test demonstrates a bug in git's side of the
> > helper code when the upstream has added new refs without us.
> 
> without us knowing, you mean?

I guess it depends what you mean by knowing. The bug is shown when the
remote has a ref that we don't; we try to feed the name of that ref to
fast-export, which of course doesn't work, because we don't have
anything by that name.

So in that sense, no, we don't know about the ref. But it is not about
us knowing about the remote having the ref; the problem is that we _do_
know that the remote has that ref, and assume we have a matching one.

-Peff

Re: [PATCH 5/8] teach remote-testgit to import non-HEAD refs

[Jeff King]

On Wed, Jun 08, 2011 at 04:21:21PM -0700, Junio C Hamano wrote:

> > diff --git a/git_remote_helpers/git/exporter.py b/git_remote_helpers/git/exporter.py
> > index f40f9d6..1855c6a 100644
> > --- a/git_remote_helpers/git/exporter.py
> > +++ b/git_remote_helpers/git/exporter.py
> > @@ -15,7 +15,7 @@ class GitExporter(object):
> >  
> >          self.repo = repo
> >  
> > -    def export_repo(self, base):
> > +    def export_repo(self, base, refs = ["HEAD"]):
> 
> This seems like an accident waiting to happen, even though it is Ok with
> the current code (because this method does not modify refs in any way), to
> specify a mutable object as the default value for an optional parameter.

Yeah, I didn't realize that python assigns default values by reference,
unlike other languages (this is actually the first python I've ever
written).

At any rate, I think this patch is obsoleted by Sverre's new series
(though I haven't read it in detail yet).

> >          sys.stdout.flush()
> >  
> >          args = ["git", "--git-dir=" + self.repo.gitpath, "fast-export", "--export-marks=" + path]
> > +	args.extend(refs)
> >  
> >          if os.path.exists(path):
> >              args.append("--import-marks=" + path)
> 
> Hmm, am I looking at the right version of this file?
> 
> I see args.append("HEAD") after this --import-marks in the existing code,
> which this patch does not remove.

Ugh, yeah. I missed that "HEAD". So this version is buggy in that it
exports everything you asked for, _plus_ HEAD. Which isn't a problem in
practice, but it's inefficient. That "HEAD" should definitely go away.

I'll check for it in Sverre's series and make sure he did it right. :)

-Peff

Re: [PATCH 4/8] t5800: document some non-functional parts of remote helpers

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Jun 08, 2011 at 04:19:29PM -0700, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> > The third test demonstrates a bug in git's side of the
>> > helper code when the upstream has added new refs without us.
>> 
>> without us knowing, you mean?
>
> I guess it depends what you mean by knowing. The bug is shown when the
> remote has a ref that we don't; we try to feed the name of that ref to
> fast-export, which of course doesn't work, because we don't have
> anything by that name.
>
> So in that sense, no, we don't know about the ref. But it is not about
> us knowing about the remote having the ref; the problem is that we _do_
> know that the remote has that ref, and assume we have a matching one.

Ok, I think I understood what the sentence wanted to say ("the upstream
added new refs. We do not have them yet", right?); it was just I found the
phrase "... without us" didn't sit well there to my ears.

Re: [PATCH 4/8] t5800: document some non-functional parts of remote helpers

[Jeff King]

On Wed, Jun 08, 2011 at 05:43:20PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > On Wed, Jun 08, 2011 at 04:19:29PM -0700, Junio C Hamano wrote:
> >
> >> Jeff King <peff@peff.net> writes:
> >> 
> >> > The third test demonstrates a bug in git's side of the
> >> > helper code when the upstream has added new refs without us.
> >> 
> >> without us knowing, you mean?
> >
> > I guess it depends what you mean by knowing. The bug is shown when the
> > remote has a ref that we don't; we try to feed the name of that ref to
> > fast-export, which of course doesn't work, because we don't have
> > anything by that name.
> >
> > So in that sense, no, we don't know about the ref. But it is not about
> > us knowing about the remote having the ref; the problem is that we _do_
> > know that the remote has that ref, and assume we have a matching one.
> 
> Ok, I think I understood what the sentence wanted to say ("the upstream
> added new refs. We do not have them yet", right?); it was just I found the
> phrase "... without us" didn't sit well there to my ears.

Fair enough. I think a better wording would be "...the upstream has refs
that we do not have locally", which is more accurate and more clear.

-Peff

Re: [PATCH 4/8] t5800: document some non-functional parts of remote helpers

[Sverre Rabbelier]

Heya,

On Thu, Jun 9, 2011 at 02:45, Jeff King <peff@peff.net> wrote:
> Fair enough. I think a better wording would be "...the upstream has refs
> that we do not have locally", which is more accurate and more clear.

Fixed locally. Although, this probably needs a new commit message anyway.

-- 
Cheers,

Sverre Rabbelier