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

Message

Jonathan Tan Sept. 27, 2018, 7:24 p.m. UTC

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/[e-mail]/

Rearranging Junio's questions:

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

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

> 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(-)

Comments

Junio C Hamano Sept. 27, 2018, 8:53 p.m. UTC | #1

Jonathan Tan <[e-mail]> writes:

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

Yeah, taking it out of context did make its purpose fuzzy.  Without
the other patches in the series that set the overall direction of
reducing ls-refs, it was unclear why some transports are marked to
specifically want to automatically call transport_get_remote_refs().
With these surrounding changes, "we call it as needed, because an
earlier step may not have called it" starts to sound quite sensible.