Re: [PATCH] fetch: print an error when declining to request an unadvertised object

[Junio C Hamano <gitster@pobox.com>]

Matt McCutchen <matt@mattmccutchen.net> writes:

> Currently "git fetch REMOTE SHA1" silently exits 1 if the server doesn't
> allow requests for unadvertised objects by sha1.  Change it to print a
> meaningful error message.
>
> Signed-off-by: Matt McCutchen <matt@mattmccutchen.net>
> ---
>
> The fetch code looks unbelievably complicated to me and I don't understand all
> the cases that can arise.  Hopefully this patch is an acceptable solution to the
> problem.

At first I thought that this should be caught on the sending end
(grep for "not our ref" in upload-pack.c), but you found a case
where we do not even ask the sender on the requesting side.

I am not convinced that modifying filter_refs() is needed or even
desirable, though.

There is this piece of code near the end of builtin/fetch-pack.c:

	/*
	 * If the heads to pull were given, we should have consumed
	 * all of them by matching the remote.  Otherwise, 'git fetch
	 * remote no-such-ref' would silently succeed without issuing
	 * an error.
	 */
	for (i = 0; i < nr_sought; i++) {
		if (!sought[i] || sought[i]->matched)
			continue;
		error("no such remote ref %s", sought[i]->name);
		ret = 1;
	}

that happens before the command shows the list of fetched refs, and
this code is prepared to inspect what happend to the requests it (in
response to the user request) made to the underlying fetch
machinery, and issue the error message.
If you change your command line to "git fetch-pack REMOTE SHA1", you
do see an error from the above.

That is a good indication that the underlying fetch machinery called
by this caller is already doing diagnosis that is necessary for the
caller to issue such an error.  So why do we fail to say anything in
"git fetch"?

I think the real issue is that when fetch-pack machinery is used via
the transport layer, the transport layer discards the information on
these original request (i.e. what is returned in sought[]).

Instead, the caller of the fetch-pack machinery receives the list of
filtered refs as the return value of fetch_pack() function, and only
looks at "refs" without checking what happened to the original
request to_fetch[] (which corresponds to sought[] in the fetch-pack
command).  This all happens in transport.c::fetch_refs_via_pack().
I think that function is a much better place to error or die than
filter_refs().


>  fetch-pack.c          | 31 ++++++++++++++++---------------
>  t/t5516-fetch-push.sh |  3 ++-
>  2 files changed, 18 insertions(+), 16 deletions(-)
>
> diff --git a/fetch-pack.c b/fetch-pack.c
> index 601f077..117874c 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -598,23 +598,24 @@ static void filter_refs(struct fetch_pack_args *args,
>  	}
>  
>  	/* Append unmatched requests to the list */
> -	if ((allow_unadvertised_object_request &
> -	    (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1))) {
> -		for (i = 0; i < nr_sought; i++) {
> -			unsigned char sha1[20];
> +	for (i = 0; i < nr_sought; i++) {
> +		unsigned char sha1[20];
>  
> -			ref = sought[i];
> -			if (ref->matched)
> -				continue;
> -			if (get_sha1_hex(ref->name, sha1) ||
> -			    ref->name[40] != '\0' ||
> -			    hashcmp(sha1, ref->old_oid.hash))
> -				continue;
> +		ref = sought[i];
> +		if (ref->matched)
> +			continue;
> +		if (get_sha1_hex(ref->name, sha1) ||
> +		    ref->name[40] != '\0' ||
> +		    hashcmp(sha1, ref->old_oid.hash))
> +			continue;
>  
> -			ref->matched = 1;
> -			*newtail = copy_ref(ref);
> -			newtail = &(*newtail)->next;
> -		}
> +		if (!(allow_unadvertised_object_request &
> +		    (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1)))
> +			die(_("Server does not allow request for unadvertised object %s"), ref->name);
> +
> +		ref->matched = 1;
> +		*newtail = copy_ref(ref);
> +		newtail = &(*newtail)->next;
>  	}
>  	*refs = newlist;
>  }
> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index 0fc5a7c..177897e 100755
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -1098,7 +1098,8 @@ test_expect_success 'fetch exact SHA1' '
>  		test_must_fail git cat-file -t $the_commit &&
>  
>  		# fetching the hidden object should fail by default
> -		test_must_fail git fetch -v ../testrepo $the_commit:refs/heads/copy &&
> +		test_must_fail git fetch -v ../testrepo $the_commit:refs/heads/copy 2>err &&
> +		test_i18ngrep "Server does not allow request for unadvertised object" err &&
>  		test_must_fail git rev-parse --verify refs/heads/copy &&
>  
>  		# the server side can allow it to succeed