Re: [PATCH 1/7] upload-pack: fix transfer.hiderefs over smart-http

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> 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.

Nicely described in a way that the reason of the breakage and the
fix is obvious to those who already know what the codepath is
supposed to be doing.

> 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.

I especially love this kind of future-proofing ;-).

Thanks, will queue.

> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  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 6cbc12d..b970773 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 b531a32..c8e8713 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -681,7 +681,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);
>  
> @@ -695,6 +695,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;
> @@ -713,7 +719,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) {
> @@ -767,8 +773,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)