On 02/23, Jonathan Tan wrote:
> On Tue, 6 Feb 2018 17:12:58 -0800
> Brandon Williams <bmwill@xxxxxxxxxx> 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.
Thanks, I'll remove that.
>
> > + /* 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.)
I actually caught this Friday morning when I realized that fetching from
a referenced repository would replicated objects instead of using them
from the referenced repository. Thanks for pointing this out :)
--
Brandon Williams
