Re: [PATCH v3 21/35] fetch-pack: perform a fetch using v2

> On Tue,  6 Feb 2018 17:12:58 -0800
> Brandon Williams <bmwill@google.com> wrote:
> 
> > +	while ((oid = get_rev())) {
> > +		packet_buf_write(req_buf, "have %s\n", oid_to_hex(oid));
> > +		if (++haves_added >= INITIAL_FLUSH)
> > +			break;
> > +	};
> 
> Unnecessary semicolon after closing brace.

> 
> > +			/* Filter 'ref' by 'sought' and those that aren't local */
> > +			if (everything_local(args, &ref, sought, nr_sought))
> > +				state = FETCH_DONE;
> > +			else
> > +				state = FETCH_SEND_REQUEST;
> > +			break;
> 
> I haven't looked at this patch in detail, but I found a bug that can be
> reproduced if you patch the following onto this patch:
> 
>     --- a/t/t5702-protocol-v2.sh
>     +++ b/t/t5702-protocol-v2.sh
>     @@ -124,6 +124,7 @@ test_expect_success 'clone with file:// using protocol v2' '
>      
>      test_expect_success 'fetch with file:// using protocol v2' '
>             test_commit -C file_parent two &&
>     +       git -C file_parent tag -d one &&
>      
>             GIT_TRACE_PACKET=1 git -C file_child -c protocol.version=2 \
>                     fetch origin 2>log &&
>     @@ -133,7 +134,8 @@ test_expect_success 'fetch with file:// using protocol v2' '
>             test_cmp expect actual &&
>      
>             # Server responded using protocol v2
>     -       grep "fetch< version 2" log
>     +       grep "fetch< version 2" log &&
>     +       grep "have " log
>      '
> 
> Merely including the second hunk (the one with 'grep "have "') does not
> make the test fail, but including both the first and second hunks does.
> That is, fetch v2 emits "have" only for remote refs that point to
> objects we already have, not for local refs.
> 
> Everything still appears to work, except that packfiles are usually much
> larger than they need to be.
> 
> I did some digging in the code and found out that the equivalent of
> find_common() (which calls `for_each_ref(rev_list_insert_ref_oid,
> NULL)`) was not called in v2. In v1, find_common() is called immediately
> after everything_local(), but there is no equivalent in v2. (I quoted
> the invocation of everything_local() in v2 above.)