On Tue, 6 Feb 2018 17:12:58 -0800
Brandon Williams <bmw...@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.)
