Re: [PATCH] fetch-pack: test explicitly that --all can fetch tag references pointing to non-commits

> Kirill Smelkov <kirr@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 <author@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.