Re: [PATCH 2/2] fetch-pack: respect --no-update-shallow in v2

[Duy Nguyen <pclouds@gmail.com>]

On Tue, Mar 26, 2019 at 12:20 PM Jeff King <peff@peff.net> wrote:
>
> On Mon, Mar 25, 2019 at 01:43:23PM -0700, Jonathan Tan wrote:
>
> > In protocol v0, when sending "shallow" lines, the server distinguishes
> > between lines caused by the remote repo being shallow and lines caused
> > by client-specified depth settings. Unless "--update-shallow" is
> > specified, there is a difference in behavior: refs that reach the former
> > "shallow" lines, but not the latter, are rejected. But in v2, the server
> > does not, and the client treats all "shallow" lines like lines caused by
> > client-specified depth settings.
> >
> > Full restoration of v0 functionality is not possible without protocol
> > change,
>
> That's rather unfortunate. Is this because the v2 ls-refs phase is
> separate, and that's when a v0 server would tell us about its shallows?
> It looks like in v2 it comes in a separate "shallow-info" section.
>
> What would the protocol change look like?  Would we just need a
> capability to instruct the server to mark the two different types of
> shallow distinctly? Or do we actually need to convey the information
> separately (e.g., in the ls-refs phase)?
>
> None of that matters for your patch here, but I'm just wondering what
> the path forward is.

If it helps (because I'm still catching up with v2 to actually help
review), this case is for cloning from a shallow repo. The commit that
outlines how .git/shallow is updated is 58babfffde (shallow.c: the 8
steps to select new commits for .git/shallow, 2013-12-05).

Since the first shallow info is about the shape of the remote repo
(where refs are the tips), ls-refs sounds like the right place to
include the information. In other words, ls-refs currently tells the
tip/top of the repo, what's missing is the piece about "the bottom"
(shallow cut points).

> > but we can implement a heuristic: if we specify any depth
> > setting, treat all "shallow" lines like lines caused by client-specified
> > depth settings (that is, unaffected by "--no-update-shallow"), but
> > otherwise, treat them like lines caused by the remote repo being shallow
> > (that is, affected by "--no-update-shallow"). This restores most of v0
> > behavior, except in the case where a client fetches from a shallow
> > repository with depth settings.
>
> That seems like the best we can do without the protocol change. And even
> if we adjust the protocol, we need some fallback behavior for existing
> v2 servers, so this is worth doing.

Are people actually doing this (i.e. cloning from or  pushing to a
shallow repo)? I added this with the intention that a big shallow repo
(e.g. one year long history) is served as the common source to reduce
server loads and everything, while the full/big repo is available but
rarely needed. I never saw anyone complain about it (so, likely not
using it).

The description of --update-shallow probably should mention this
fallback behavior? --update-shallow was not default because I feared
the local repo could be cut short by unsolicited shallow requests from
the server side, and it looks like --update-shallow is default (by
mistake) in v2? Maybe I worried for nothing. I dunno.
-- 
Duy