Re: [PATCH] fetch-pack: do not take shallow lock unnecessarily

[Jonathan Tan <jonathantanmy@google.com>]

> In other parts of the code we often have an early exit instead of
> setting a variable and reacting to that in the end, i.e. what
> do you think about:
> 
> static void receive_shallow_info(struct fetch_pack_args *args,
>     struct packet_reader *reader)
> {
>      process_section_header(reader, "shallow-info", 0);
> +    if (reader->status == PACKET_READ_FLUSH ||
> +        reader->status == PACKET_READ_DELIM)
> +            /* useful comment why empty sections appear */
> +            return;
>     while (packet_reader_read(reader) == PACKET_READ_NORMAL) {
>     ...
> 
> instead? This would allow us to keep the rest of the function
> relatively simple as well as we'd have a dedicated space where
> we can explain why empty sections need to be treated specially.

Good idea. I'll do something like this in the next version, which will
be combined with another patch of mine into a series [1].

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

> I like this patch..

Thanks.

> > +test_expect_success 'ensure that multiple fetches in same process from a shallow repo works' '
> > +       rm -rf server client trace &&
> > +
> > +       test_create_repo server &&
> > +       test_commit -C server one &&
> > +       test_commit -C server two &&
> > +       test_commit -C server three &&
> > +       git clone --shallow-exclude two "file://$(pwd)/server" client &&
> > +
> > +       git -C server tag -a -m "an annotated tag" twotag two &&
> > +
> > +       # Triggers tag following (thus, 2 fetches in one process)
> > +       GIT_TRACE_PACKET="$(pwd)/trace" git -C client -c protocol.version=2 \
> > +               fetch --shallow-exclude one origin &&
> > +       # Ensure that protocol v2 is used
> > +       grep "fetch< version 2" trace
> > +'
> 
> Would we also need to ensure tags 'one' and 'three',
> but not 'two' are present?
> (What error condition do we see without this patch?)

Well, both "two" and "three" should be present, but not "one". The error
condition is that, previously, this would fail with "fatal: shallow file
has changed since we read it". I'll add a note about this in the commit
message.