From e172755b1eeb061c8fafb5ae8e4e9e72f68a0cdd Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 13 Mar 2015 00:42:12 -0400 Subject: [PATCH 1/2] upload-pack: fix transfer.hiderefs over smart-http When upload-pack advertises the refs (either for a normal, non-stateless request, or for the initial contact in a stateless one), we call for_each_ref with the send_ref function as its callback. send_ref, in turn, calls mark_our_ref, which checks whether the ref is hidden, and sets OUR_REF or HIDDEN_REF on the object as appropriate. If it is hidden, mark_our_ref also returns "1" to signal send_ref that the ref should not be advertised. If we are not advertising refs, (i.e., the follow-up invocation by an http client to send its "want" lines), we use mark_our_ref directly as a callback to for_each_ref. Its marking does the right thing, but when it then returns "1" to for_each_ref, the latter interprets this as an error and stops iterating. As a result, we skip marking all of the refs that come lexicographically after it. Any "want" lines from the client asking for those objects will fail, as they were not properly marked with OUR_REF. To solve this, we introduce a wrapper callback around mark_our_ref which always returns 0 (even if the ref is hidden, we want to keep iterating). We also tweak the signature of mark_our_ref to exclude unnecessary parameters that were present only to conform to the callback interface. This should make it less likely for somebody to accidentally use it as a callback in the future. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t5551-http-fetch-smart.sh | 11 +++++++++++ upload-pack.c | 14 ++++++++++---- 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh index 6cbc12d9a7..b97077351d 100755 --- a/t/t5551-http-fetch-smart.sh +++ b/t/t5551-http-fetch-smart.sh @@ -213,6 +213,17 @@ test_expect_success 'cookies stored in http.cookiefile when http.savecookies set test_cmp expect_cookies.txt cookies_tail.txt ' +test_expect_success 'transfer.hiderefs works over smart-http' ' + test_commit hidden && + test_commit visible && + git push public HEAD^:refs/heads/a HEAD:refs/heads/b && + git --git-dir="$HTTPD_DOCUMENT_ROOT_PATH/repo.git" \ + config transfer.hiderefs refs/heads/a && + git clone --bare "$HTTPD_URL/smart/repo.git" hidden.git && + test_must_fail git -C hidden.git rev-parse --verify a && + git -C hidden.git rev-parse --verify b +' + test_expect_success EXPENSIVE 'create 50,000 tags in the repo' ' ( cd "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" && diff --git a/upload-pack.c b/upload-pack.c index 433211a238..af2a13acc6 100644 --- a/upload-pack.c +++ b/upload-pack.c @@ -680,7 +680,7 @@ static void receive_needs(void) } /* return non-zero if the ref is hidden, otherwise 0 */ -static int mark_our_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data) +static int mark_our_ref(const char *refname, const unsigned char *sha1) { struct object *o = lookup_unknown_object(sha1); @@ -694,6 +694,12 @@ static int mark_our_ref(const char *refname, const unsigned char *sha1, int flag return 0; } +static int check_ref(const char *refname, const unsigned char *sha1, int flag, void *cb_data) +{ + mark_our_ref(refname, sha1); + return 0; +} + static void format_symref_info(struct strbuf *buf, struct string_list *symref) { struct string_list_item *item; @@ -712,7 +718,7 @@ static int send_ref(const char *refname, const unsigned char *sha1, int flag, vo const char *refname_nons = strip_namespace(refname); unsigned char peeled[20]; - if (mark_our_ref(refname, sha1, flag, NULL)) + if (mark_our_ref(refname, sha1)) return 0; if (capabilities) { @@ -766,8 +772,8 @@ static void upload_pack(void) advertise_shallow_grafts(1); packet_flush(1); } else { - head_ref_namespaced(mark_our_ref, NULL); - for_each_namespaced_ref(mark_our_ref, NULL); + head_ref_namespaced(check_ref, NULL); + for_each_namespaced_ref(check_ref, NULL); } string_list_clear(&symref, 1); if (advertise_refs) From 8ddf3ca74f40bebd79cedba5ba82ca9f2eb33860 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 13 Mar 2015 00:42:25 -0400 Subject: [PATCH 2/2] upload-pack: do not check NULL return of lookup_unknown_object We check whether the return value of lookup_unknown_object is NULL, but some code paths dereference it before our check. This turns out not to be capable of causing a segfault, though. The lookup_unknown_object function will never return NULL, since the whole point is to allocate an object struct if it does not find an existing one. So the code here is not wrong, it is just confusing. Let's just drop the NULL check. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- upload-pack.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/upload-pack.c b/upload-pack.c index af2a13acc6..4bc2dc3607 100644 --- a/upload-pack.c +++ b/upload-pack.c @@ -688,8 +688,6 @@ static int mark_our_ref(const char *refname, const unsigned char *sha1) o->flags |= HIDDEN_REF; return 1; } - if (!o) - die("git upload-pack: cannot find object %s:", sha1_to_hex(sha1)); o->flags |= OUR_REF; return 0; }