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