Re: [PATCH v2] fetch-pack: don't try to fetch peel values with --all

> 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