From: Brandon Williams <bmwill@google.com>
To: Jonathan Tan <jonathantanmy@google.com>
Cc: git@vger.kernel.org, peff@peff.net, gitster@pobox.com
Subject: Re: [PATCH] fetch-pack: unify ref in and out param
Date: Wed, 1 Aug 2018 14:38:26 -0700 [thread overview]
Message-ID: <20180801213826.GA66237@google.com> (raw)
In-Reply-To: <20180801201320.201133-1-jonathantanmy@google.com>
On 08/01, Jonathan Tan wrote:
> When a user fetches:
> - at least one up-to-date ref and at least one non-up-to-date ref,
> - using HTTP with protocol v0 (or something else that uses the fetch
> command of a remote helper)
> some refs might not be updated after the fetch.
>
> This bug was introduced in commit 989b8c4452 ("fetch-pack: put shallow
> info in output parameter", 2018-06-28) which allowed transports to
> report the refs that they have fetched in a new out-parameter
> "fetched_refs". If they do so, transport_fetch_refs() makes this
> information available to its caller.
>
> Users of "fetched_refs" rely on the following 3 properties:
> (1) it is the complete list of refs that was passed to
> transport_fetch_refs(),
> (2) it has shallow information (REF_STATUS_REJECT_SHALLOW set if
> relevant), and
> (3) it has updated OIDs if ref-in-want was used (introduced after
> 989b8c4452).
>
> In an effort to satisfy (1), whenever transport_fetch_refs()
> filters the refs sent to the transport, it re-adds the filtered refs to
> whatever the transport supplies before returning it to the user.
> However, the implementation in 989b8c4452 unconditionally re-adds the
> filtered refs without checking if the transport refrained from reporting
> anything in "fetched_refs" (which it is allowed to do), resulting in an
> incomplete list, no longer satisfying (1).
>
> An earlier effort to resolve this [1] solved the issue by readding the
> filtered refs only if the transport did not refrain from reporting in
> "fetched_refs", but after further discussion, it seems that the better
> solution is to revert the API change that introduced "fetched_refs".
> This API change was first suggested as part of a ref-in-want
> implementation that allowed for ref patterns and, thus, there could be
> drastic differences between the input refs and the refs actually fetched
> [2]; we eventually decided to only allow exact ref names, but this API
> change remained even though its necessity was decreased.
>
> Therefore, revert this API change by reverting commit 989b8c4452, and
> make receive_wanted_refs() update the OIDs in the sought array (like how
> update_shallow() updates shallow information in the sought array)
> instead. A test is also included to show that the user-visible bug
> discussed at the beginning of this commit message no longer exists.
>
> [1] https://public-inbox.org/git/20180801171806.GA122458@google.com/
> [2] https://public-inbox.org/git/86a128c5fb710a41791e7183207c4d64889f9307.1485381677.git.jonathantanmy@google.com/
>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
> I now think that it's better to revert the API change introducing
> "fetched_refs" (or as Peff describes it, "this whole 'return the fetched
> refs' scheme from 989b8c4452"), so here is a patch doing so. I hope to
> have covered all of Peff's and Junio's questions in the commit message.
>
> As for Brandon's question:
>
> > I haven't thought too much about what we would need to do in the event
> > we add patterns to ref-in-want, but couldn't we possible mutate the
> > input list again in this case and just simply add the resulting refs to
> > the input list?
>
> If we support ref patterns, we would need to support deletion of refs,
> not just addition (because a ref might have existed in the initial ref
> advertisement, but not when the packfile is delivered). But it should
> be possible to add a flag stating "don't use this" to the ref, and
> document that transport_fetch_refs() can append additional refs to the
> tail of the input list. Upon hindsight, maybe this should have been the
> original API change instead of the "fetched_refs" mechanism.
Thanks for getting this out, it looks good to me. If we end up adding
patterns to ref-in-want then we can explore what changes would need to
be made then, I expect we may need to do a bit more work on the whole
fetching stack to get what we'd want in that case (because we would want
to avoid this issue again).
--
Brandon Williams
next prev parent reply other threads:[~2018-08-01 21:38 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-07-29 12:19 [BUG] fetching sometimes doesn't update refs Jeff King
2018-07-30 17:53 ` Brandon Williams
2018-07-30 22:56 ` [PATCH] transport: report refs only if transport does Jonathan Tan
2018-07-31 19:24 ` Jeff King
2018-07-31 21:38 ` Junio C Hamano
2018-07-31 23:29 ` Jonathan Tan
2018-07-31 23:23 ` Jonathan Tan
2018-08-01 17:18 ` Brandon Williams
2018-08-02 16:30 ` Jeff King
2018-08-01 20:13 ` [PATCH] fetch-pack: unify ref in and out param Jonathan Tan
2018-08-01 21:38 ` Brandon Williams [this message]
2018-08-01 22:23 ` Junio C Hamano
2018-08-02 16:40 ` Jeff King
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=20180801213826.GA66237@google.com \
--to=bmwill@google.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=jonathantanmy@google.com \
--cc=peff@peff.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
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;
as well as URLs for NNTP newsgroup(s).