patchfetch-pack: demonstrate --all breakage when remote have tags to non-commit objects

> Doubly interesting, it looks like this case _used_ to work, but was
> broken by 5f0fc64513 (fetch-pack: eliminate spurious error messages,
> 2012-09-09). Which only changed the fetch-pack side. It moved the
> handling of --all so that it was no longer in the "else" for
> check_refname_format(). I guess the original code was rejecting those
> peeled bits as "not a ref" (which makes sense).
> 
> So that seems like a bug in fetch-pack. But I'm still not convinced that
> upload-pack doesn't also have a bug.

> Subject: fetch-pack: don't try to fetch peeled values with --all
> [...]
> Original report and test from Kirill Smelkov.
>
> Signed-off-by: Kirill Smelkov <kirr@nexedi.com>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
> @@ -506,30 +506,45 @@ test_expect_success 'test missing ref before existing' '
> +test_expect_success 'test --all wrt tag to non-commits' '
> +       blob_sha1=$(echo "hello blob" | git hash-object -t blob -w --stdin) &&
> +       git tag -a -m "tag -> blob" tag-to-blob $blob_sha1 &&
> +       tree_sha1=$(printf "100644 blob $blob_sha1\tfile\n" | git mktree) &&

> +       git tag -a -m "tag -> tree" tag-to-tree $tree_sha1 &&
> +       mkdir fetchall &&
> +       (
> +               cd fetchall &&
> +               git init &&
> +               git fetch-pack --all .. &&

> On Mon, Jun 11, 2018 at 12:47 AM, Jeff King <peff@peff.net> wrote:
> > Subject: fetch-pack: don't try to fetch peeled values with --all
> > [...]
> > Original report and test from Kirill Smelkov.
> >
> > Signed-off-by: Kirill Smelkov <kirr@nexedi.com>
> > Signed-off-by: Jeff King <peff@peff.net>
> > ---
> > diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
> > @@ -506,30 +506,45 @@ test_expect_success 'test missing ref before existing' '
> > +test_expect_success 'test --all wrt tag to non-commits' '
> > +       blob_sha1=$(echo "hello blob" | git hash-object -t blob -w --stdin) &&
> > +       git tag -a -m "tag -> blob" tag-to-blob $blob_sha1 &&
> > +       tree_sha1=$(printf "100644 blob $blob_sha1\tfile\n" | git mktree) &&
> 
> Perhaps modernize these names to 'blob_oid' and 'tree_oid', or even
> simpler, just 'blob' and 'tree'.

> > +       git tag -a -m "tag -> tree" tag-to-tree $tree_sha1 &&
> > +       mkdir fetchall &&
> > +       (
> > +               cd fetchall &&
> > +               git init &&
> > +               git fetch-pack --all .. &&
> 
> Simpler:
> 
>     git init fetchall &&
>     (
>         cd fetchall &&
>         git fetch-pack --all .. &&
> 
> Although, I see that this script already has a mix of the two styles
> (simpler and not-so-simple), so...

> On Mon, Jun 11, 2018 at 01:28:23AM -0400, Eric Sunshine wrote:
> 
> > On Mon, Jun 11, 2018 at 12:47 AM, Jeff King <peff@peff.net> wrote:
> > > Subject: fetch-pack: don't try to fetch peeled values with --all
> > > [...]
> > > Original report and test from Kirill Smelkov.
> > >
> > > Signed-off-by: Kirill Smelkov <kirr@nexedi.com>
> > > Signed-off-by: Jeff King <peff@peff.net>
> > > ---
> > > diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
> > > @@ -506,30 +506,45 @@ test_expect_success 'test missing ref before existing' '
> > > +test_expect_success 'test --all wrt tag to non-commits' '
> > > +       blob_sha1=$(echo "hello blob" | git hash-object -t blob -w --stdin) &&
> > > +       git tag -a -m "tag -> blob" tag-to-blob $blob_sha1 &&
> > > +       tree_sha1=$(printf "100644 blob $blob_sha1\tfile\n" | git mktree) &&
> > 
> > Perhaps modernize these names to 'blob_oid' and 'tree_oid', or even
> > simpler, just 'blob' and 'tree'.
> 
> Looking deeper, we do not need these trees and blobs at all. The problem
> is really just a tag that peels to an object that is not otherwise a ref
> tip, regardless of its type.

> So below is a patch that simplifies the test even further (the actual
> code change is the same).
> 
> > > +       git tag -a -m "tag -> tree" tag-to-tree $tree_sha1 &&
> > > +       mkdir fetchall &&
> > > +       (
> > > +               cd fetchall &&
> > > +               git init &&
> > > +               git fetch-pack --all .. &&
> > 
> > Simpler:
> > 
> >     git init fetchall &&
> >     (
> >         cd fetchall &&
> >         git fetch-pack --all .. &&
> > 
> > Although, I see that this script already has a mix of the two styles
> > (simpler and not-so-simple), so...
> 
> The nearby tests actually reuse the "client" directory. We can do that,
> too, if we simply create new objects for our test, to make sure they
> still need fetching. See below (we could also use "git -C" there, but
> the subshell matches the other tests).
> 
> -- >8 --
> 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 <kirr@nexedi.com>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  fetch-pack.c          |  8 ++++----
>  t/t5500-fetch-pack.sh | 10 ++++++++++
>  2 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/fetch-pack.c b/fetch-pack.c
> index a320ce9872..cc7a42fee9 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -657,11 +657,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 d4f435155f..5726f83ea3 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 &&
>  	(
> -- 
> 2.18.0.rc1.446.g4486251e51

> > Looking deeper, we do not need these trees and blobs at all. The problem
> > is really just a tag that peels to an object that is not otherwise a ref
> > tip, regardless of its type.
> 
> Thanks for feedback and for coming up with the fix. Sure, I'm ok with
> moving the test into your patch. However, even if a test becomes
> different - narrowing down root of _current_ problem, I suggest to also
> keep explicitly testing tag-to-blob and tag-to-tree (and if we really
> also want tag-to-commit and tag-to-tag) behaviour. Reason is: if we skip
> those now, they can potentially break in the future.

> I would also suggest to fix upload-pack, as it is just not consistent to
> reject sending objects that were advertised, and so can strike again
> some way in the future. After all git.git's fetch-pack is not the only
> git client that should be possible to interact with git.git's
> upload-pack on remote side, right?

> For the reference all the cases presented here are real - they appear in
> our repositories on lab.nexedi.com for which I maintain the backup, and
> I've noticed them in the process of switching git-backup from using
> fetch to fetch-pack here:
> 
> https://lab.nexedi.com/kirr/git-backup/blob/0ab7bbb6/git-backup.go#L436

> On Mon, Jun 11, 2018 at 09:43:02AM +0000, Kirill Smelkov wrote:
> 
> > > Looking deeper, we do not need these trees and blobs at all. The problem
> > > is really just a tag that peels to an object that is not otherwise a ref
> > > tip, regardless of its type.
> > 
> > Thanks for feedback and for coming up with the fix. Sure, I'm ok with
> > moving the test into your patch. However, even if a test becomes
> > different - narrowing down root of _current_ problem, I suggest to also
> > keep explicitly testing tag-to-blob and tag-to-tree (and if we really
> > also want tag-to-commit and tag-to-tag) behaviour. Reason is: if we skip
> > those now, they can potentially break in the future.
> 
> Yeah, I have no problem testing these cases separately. There's no bug
> with them now, but it is a slightly uncommon case. My suggestion would
> be to submit a patch that goes on top of mine that covers these cases.

> > I would also suggest to fix upload-pack, as it is just not consistent to
> > reject sending objects that were advertised, and so can strike again
> > some way in the future. After all git.git's fetch-pack is not the only
> > git client that should be possible to interact with git.git's
> > upload-pack on remote side, right?
> 
> No, it's not the only client. At the same time, I am on the fence over
> whether upload-pack's behavior is wrong or not. It depends what you take
> a peeled advertisement line to mean. Does it mean: this object has been
> advertised and clients should be able to fetch it? Or does it mean: by
> the way, you may be interested to know the peeled value of this tag in
> case you want to do tag-following?
> 
> So far I think it has only meant the latter. I could see an argument for
> the former, but any client depending on that would never have worked,
> AFAICT. We could _make_ it work, but how would a client know which
> server version it's talking to (and therefore whether it is safe to make
> the request?). I think you'd have to add a capability to negotiate.

> > I'm not sure, but I would say that `fetch-pack --all` from an empty
> > repository should not fail and should just give empty output as fetch
> > does.
> 
> Yeah, that seems reasonable to me. The die() that catches this dates
> back to 2005-era, and we later taught the "fetch" porcelain to handle
> this. I don't _think_ anybody would be upset that the plumbing learned
> to treat this as a noop. It's probably a one-liner change in
> fetch_pack() to return early instead of dying.

> > For the reference all the cases presented here are real - they appear in
> > our repositories on lab.nexedi.com for which I maintain the backup, and
> > I've noticed them in the process of switching git-backup from using
> > fetch to fetch-pack here:
> > 
> > https://lab.nexedi.com/kirr/git-backup/blob/0ab7bbb6/git-backup.go#L436
> 
> I applaud you using the porcelain for your scripts, but I suspect that
> fetch-pack by itself is not at all well-used or well-tested these days
> (certainly this --all bug has been around for almost 6 years and is not
> very hard to trigger in practice).

> If an extra connection isn't a problem, you might be better off with
> "git ls-remote", and then picking through the results for refs of
> interest, and then "git fetch-pack" to actually get the pack. That's how
> git-fetch worked when it was a shell script (e.g., see c3a200120d, the
> last shell version).

> 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 ..
>         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

> +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 &&

> +	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 &&
>  	(

> 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.

> 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

> > 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?

> On Wed, Jun 13, 2018 at 05:05:09PM -0400, Jeff King wrote:
> 
> > > 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?
> 
> Hmm, it looks like this already hit 'next', so it is too late to change
> the commit message (although 'next' will get rewound after the release,
> so we _could_ do it then).
> 
> I also was going to suggest these style fixes, which could be applied on
> top (or squashed if we end up going that route). I actually wonder if
> the final tag one could just use two invocations of "git tag -m", but
> it's probably not worth spending too much time on polishing.

> -- >8 --
> Subject: [PATCH] t5500: prettify non-commit tag tests
> 
> We don't need to use backslash continuation, as the "&&"
> already provides continuation (and happily soaks up empty
> lines between commands).
> 
> We can also expand the multi-line printf into a
> here-document, which lets us use line breaks more naturally
> (and avoids another continuation that required us to break
> the natural indentation).
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  t/t5500-fetch-pack.sh | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
> index ea6570e819..3d33ab3875 100755
> --- a/t/t5500-fetch-pack.sh
> +++ b/t/t5500-fetch-pack.sh
> @@ -533,19 +533,26 @@ test_expect_success 'test --all wrt tag to non-commits' '
>  	# 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 <author@example.com> 0 +0000\n\nhello tag" | git mktag) &&
> +	tag=$(git mktag <<-EOF
> +		object $blob2
> +		type blob
> +		tag tag-to-blob2
> +		tagger author A U Thor <author@example.com> 0 +0000
> +
> +		hello tag
> +	EOF
> +	) &&
>  	git tag -a -m "tag -> tag" tag-to-tag $tag &&
> - \
> +
>  	# `fetch-pack --all` should succeed fetching all those objects.
>  	mkdir fetchall &&
>  	(
> -- 
> 2.18.0.rc2.519.gb87ed92113

> Jeff, thanks for corrections. I originally tried to look into invoking
> "git tag" two times, but since git tag always creates a reference it
> would not be semantically the same as having one referenced tag object
> pointing to another tag object which does not have anything in refs/
> pointing to it directly.

> Kirill Smelkov <kirr@nexedi.com> writes:
> 
> > Jeff, thanks for corrections. I originally tried to look into invoking
> > "git tag" two times, but since git tag always creates a reference it
> > would not be semantically the same as having one referenced tag object
> > pointing to another tag object which does not have anything in refs/
> > pointing to it directly.
> 
> Well, then you could remove it after you are done, no?  I.e.
> 
> 	git tag -a -m "tag to commit" tag-to-commit HEAD
> 	git tag -a -m "tag to commit (1)" temp-tag HEAD~1
> 	git tag -a -m "tag to tag" tag-to-tag temp-tag
> 	git tag -d temp-tag
> 
> would make the temp-tag object reachable only via refs/tags/tag-to-tag
> I think.

> > > I'm not sure, but I would say that `fetch-pack --all` from an empty
> > > repository should not fail and should just give empty output as fetch
> > > does.
> > 
> > Yeah, that seems reasonable to me. The die() that catches this dates
> > back to 2005-era, and we later taught the "fetch" porcelain to handle
> > this. I don't _think_ anybody would be upset that the plumbing learned
> > to treat this as a noop. It's probably a one-liner change in
> > fetch_pack() to return early instead of dying.
> 
> Ok, I will try to send related testcase, and it is indeed easy to find
> - the fix itself.

> ( Junio, please pick up the patch provided in the end )
>
> On Tue, Jun 12, 2018 at 06:54:17PM +0000, Kirill Smelkov wrote:
>> On Tue, Jun 12, 2018 at 05:48:49AM -0400, Jeff King wrote:
>> > On Mon, Jun 11, 2018 at 09:43:02AM +0000, Kirill Smelkov wrote:
> [...]
>
>> > > I'm not sure, but I would say that `fetch-pack --all` from an empty
>> > > repository should not fail and should just give empty output as fetch
>> > > does.
>> > 
>> > Yeah, that seems reasonable to me. The die() that catches this dates
>> > back to 2005-era, and we later taught the "fetch" porcelain to handle
>> > this. I don't _think_ anybody would be upset that the plumbing learned
>> > to treat this as a noop. It's probably a one-liner change in
>> > fetch_pack() to return early instead of dying.

> +test_expect_failure 'test --all wrt empty.git' '
> +	git init --bare empty.git &&
> +	(
> +		cd client &&
> +		git fetch-pack --all ../empty.git
> +	)
> +'

> Kirill Smelkov <kirr@nexedi.com> writes:
> 
> > ( Junio, please pick up the patch provided in the end )
> >
> > On Tue, Jun 12, 2018 at 06:54:17PM +0000, Kirill Smelkov wrote:
> >> On Tue, Jun 12, 2018 at 05:48:49AM -0400, Jeff King wrote:
> >> > On Mon, Jun 11, 2018 at 09:43:02AM +0000, Kirill Smelkov wrote:
> > [...]
> >
> >> > > I'm not sure, but I would say that `fetch-pack --all` from an empty
> >> > > repository should not fail and should just give empty output as fetch
> >> > > does.
> >> > 
> >> > Yeah, that seems reasonable to me. The die() that catches this dates
> >> > back to 2005-era, and we later taught the "fetch" porcelain to handle
> >> > this. I don't _think_ anybody would be upset that the plumbing learned
> >> > to treat this as a noop. It's probably a one-liner change in
> >> > fetch_pack() to return early instead of dying.
> 
> I actually have a slight preference to the current "attempting to
> fetch from a total emptiness is so rare that it is worth grabbing
> attention of whoever does so" behaviour, to be honest.

> Oh, wait, is this specific to "fetch-pack" and the behaviour of
> end-user-facing "git fetch" is kept same as before?  If then, I'd be
> somewhat sympathetic to the cause---it would be more convenient for
> the calling Porcelain script if this turned into a silent noop (even
> though it would probably make it harder to diagnose when such a
> Porcelain is set up incorrectly e.g. pointing at an empty repository
> that is not the one the Porcelain writer intended to fetch from).

> > However with transport.c being there too, since I'm no longer using
> > `fetch-pack --all`, now it is best for me to not delve into this story
> > and just stop with attached patch.
> 
> If we do not plan to change the behaviour later ourselves, I do not
> think it makes sense, nor it is fair to those future developers who
> inherit this project, to declare that the established behaviour is
> wrong with an 'expect-failure' test like this, to be honest.

> > +test_expect_failure 'test --all wrt empty.git' '
> > +	git init --bare empty.git &&
> > +	(
> > +		cd client &&
> > +		git fetch-pack --all ../empty.git
> > +	)
> > +'

> > If an extra connection isn't a problem, you might be better off with
> > "git ls-remote", and then picking through the results for refs of
> > interest, and then "git fetch-pack" to actually get the pack. That's how
> > git-fetch worked when it was a shell script (e.g., see c3a200120d, the
> > last shell version).
> 
> Yes, this is what I ended up doing:
> 
> https://lab.nexedi.com/kirr/git-backup/commit/899103bf
> 
> but for another reason - to avoid repeating for every fetched repository
> slow (in case of my "big" destination backup repository) quickfetch()
> checking in every spawned `git fetch`: git-backup can build index of
> objects we already have ourselves only once at startup, and then in
> fetch, after checking lsremote output, consult that index, and if we see
> we already have everything for an advertised reference - just avoid
> giving it to fetch-pack to process. It turns out for many pulled
> repositories there is usually no references changed at all and this way
> fetch-pack can be skipped completely:
> 
> https://lab.nexedi.com/kirr/git-backup/commit/3efed898

> On Tue, Jun 12, 2018 at 06:54:17PM +0000, Kirill Smelkov wrote:
> 
> > > If an extra connection isn't a problem, you might be better off with
> > > "git ls-remote", and then picking through the results for refs of
> > > interest, and then "git fetch-pack" to actually get the pack. That's how
> > > git-fetch worked when it was a shell script (e.g., see c3a200120d, the
> > > last shell version).
> > 
> > Yes, this is what I ended up doing:
> > 
> > https://lab.nexedi.com/kirr/git-backup/commit/899103bf
> > 
> > but for another reason - to avoid repeating for every fetched repository
> > slow (in case of my "big" destination backup repository) quickfetch()
> > checking in every spawned `git fetch`: git-backup can build index of
> > objects we already have ourselves only once at startup, and then in
> > fetch, after checking lsremote output, consult that index, and if we see
> > we already have everything for an advertised reference - just avoid
> > giving it to fetch-pack to process. It turns out for many pulled
> > repositories there is usually no references changed at all and this way
> > fetch-pack can be skipped completely:
> > 
> > https://lab.nexedi.com/kirr/git-backup/commit/3efed898
> 
> Thanks for sharing that, it's an interesting case. I'd hope that
> git-fetch is smart enough not to even bother with quickfetch() if there
> are no refs to update. But if we have even one change to fetch, then
> yeah, in the general case it makes sense to me that you could do better
> by amortizing the scan of local objects across many operations.