threads / patch / 38398

patchtransport-helper: do not request symbolic refs to remote helpers

Subject: [PATCH] transport-helper: do not request symbolic refs to remote helpers

## tl;dr

8 messages between Jan 19, 2015 and Jan 22, 2015. Diffs are folded; open one to read it.

replies: 7people: 2as markdown or json

Mike Hommey· Jan 19, 2015, 01:35 UTC · lore

A typical remote helper will return a `list` of refs containing a symbolic ref HEAD, pointing to, e.g. refs/heads/master. In the case of a clone, all the refs are being requested through `fetch` or `import`, including the symbolic ref.

While this works properly, in some cases of a fetch, like `git fetch url` or `git fetch origin HEAD`, or any fetch command involving a symbolic ref without also fetching the corresponding ref it points to, the fetch command fails with:

  fatal: bad object 0000000000000000000000000000000000000000
  error: <remote> did not send all necessary objects
(in the case the remote helper returned '?' values to the `list` command).

This is because there is only one ref given to fetch(), and it's not further resolved to something at the end of fetch_with_import().

While this can be somehow handled in the remote helper itself, by adding a refspec for the symbolic ref, and storing an explicit ref in a private namespace, and then handling the `import` for that symbolic ref specifically, very few existing remote helpers are actually doing that.

So, instead of requesting the exact list of wanted refs to remote helpers, treat symbolic refs differently and request the ref they point to instead. Then, resolve the symbolic refs values based on the pointed ref. This assumes there is no more than one level of indirection (a symbolic ref doesn't point to another symbolic ref).

Signed-off-by: Mike Hommey <mh@glandium.org>
---
 git-remote-testgit.sh     |  8 +++++++-
 t/t5801-remote-helpers.sh | 24 ++++++++++++++++++++++++
 transport-helper.c        | 13 ++++++++-----
 3 files changed, 39 insertions(+), 6 deletions(-)
Show changes to 3 files +39 −6

git-remote-testgit.sh, t/t5801-remote-helpers.sh, transport-helper.c

diff --git a/git-remote-testgit.sh b/git-remote-testgit.sh
index a9c75a2..752c763 100755
--- a/git-remote-testgit.sh
+++ b/git-remote-testgit.sh
@@ -1,7 +1,13 @@
 #!/bin/sh
 # Copyright (c) 2012 Felipe Contreras
 
-alias=$1
+# The first argument can be a url when the fetch/push command was a url
+# instead of a configured remote. In this case, use a generic alias.
+if test "$1" = "testgit::$2"; then
+	alias=_
+else
+	alias=$1
+fi
 url=$2
 
 dir="$GIT_DIR/testgit/$alias"
diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh
index 2419407..c9d3ed1 100755
--- a/t/t5801-remote-helpers.sh
+++ b/t/t5801-remote-helpers.sh
@@ -281,4 +281,28 @@ test_expect_success 'push messages' '
 	)
 '
 
+test_expect_success 'fetch HEAD' '
+	(cd server &&
+	git checkout master &&
+	echo more >>file &&
+	git commit -a -m more
+	) &&
+	(cd local &&
+	git fetch origin HEAD
+	) &&
+	compare_refs server HEAD local FETCH_HEAD
+'
+
+test_expect_success 'fetch url' '
+	(cd server &&
+	git checkout master &&
+	echo more >>file &&
+	git commit -a -m more
+	) &&
+	(cd local &&
+	git fetch "testgit::${PWD}/../server"
+	) &&
+	compare_refs server HEAD local FETCH_HEAD
+'
+
 test_done
diff --git a/transport-helper.c b/transport-helper.c
index 0224687..27c82f7 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -356,7 +356,8 @@ static int fetch_with_fetch(struct transport *transport,
 			continue;
 
 		strbuf_addf(&buf, "fetch %s %s\n",
-			    sha1_to_hex(posn->old_sha1), posn->name);
+			    sha1_to_hex(posn->old_sha1),
+			    posn->symref ? posn->symref : posn->name);
 	}
 
 	strbuf_addch(&buf, '\n');
@@ -454,7 +455,8 @@ static int fetch_with_import(struct transport *transport,
 		if (posn->status & REF_STATUS_UPTODATE)
 			continue;
 
-		strbuf_addf(&buf, "import %s\n", posn->name);
+		strbuf_addf(&buf, "import %s\n",
+			    posn->symref ? posn->symref : posn->name);
 		sendline(data, &buf);
 		strbuf_reset(&buf);
 	}
@@ -487,14 +489,15 @@ static int fetch_with_import(struct transport *transport,
 	 * fast-forward or this is a forced update.
 	 */
 	for (i = 0; i < nr_heads; i++) {
-		char *private;
+		char *private, *name;
 		posn = to_fetch[i];
 		if (posn->status & REF_STATUS_UPTODATE)
 			continue;
+		name = posn->symref ? posn->symref : posn->name;
 		if (data->refspecs)
-			private = apply_refspecs(data->refspecs, data->refspec_nr, posn->name);
+			private = apply_refspecs(data->refspecs, data->refspec_nr, name);
 		else
-			private = xstrdup(posn->name);
+			private = xstrdup(name);
 		if (private) {
 			read_ref(private, posn->old_sha1);
 			free(private);
-- 
2.2.2.dirty
Junio C Hamano· Jan 22, 2015, 06:46 UTC · re: Mike Hommey · lore

Re: [PATCH] transport-helper: do not request symbolic refs to remote helpers

Mike Hommey <mh@glandium.org> writes:
Show 14 quoted lines
> A typical remote helper will return a `list` of refs containing a symbolic
> ref HEAD, pointing to, e.g. refs/heads/master. In the case of a clone, all
> the refs are being requested through `fetch` or `import`, including the
> symbolic ref.
>
> While this works properly, in some cases of a fetch, like `git fetch url`
> or `git fetch origin HEAD`, or any fetch command involving a symbolic ref
> without also fetching the corresponding ref it points to, the fetch command
> fails with:
>
>   fatal: bad object 0000000000000000000000000000000000000000
>   error: <remote> did not send all necessary objects
>
> (in the case the remote helper returned '?' values to the `list` command).
Hmph.

Since the most "typical remote helper" I immediately think of is remote-curl and "git fetch https://code.googlesource.com/git HEAD" does not seem to fail that way, I am not sure what to make of the above. It is unclear if you meant that the above is inherent due to the way how remote helper protocol works (e.g. there is only one thing we can associate with a ref and we cannot say "HEAD points at this commit" at the same time we say "HEAD points at refs/heads/master"), or just due to broken or lazy implementation of the remote helpers that are invoked by transport-helper.c interface.

> This is because there is only one ref given to fetch(), and it's not
> further resolved to something at the end of fetch_with_import().
There is no get_refs_list() or something similar involved?
Mike Hommey· Jan 22, 2015, 07:03 UTC · re: Junio C Hamano · lore

Re: [PATCH] transport-helper: do not request symbolic refs to remote helpers

On Wed, Jan 21, 2015 at 10:46:48PM -0800, Junio C Hamano wrote:
Show 28 quoted lines
> Mike Hommey <mh@glandium.org> writes:
> 
> > A typical remote helper will return a `list` of refs containing a symbolic
> > ref HEAD, pointing to, e.g. refs/heads/master. In the case of a clone, all
> > the refs are being requested through `fetch` or `import`, including the
> > symbolic ref.
> >
> > While this works properly, in some cases of a fetch, like `git fetch url`
> > or `git fetch origin HEAD`, or any fetch command involving a symbolic ref
> > without also fetching the corresponding ref it points to, the fetch command
> > fails with:
> >
> >   fatal: bad object 0000000000000000000000000000000000000000
> >   error: <remote> did not send all necessary objects
> >
> > (in the case the remote helper returned '?' values to the `list` command).
> 
> Hmph.
> 
> Since the most "typical remote helper" I immediately think of is
> remote-curl and "git fetch https://code.googlesource.com/git HEAD"
> does not seem to fail that way, I am not sure what to make of the
> above.  It is unclear if you meant that the above is inherent due to
> the way how remote helper protocol works (e.g. there is only one
> thing we can associate with a ref and we cannot say "HEAD points at
> this commit" at the same time we say "HEAD points at
> refs/heads/master"), or just due to broken or lazy implementation of
> the remote helpers that are invoked by transport-helper.c interface.

Note the most important part is actually between the parens: that only happens when the remote helper returns '?' to the `list` command, which non-git remotes helpers (like git-remote-hg or git-remote-bzr) do. git-remote-testgit also does, so if you only apply the test parts of the patch, you'll see that the test fails.

remote-curl probably doesn't hit the problem because it's not returning '?' to `list`.

Mike
Junio C Hamano· Jan 22, 2015, 07:41 UTC · re: Mike Hommey · lore

Re: [PATCH] transport-helper: do not request symbolic refs to remote helpers

Mike Hommey <mh@glandium.org> writes:
Show 8 quoted lines
> Note the most important part is actually between the parens: that only
> happens when the remote helper returns '?' to the `list` command, which
> non-git remotes helpers (like git-remote-hg or git-remote-bzr) do.
> git-remote-testgit also does, so if you only apply the test parts of the
> patch, you'll see that the test fails.
>
> remote-curl probably doesn't hit the problem because it's not returning
> '?' to `list`.

Hmm, that suggests that the new codepath should be taken only when the remote helper says '?' (does it mean "I dunno"? where are these documented, by the way?), yes? It wasn't immediately obvious from the patch text that it was the case.

Thanks.
Mike Hommey· Jan 22, 2015, 08:06 UTC · re: Junio C Hamano · lore

Re: [PATCH] transport-helper: do not request symbolic refs to remote helpers

On Wed, Jan 21, 2015 at 11:41:51PM -0800, Junio C Hamano wrote:
Show 14 quoted lines
> Mike Hommey <mh@glandium.org> writes:
> 
> > Note the most important part is actually between the parens: that only
> > happens when the remote helper returns '?' to the `list` command, which
> > non-git remotes helpers (like git-remote-hg or git-remote-bzr) do.
> > git-remote-testgit also does, so if you only apply the test parts of the
> > patch, you'll see that the test fails.
> >
> > remote-curl probably doesn't hit the problem because it's not returning
> > '?' to `list`.
> 
> Hmm, that suggests that the new codepath should be taken only when
> the remote helper says '?' (does it mean "I dunno"? where are these
> documented, by the way?)
in Documentation/gitremote-helpers.txt
> , yes?  It wasn't immediately obvious from
> the patch text that it was the case.

The patch changes the behavior in all cases, because it didn't feel necessary to have a different behavior between the "normal" case and the '?' case: it makes sense to request the ref being pointed at than the symbolic ref in every case. Moreover, this makes existing non-git remote-helpers work without having to modify them to provide a refspec for HEAD (none of the 5 mercurial remote-helpers I checked do). The paragraph before last in the commit message mentioned this in maybe not very clear terms.

Mike
Junio C Hamano· Jan 22, 2015, 17:52 UTC · re: Mike Hommey · lore

Re: [PATCH] transport-helper: do not request symbolic refs to remote helpers

Mike Hommey <mh@glandium.org> writes:
Show 8 quoted lines
> The patch changes the behavior in all cases, because it didn't feel
> necessary to have a different behavior between the "normal" case and the
> '?' case: it makes sense to request the ref being pointed at than the
> symbolic ref in every case.
>
> Moreover, this makes existing non-git remote-helpers work without
> having to modify them to provide a refspec for HEAD (none of the 5
> mercurial remote-helpers I checked do).

I do not question the latter. It is not surprising if all of them share the same limitation that shares the same root in the same impedance mismatch.

The trouble I had in supporting "makes sense ... in every case" was that you said that the code as patched would not work for a symref pointing at another symref. The original code did not have that problem with remote helpers that support the 'list' command.

Does the new code avoid regressions for them and if so how? That is what was needed in the justification.

For remote helpers that support the 'list' command, asking for a symref and asking for a ref that the symref points at both work OK and behave the same, and hopefully that would be true even when the latter is a symref that points yet another ref, so dereferencing only one level on our end when making a request, instead of letting the remote side dereference, is not likely to cause regression.

Mike Hommey· Jan 22, 2015, 22:13 UTC · re: Junio C Hamano · lore

Re: [PATCH] transport-helper: do not request symbolic refs to remote helpers

On Thu, Jan 22, 2015 at 09:52:55AM -0800, Junio C Hamano wrote:
Show 29 quoted lines
> Mike Hommey <mh@glandium.org> writes:
> 
> > The patch changes the behavior in all cases, because it didn't feel
> > necessary to have a different behavior between the "normal" case and the
> > '?' case: it makes sense to request the ref being pointed at than the
> > symbolic ref in every case.
> >
> > Moreover, this makes existing non-git remote-helpers work without
> > having to modify them to provide a refspec for HEAD (none of the 5
> > mercurial remote-helpers I checked do).
> 
> I do not question the latter.  It is not surprising if all of them
> share the same limitation that shares the same root in the same
> impedance mismatch.
> 
> The trouble I had in supporting "makes sense ... in every case" was
> that you said that the code as patched would not work for a symref
> pointing at another symref.  The original code did not have that
> problem with remote helpers that support the 'list' command.
> 
> Does the new code avoid regressions for them and if so how?  That is
> what was needed in the justification.
> 
> For remote helpers that support the 'list' command, asking for a
> symref and asking for a ref that the symref points at both work OK
> and behave the same, and hopefully that would be true even when the
> latter is a symref that points yet another ref, so dereferencing
> only one level on our end when making a request, instead of letting
> the remote side dereference, is not likely to cause regression.

If I'm not mistaken, in that case with more than one level of symref, nothing would break more than it already is, the bug would only not be fixed for that case. That said, does this theoretical double indirection actually happen in the wild? For one, afaict, it's not even possible to create such a double indirection with git update-ref. You have to edit a .git/refs/ file manually.

Mike
Junio C Hamano· Jan 22, 2015, 22:24 UTC · re: Mike Hommey · lore

Re: [PATCH] transport-helper: do not request symbolic refs to remote helpers

Mike Hommey <mh@glandium.org> writes:
Show 15 quoted lines
> On Thu, Jan 22, 2015 at 09:52:55AM -0800, Junio C Hamano wrote:
>
>> Does the new code avoid regressions for them and if so how?  That is
>> what was needed in the justification.
>> 
>> For remote helpers that support the 'list' command, asking for a
>> symref and asking for a ref that the symref points at both work OK
>> and behave the same, and hopefully that would be true even when the
>> latter is a symref that points yet another ref, so dereferencing
>> only one level on our end when making a request, instead of letting
>> the remote side dereference, is not likely to cause regression.
>
> If I'm not mistaken, in that case with more than one level of symref,
> nothing would break more than it already is, the bug would only not be
> fixed for that case.
Yes, I think we are in agreement.  All is well.
> That said, does this theoretical double indirection actually
> happen in the wild?

With the proliferation of Git-using people and third-party tools that work with Git, I think the value of asking that question has diminished. People do strange things.

And I do not think the patch under discussion does not introduce any new theoretical funnies; it fixes one known case and leaves the rest unfixed, without introducing any new breakage, which is perfectly fine and exactly how we want to make progress. If the unfixed one has a real-world need to be fixed, somebody will raise hand, and if they do not bother to even raise their hands, that is an indication that it is not worth our time worrying about it.

The only thing we need to avoid, while making "one step at a time" progress, is to paint ourselves to a corner we cannot get out of by promising too much --- and I do not think the patch under discussion does that, either.

← back to recent threads