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

From: Jonathan Tan <jonathantanmy@google.com>
Date: Mar 26, 2019, 17:37 UTC
Message-ID: <20190326173706.175638-1-jonathantanmy@google.com>
In-Reply-To: <20190326052011.GB1933@sigill.intra.peff.net>

Show 16 quoted lines

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

That's right. In v2, it comes in "shallow-info", which happens right before the server sends the packfile.

Show 7 quoted lines

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

Conveying it in the ls-refs would work.

Show 11 quoted lines

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

Thanks.

Show 18 quoted lines

> The patch looks reasonable to me, though I am far from an expert on the
> shallow bits of the protocol. One thing I did notice:
> 
> >  static void receive_shallow_info(struct fetch_pack_args *args,
> > -				 struct packet_reader *reader)
> > +				 struct packet_reader *reader,
> > +				 struct shallow_info *si)
> >  {
> > -	int line_received = 0;
> > +	struct oid_array *shallows;
> > +	int unshallow_received = 0;
> > +
> > +	shallows = xcalloc(1, sizeof(*shallows));
> 
> This has to be heap-allocated, since we pass off ownership to "si"
> (sometimes). But in the v0 case, it comes from the transport's
> &data->shallows of a local variable in cmd_fetch_pack(), and we never
> free it. So I think this oid_array ends up getting leaked.

Thanks for the catch.

> Perhaps it's worth passing down the shallows array we get from the
> caller of fetch_pack(). Something like the patch below (I think it is
> never NULL, which means in your patch 1 you can simplify the conditional
> for the BUG).

[snip patch]

You're right that it is never NULL - I have removed that check. As for passing down the shallows array that we get from the caller of fetch_pack(), that would get confusing because we end up modifying the shallows array in some code paths, and the transport is sometimes reused (for example, when backfilling tags). I have instead made a shallows_scratch variable in fetch_pack(), and made it pass it down (like in the diff you provided).