fetch-pack: don't try to fetch peel values with --all

When "fetch-pack --all" sees a tag-to-blob on the remote, it
tries to fetch both the tag itself ("refs/tags/foo") and the
peeled value that the remote advertises ("refs/tags/foo^{}").
Asking for the object pointed to by the latter can cause
upload-pack to complain with "not our ref", since it does
not mark the peeled objects with the OUR_REF (unless they
were at the tip of some other ref).

Arguably upload-pack _should_ be marking those peeled
objects. But it never has in the past, since clients would
generally just ask for the tag and expect to get the peeled
value along with it. And that's how "git fetch" works, as
well as older versions of "fetch-pack --all".

The problem was introduced by 5f0fc64513 (fetch-pack:
eliminate spurious error messages, 2012-09-09). Before then,
the matching logic was something like:

if (refname is ill-formed)
do nothing
else if (doing --all)
always consider it matched
else
look through list of sought refs for a match

That commit wanted to flip the order of the second two arms
of that conditional. But we ended up with:

if (refname is ill-formed)
do nothing
else
look through list of sought refs for a match

if (--all and no match so far)
always consider it matched

That means tha an ill-formed ref will trigger the --all
conditional block, even though we should just be ignoring
it. We can fix that by having a single "else" with all of
the well-formed logic, that checks the sought refs and
"--all" in the correct order.

Reported-by: Kirill Smelkov <kirr@nexedi.com>
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 601f0779a1903a2b860412e3404e4ea8e4bce573..c85fc6aeed7409c05a2f299105eb3d68b2471ccf 100644 (file)
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -582,11 +582,11 @@ static void filter_refs(struct fetch_pack_args *args,
                                }
                                i++;
                        }
-               }
 
-               if (!keep && args->fetch_all &&
-                   (!args->deepen || !starts_with(ref->name, "refs/tags/")))
-                       keep = 1;
+                       if (!keep && args->fetch_all &&
+                           (!args->deepen || !starts_with(ref->name, "refs/tags/")))
+                               keep = 1;
+               }
 
                if (keep) {
                        *newtail = ref;

diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 505e1b4a7f421d377aa3798e94c90c14249bce91..f20bb59d22e9a9143e1c1bb3125677b33306d583 100755 (executable)
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -518,6 +518,16 @@ test_expect_success 'test --all, --depth, and explicit tag' '
        ) >out-adt 2>error-adt
 '
 
+test_expect_success 'test --all with tag to non-tip' '
+       git commit --allow-empty -m non-tip &&
+       git commit --allow-empty -m tip &&
+       git tag -m "annotated" non-tip HEAD^ &&
+       (
+               cd client &&
+               git fetch-pack --all ..
+       )
+'
+
 test_expect_success 'shallow fetch with tags does not break the repository' '
        mkdir repo1 &&
        (