Kirill Smelkov <k...@nexedi.com> writes:
> 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.
Thanks. Very much appreciated
>
> 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 ..
> 440858748ae905d48259d4fb67a12a7aa1520cf7 HEAD
> ...
> bc4e9e1fa80662b449805b1ac29fc9b1e4c49187 refs/tags/tag-to-blob
> # <-- NOTE
> 038f48ad0beaffbea71d186a05084b79e3870cbf
> refs/tags/tag-to-blob^{}
> 520db1f5e1afeaa12b1a8d73ce82db72ca036ee1 refs/tags/tag-to-tree
> # <-- NOTE
> 7395c100223b7cd760f58ccfa0d3f3d2dd539bb6
> refs/tags/tag-to-tree^{}
> .../git/t/trash directory.t5500-fetch-pack/fetchall$ git fetch-pack
> --all ..
> fatal: A git upload-pack: not our ref
> 038f48ad0beaffbea71d186a05084b79e3870cbf
> fatal: The remote end hung up unexpectedly
... except for this bit. I am not sure readers understand what
these two overlong lines want to say. Would it be easier to
understand if you wrote the above like this?
.../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
Instead of marking the tag-to-blob and tag-to-tree entries (which
are not where the 'fatal' breakage comes from), I think it makes
more sense to mark the entries that show peeled version (which also
matches the object name in the error message), and by shortening the
line, readers can see more easily which lines are highlighted.
> +test_expect_success 'test --all wrt tag to non-commits' '
> + 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 <aut...@example.com> 0 +0000\n\nhello tag" | git
> mktag) &&
> + git tag -a -m "tag -> tag" tag-to-tag $tag &&
All of the above, while may not be techincallly wrong per-se, look
unnecessarily complex.
I guess the reason why you do the above is because you do not want
to use any object that is reachable via other existing refs and
instead want to ensure these objects are reachable only via the tags
you are creating in this test. Otherwise using HEAD~4:test.txt and
HEAD^{tree} instead of $blob and $tree constructed via hash-object
and mktree would be sufficient and more readable. Oh well.
> + 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 &&
> (
