fetch: do not consider peeled tags as advertised tips

Our filter_refs() function accidentally considers the target of a peeled
tag to be advertised by the server, even though upload-pack on the
server side does not consider it so. This can result in the client
making a bogus fetch to the server, which will end with the server
complaining "not our ref". Whereas the correct behavior is for the
client to notice that the server will not allow the request and error
out immediately.

So as bugs go, this is not very serious (the outcome is the same either
way -- the fetch fails). But it's worth making the logic here correct
and consistent with other related cases (e.g., fetching an oid that the
server did not mention at all).

The crux of the issue comes from fdb69d33c4 (fetch-pack: always allow
fetching of literal SHA1s, 2017-05-15). After that, the strategy of
filter_refs() is basically:

- for each advertised ref, try to match it with a "sought" ref
provided by the user. Skip any malformed refs (which includes
peeled values like "refs/tags/foo^{}"), and place any unmatched
items onto the unmatched list.

- if there are unmatched sought refs, then put all of the advertised
tips into an oidset, including the unmatched ones.

- for each sought ref, see if it's in the oidset, in which case it's
legal for us to ask the server for it

The problem is in the second step. Our list of unmatched refs includes
the peeled refs, even though upload-pack does not allow them to be
directly fetched. So the simplest fix would be to exclude them during
that step.

However, we can observe that the unmatched list isn't used for anything
else, and is freed at the end. We can just free those malformed refs
immediately. That saves us having to check each ref a second time to see
if it's malformed.

Note that this code only kicks in when "strict" is in effect. I.e., if
we are using the v0 protocol and uploadpack.allowReachableSHA1InWant is
not in effect. With v2, all oids are allowed, and we do not bother
creating or consulting the oidset at all. To future-proof our test
against the upcoming GIT_TEST_PROTOCOL_VERSION flag, we'll manually mark
it as a v0-only test.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/fetch-pack.c b/fetch-pack.c
index a181d3401d1e46e641a0212e09b6b1f056019bf0..bb8eac81263edbee65f10e8b6bfdfa8a40813a09 100644 (file)
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -573,9 +573,14 @@ static void filter_refs(struct fetch_pack_args *args,
                next = ref->next;
 
                if (starts_with(ref->name, "refs/") &&
-                   check_refname_format(ref->name, 0))
-                       ; /* trash */
-               else {
+                   check_refname_format(ref->name, 0)) {
+                       /*
+                        * trash or a peeled value; do not even add it to
+                        * unmatched list
+                        */
+                       free_one_ref(ref);
+                       continue;
+               } else {
                        while (i < nr_sought) {
                                int cmp = strcmp(ref->name, sought[i]->name);
                                if (cmp < 0)

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 98ef71b48c9b70a1cdf72b571e4bee8eea3e5723..4f065212b85c437d1669767bca50795490f8a2c7 100755 (executable)
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1273,6 +1273,17 @@ test_expect_success 'fetch follows tags by default' '
        test_cmp expect actual
 '
 
+test_expect_success 'peeled advertisements are not considered ref tips' '
+       mk_empty testrepo &&
+       git -C testrepo commit --allow-empty -m one &&
+       git -C testrepo commit --allow-empty -m two &&
+       git -C testrepo tag -m foo mytag HEAD^ &&
+       oid=$(git -C testrepo rev-parse mytag^{commit}) &&
+       test_must_fail env GIT_TEST_PROTOCOL_VERSION= \
+               git fetch testrepo $oid 2>err &&
+       test_i18ngrep "Server does not allow request for unadvertised object" err
+'
+
 test_expect_success 'pushing a specific ref applies remote.$name.push as refmap' '
        mk_test testrepo heads/master &&
        rm -fr src dst &&