* [PATCH] remote-curl: don't pass back fake refs
@ 2011-12-17 10:45 Jeff King
2011-12-19 17:10 ` Jeff King
0 siblings, 1 reply; 6+ messages in thread
From: Jeff King @ 2011-12-17 10:45 UTC (permalink / raw)
To: git; +Cc: Shawn O. Pearce, Junio C Hamano
When receive-pack advertises its list of refs, it generally
hides the capabilities information after a NUL at the end of
the first ref. However, when we have an empty repository,
there are no refs, and therefore receive-pack writes a fake
ref "capabilities^{}" with the capabilities afterwards.
On the client side, git reads the result with
get_remote_heads. We pick the capabilities from the end of
the line, and then call check_ref to make sure the ref name
is valid. We see that it isn't, and don't bother adding it
to our list of refs.
However, the call to check_ref is enabled by passing the
REF_NORMAL flag to get_remote_heads. For the regular git
transport, we pass REF_NORMAL in get_refs_via_connect if we
are doing a push (since only receive-pack uses this fake
ref). But in remote-curl, we never use this flag, and we
accept the fake ref as a real one, passing it back from the
helper to the parent git-push.
Most of the time this bug goes unnoticed, as the fake ref
won't match our refspecs. However, if "--mirror" is used,
then we see it as remote cruft to be pruned, and try to pass
along a deletion refspec for it. Of course this refspec has
bogus syntax (because of the ^{}), and the helper complains,
aborting the push.
Let's have remote-curl mirror what the builtin
get_refs_via_connect does (at least for the case of using
git protocol; we can leave the dumb info/refs reader as it
is).
Signed-off-by: Jeff King <peff@peff.net>
---
remote-curl.c | 7 ++++---
t/t5541-http-push.sh | 14 ++++++++++++++
2 files changed, 18 insertions(+), 3 deletions(-)
diff --git a/remote-curl.c b/remote-curl.c
index 0e720ee..b780ba5 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -188,7 +188,7 @@ static int write_discovery(int in, int out, void *data)
return err;
}
-static struct ref *parse_git_refs(struct discovery *heads)
+static struct ref *parse_git_refs(struct discovery *heads, int for_push)
{
struct ref *list = NULL;
struct async async;
@@ -200,7 +200,8 @@ static struct ref *parse_git_refs(struct discovery *heads)
if (start_async(&async))
die("cannot start thread to parse advertised refs");
- get_remote_heads(async.out, &list, 0, NULL, 0, NULL);
+ get_remote_heads(async.out, &list, 0, NULL,
+ for_push ? REF_NORMAL : 0, NULL);
close(async.out);
if (finish_async(&async))
die("ref parsing thread failed");
@@ -268,7 +269,7 @@ static struct ref *get_refs(int for_push)
heads = discover_refs("git-upload-pack");
if (heads->proto_git)
- return parse_git_refs(heads);
+ return parse_git_refs(heads, for_push);
return parse_info_refs(heads);
}
diff --git a/t/t5541-http-push.sh b/t/t5541-http-push.sh
index a73c826..89232b2 100755
--- a/t/t5541-http-push.sh
+++ b/t/t5541-http-push.sh
@@ -154,5 +154,19 @@ test_expect_success 'push (chunked)' '
test $HEAD = $(git rev-parse --verify HEAD))
'
+test_expect_success 'push --all can push to empty repo' '
+ d=$HTTPD_DOCUMENT_ROOT_PATH/empty-all.git &&
+ git init --bare "$d" &&
+ git --git-dir="$d" config http.receivepack true &&
+ git push --all "$HTTPD_URL"/smart/empty-all.git
+'
+
+test_expect_success 'push --mirror can push to empty repo' '
+ d=$HTTPD_DOCUMENT_ROOT_PATH/empty-mirror.git &&
+ git init --bare "$d" &&
+ git --git-dir="$d" config http.receivepack true &&
+ git push --mirror "$HTTPD_URL"/smart/empty-mirror.git
+'
+
stop_httpd
test_done
--
1.7.7.4.13.g57bf4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] remote-curl: don't pass back fake refs
2011-12-17 10:45 [PATCH] remote-curl: don't pass back fake refs Jeff King
@ 2011-12-19 17:10 ` Jeff King
2011-12-19 19:28 ` Junio C Hamano
0 siblings, 1 reply; 6+ messages in thread
From: Jeff King @ 2011-12-19 17:10 UTC (permalink / raw)
To: git; +Cc: Shawn O. Pearce, Junio C Hamano
On Sat, Dec 17, 2011 at 05:45:39AM -0500, Jeff King wrote:
> Most of the time this bug goes unnoticed, as the fake ref
> won't match our refspecs. However, if "--mirror" is used,
> then we see it as remote cruft to be pruned, and try to pass
> along a deletion refspec for it. Of course this refspec has
> bogus syntax (because of the ^{}), and the helper complains,
> aborting the push.
>
> Let's have remote-curl mirror what the builtin
> get_refs_via_connect does (at least for the case of using
> git protocol; we can leave the dumb info/refs reader as it
> is).
I did some experimenting, and this also fixes another bug: pushing with
--mirror to a smart-http remote that uses alternates.
The fake ".have" refs that the server produces are similarly bogus and
should not be passed back from remote-curl to the parent git process.
Currently they are, so you get:
remote part of refspec is not a valid name in :.have
in the --mirror case.
I had thought this patch wouldn't make a difference there, since
get_remote_heads handles ".have" specifically before the check_refname
call. But it only does so if you pass in a non-NULL extra_have_objects
pointer. We do for regular git (since we care about the .haves for
efficiency, obviously).
But for smart-http, we actually end up parsing the refs twice: once to
get the list of refs to hand back to the parent git process, and then
again later in a send-pack subprocess that actually does care about the
.haves. In the first one, we just pass NULL for extra_have, and
get_remote_heads happily adds the bogus ones to the list.
For the same reason that this patch squelches the bogus "capability^{}",
it also squelches the bogus ".have" refs (but of course they are still
in our buffer to be handed to send-pack, so there is no loss of
efficiency).
Perhaps we should squash in the test below, which demonstrates the
breakage. I also wonder if this is maint-worthy.
-Peff
---
diff --git a/t/t5541-http-push.sh b/t/t5541-http-push.sh
index 89232b2..9b85d42 100755
--- a/t/t5541-http-push.sh
+++ b/t/t5541-http-push.sh
@@ -168,5 +168,23 @@ test_expect_success 'push --mirror can push to empty repo' '
git push --mirror "$HTTPD_URL"/smart/empty-mirror.git
'
+test_expect_success 'push --all to repo with alternates' '
+ s=$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git &&
+ d=$HTTPD_DOCUMENT_ROOT_PATH/alternates-all.git &&
+ git clone --bare --shared "$s" "$d" &&
+ git --git-dir="$d" config http.receivepack true &&
+ git --git-dir="$d" repack -adl &&
+ git push --all "$HTTPD_URL"/smart/alternates-all.git
+'
+
+test_expect_success 'push --mirror to repo with alternates' '
+ s=$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git &&
+ d=$HTTPD_DOCUMENT_ROOT_PATH/alternates-mirror.git &&
+ git clone --bare --shared "$s" "$d" &&
+ git --git-dir="$d" config http.receivepack true &&
+ git --git-dir="$d" repack -adl &&
+ git push --mirror "$HTTPD_URL"/smart/alternates-mirror.git
+'
+
stop_httpd
test_done
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] remote-curl: don't pass back fake refs
2011-12-19 17:10 ` Jeff King
@ 2011-12-19 19:28 ` Junio C Hamano
2011-12-19 21:12 ` Jeff King
0 siblings, 1 reply; 6+ messages in thread
From: Junio C Hamano @ 2011-12-19 19:28 UTC (permalink / raw)
To: Jeff King; +Cc: git, Shawn O. Pearce, Junio C Hamano
Jeff King <peff@peff.net> writes:
> Perhaps we should squash in the test below, which demonstrates the
> breakage. I also wonder if this is maint-worthy.
Thanks for a thorough analysis. I agree that this should go to maint even
more so, as it fixes a case to push to a non-empty repository.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] remote-curl: don't pass back fake refs
2011-12-19 19:28 ` Junio C Hamano
@ 2011-12-19 21:12 ` Jeff King
2011-12-19 21:28 ` Junio C Hamano
0 siblings, 1 reply; 6+ messages in thread
From: Jeff King @ 2011-12-19 21:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Shawn O. Pearce
On Mon, Dec 19, 2011 at 11:28:14AM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > Perhaps we should squash in the test below, which demonstrates the
> > breakage. I also wonder if this is maint-worthy.
>
> Thanks for a thorough analysis. I agree that this should go to maint even
> more so, as it fixes a case to push to a non-empty repository.
Do you want to squash in those tests, or should I re-send with a commit
message more fully explaining the situation?
-Peff
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] remote-curl: don't pass back fake refs
2011-12-19 21:12 ` Jeff King
@ 2011-12-19 21:28 ` Junio C Hamano
2011-12-19 21:29 ` Jeff King
0 siblings, 1 reply; 6+ messages in thread
From: Junio C Hamano @ 2011-12-19 21:28 UTC (permalink / raw)
To: Jeff King; +Cc: git, Shawn O. Pearce
Jeff King <peff@peff.net> writes:
> On Mon, Dec 19, 2011 at 11:28:14AM -0800, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>>
>> > Perhaps we should squash in the test below, which demonstrates the
>> > breakage. I also wonder if this is maint-worthy.
>>
>> Thanks for a thorough analysis. I agree that this should go to maint even
>> more so, as it fixes a case to push to a non-empty repository.
>
> Do you want to squash in those tests, or should I re-send with a commit
> message more fully explaining the situation?
I was lazy and added these three lines at the end:
This also fixes pushing with --mirror to a smart-http remote that uses
alternates. The fake ".have" refs the server gives to avoid unnecessary
network transfer has a similar bad interactions with the machinery.
but it may warrant a more thorough write-up there.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] remote-curl: don't pass back fake refs
2011-12-19 21:28 ` Junio C Hamano
@ 2011-12-19 21:29 ` Jeff King
0 siblings, 0 replies; 6+ messages in thread
From: Jeff King @ 2011-12-19 21:29 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Shawn O. Pearce
On Mon, Dec 19, 2011 at 01:28:09PM -0800, Junio C Hamano wrote:
> > Do you want to squash in those tests, or should I re-send with a commit
> > message more fully explaining the situation?
>
> I was lazy and added these three lines at the end:
>
> This also fixes pushing with --mirror to a smart-http remote that uses
> alternates. The fake ".have" refs the server gives to avoid unnecessary
> network transfer has a similar bad interactions with the machinery.
>
> but it may warrant a more thorough write-up there.
I think that's probably enough. I could restructure the whole text to
talk less about capabilities^{} and more about generically preventing
fake refs, but I really don't think there's much point.
-Peff
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-12-19 21:30 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-12-17 10:45 [PATCH] remote-curl: don't pass back fake refs Jeff King
2011-12-19 17:10 ` Jeff King
2011-12-19 19:28 ` Junio C Hamano
2011-12-19 21:12 ` Jeff King
2011-12-19 21:28 ` Junio C Hamano
2011-12-19 21:29 ` Jeff King
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.