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

From: Jeff King <peff@peff.net>
Date: Jun 13, 2018, 21:05 UTC
Message-ID: <20180613210509.GA15574@sigill.intra.peff.net>
In-Reply-To: <20180613184301.GB22854@deco.navytux.spb.ru>

On Wed, Jun 13, 2018 at 06:43:04PM +0000, Kirill Smelkov wrote:

Show 28 quoted lines

> From: Kirill Smelkov <kirr@nexedi.com>
> Date: Wed, 13 Jun 2018 12:28:21 +0300
> Subject: [PATCH v2] 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

TBH, I do not find this snippet all that compelling. We know that e9502c0a7f already fixed the bug, and that it had nothing to do with non-commits at all.

The primary reason to add these tests is that in general we do not cover fetch-pack over tags to non-commits. And I think the reason to use otherwise unreferenced objects is that it they are more likely to have detectable symptoms if they tickle a bug.

So why don't we say that, instead of re-hashing output from the earlier fix?

-Peff