From: Junio C Hamano <gitster@pobox•com>
To: Jeff King <peff@peff•net>
Cc: git@vger•kernel.org
Subject: Re: [PATCH 1/7] upload-pack: fix transfer.hiderefs over smart-http
Date: Thu, 12 Mar 2015 23:21:09 -0700 [thread overview]
Message-ID: <xmqqioe5zacq.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <20150313044212.GA18532@peff.net> (Jeff King's message of "Fri, 13 Mar 2015 00:42:12 -0400")
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)
next prev parent reply other threads:[~2015-03-13 6:21 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-03-13 4:41 [PATCH 0/7] fix transfer.hiderefs with smart http Jeff King
2015-03-13 4:42 ` [PATCH 1/7] upload-pack: fix transfer.hiderefs over smart-http Jeff King
2015-03-13 6:21 ` Junio C Hamano [this message]
2015-03-13 4:42 ` [PATCH 2/7] upload-pack: do not check NULL return of lookup_unknown_object Jeff King
2015-03-13 4:48 ` [PATCH 3/7] t: translate SIGINT to an exit Jeff King
2015-03-13 4:50 ` [PATCH 4/7] t: redirect stderr GIT_TRACE to descriptor 4 Jeff King
2015-03-13 4:51 ` [PATCH 5/7] t: pass GIT_TRACE through Apache Jeff King
2015-03-13 4:53 ` [PATCH 6/7] t5541: move run_with_cmdline_limit to test-lib.sh Jeff King
2015-03-13 6:45 ` Eric Sunshine
2015-03-13 4:57 ` [PATCH 7/7] t5551: make EXPENSIVE test cheaper Jeff King
2015-03-13 4:59 ` [PATCH 0/7] fix transfer.hiderefs with smart http Jeff King
2015-03-13 5:21 ` Duy Nguyen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqioe5zacq.fsf@gitster.dls.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=peff@peff$(echo .)net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox