[RFC PATCH v2 0/4] Avoid ls-refs when possible in protocol v2

From: Jonathan Tan <jonathantanmy@google.com>
Date: Sep 27, 2018, 19:24 UTC
Message-ID: <cover.1538075680.git.jonathantanmy@google.com>
In-Reply-To: <20180925225355.74237-1-jonathantanmy@google.com>

To answer Junio's questions in [1], I think it's best to include the full patch set that I'm developing, so here it is. The original patch is now patch 3 of this set.

[1] https://public-inbox.org/git/xmqq8t3pnphe.fsf@gitster-ct.c.googlers.com/

Rearranging Junio's questions:

Show 5 quoted lines

> ... ah, do you mean that this is not a new feature, but is a bugfix
> for some callers that are not calling get-remote-refs before calling
> fetch-refs, and the bit is to work around the fact that some
> transport not just can function without get-remote-refs first but do
> not want to call it?

Yes, it is the bugfix you describe, except that the bug coincidentally does not cause any bad behavior. fetch-object.c indeed does not call get-remote-refs before fetch-refs, but it calls transport_set_option(), which so happens to do what we need (call set_helper_option()).

However, we need it now, because ...

Show 9 quoted lines

> But this I do not quite understand.  It looks saying "when asked to
> fetch, if the transport does not allow us to do so without first
> getting the advertisement, lazily do that", and that may be a good
> thing to do, but then aren't the current set of callers already
> calling transport-get-remote-refs elsewhere before they call
> transport-fetch-refs?  IOW, I would have expected to see a matching
> removal, or at least a code that turns an unconditional call to
> get-remote-refs to a conditional one that is done only for the
> transport that lacks the capability, or something along that line.

... this "matching removal" you are talking about is in the subsequent patch 4. And there is no transport_set_option() to save us this time, so we really do need this bugfix.

Show 11 quoted lines

> IOW, I am a bit confused by this comment (copied from an earlier part)
> 
> > +	/**
> > +	 * This transport supports the fetch() function being called
> > +	 * without get_refs_list() first being called.
> > +	 */
> 
> Shouldn't it read more like "this transport does not want its
> get-refs-list called when fetch-refs is done"?
> 
> I dunno.

I'm not sure I understand - transports generally don't care if get-refs-list is called after fetch-refs. Also, this already happens when fetching with tag following from a server that does not support tag following, using a transport that supports reuse.

Jonathan Tan (4):
  transport: allow skipping of ref listing
  transport: do not list refs if possible
  transport: list refs before fetch if necessary
  fetch: do not list refs if fetching only hashes

 builtin/fetch.c             | 32 +++++++++++++++++-----
 fetch-pack.c                |  2 +-
 t/t5551-http-fetch-smart.sh | 15 +++++++++++
 t/t5702-protocol-v2.sh      | 18 +++++++++++++
 transport-helper.c          |  1 +
 transport-internal.h        |  6 +++++
 transport.c                 | 54 ++++++++++++++++++++++++++++++++-----
 7 files changed, 115 insertions(+), 13 deletions(-)

-- 
2.19.0.605.g01d371f741-goog