From e9502c0a7fdb5649e3a3aece4e0d54ddb22983df Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 11 Jun 2018 01:53:57 -0400 Subject: 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 Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t5500-fetch-pack.sh | 10 ++++++++++ 1 file changed, 10 insertions(+) (limited to 't') diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh index 505e1b4a7f..f20bb59d22 100755 --- 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 && ( -- cgit v1.2.3 From c12c9df5275add49b7b47cad82879a84c6bea469 Mon Sep 17 00:00:00 2001 From: Kirill Smelkov Date: Wed, 13 Jun 2018 12:28:21 +0300 Subject: fetch-pack: test explicitly that --all can fetch tag references pointing to non-commits Fetch-pack --all became broken with respect to unusual tags in 5f0fc64513 (fetch-pack: eliminate spurious error messages, 2012-09-09), and was fixed only recently in e9502c0a7f (fetch-pack: don't try to fetch peel values with --all, 2018-06-11). However the test added in e9502c0a7f does not explicitly cover all funky cases. In order to be sure fetching funky tags will never break, let's explicitly test all relevant cases with 4 tag objects pointing to 1) a blob, 2) a tree, 3) a commit, and 4) another tag objects. The referenced tag objects themselves are referenced from under regular refs/tags/* namespace. Before e9502c0a7f `fetch-pack --all` was failing e.g. this way: .../git/t/trash directory.t5500-fetch-pack/fetchall$ git ls-remote .. 44085874... HEAD ... bc4e9e1f... refs/tags/tag-to-blob 038f48ad... refs/tags/tag-to-blob^{} # peeled 520db1f5... refs/tags/tag-to-tree 7395c100... refs/tags/tag-to-tree^{} # peeled .../git/t/trash directory.t5500-fetch-pack/fetchall$ git fetch-pack --all .. fatal: A git upload-pack: not our ref 038f48ad... fatal: The remote end hung up unexpectedly Helped-by: Junio C Hamano Signed-off-by: Kirill Smelkov Signed-off-by: Junio C Hamano --- t/t5500-fetch-pack.sh | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) (limited to 't') diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh index f20bb59d22..5879ee44d7 100755 --- a/t/t5500-fetch-pack.sh +++ b/t/t5500-fetch-pack.sh @@ -528,6 +528,37 @@ test_expect_success 'test --all with tag to non-tip' ' ) ' +test_expect_success 'test --all wrt tag to non-commits' ' + # create tag-to-{blob,tree,commit,tag}, making sure all tagged objects + # are reachable only via created tag references. + blob=$(echo "hello blob" | git hash-object -t blob -w --stdin) && + git tag -a -m "tag -> blob" tag-to-blob $blob && + \ + tree=$(printf "100644 blob $blob\tfile" | git mktree) && + git tag -a -m "tag -> tree" tag-to-tree $tree && + \ + tree2=$(printf "100644 blob $blob\tfile2" | git mktree) && + commit=$(git commit-tree -m "hello commit" $tree) && + git tag -a -m "tag -> commit" tag-to-commit $commit && + \ + blob2=$(echo "hello blob2" | git hash-object -t blob -w --stdin) && + tag=$(printf "object $blob2\ntype blob\ntag tag-to-blob2\n\ +tagger author A U Thor 0 +0000\n\nhello tag" | git mktag) && + git tag -a -m "tag -> tag" tag-to-tag $tag && + \ + # `fetch-pack --all` should succeed fetching all those objects. + mkdir fetchall && + ( + cd fetchall && + git init && + git fetch-pack --all .. && + git cat-file blob $blob >/dev/null && + git cat-file tree $tree >/dev/null && + git cat-file commit $commit >/dev/null && + git cat-file tag $tag >/dev/null + ) +' + test_expect_success 'shallow fetch with tags does not break the repository' ' mkdir repo1 && ( -- cgit v1.2.3