Re: [PATCH] fetch: print an error when declining to request an unadvertised object
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 10, 2017, 18:36 UTC
- Message-ID
- <xmqqwpcxlwpv.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <1486747828.17272.5.camel@mattmccutchen.net>
Matt McCutchen <matt@mattmccutchen.net> writes:
Show 10 quoted lines
> 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().
Show 62 quoted lines
> 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