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

From: Matt McCutchen <matt@mattmccutchen.net>
Date: Feb 12, 2017, 21:13 UTC
Message-ID: <1486934007.8517.10.camel@mattmccutchen.net>
In-Reply-To: <xmqqwpcxlwpv.fsf@gitster.mtv.corp.google.com>

On Fri, 2017-02-10 at 10:36 -0800, Junio C Hamano wrote:

Show 11 quoted lines

> > 
> There is this piece of code near the end of builtin/fetch-pack.c:
> 
> [...]
> 
> 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.

Yes, "error: no such remote ref NNNN", which at least makes clear that the operation didn't work, though it would be nice to give a more specific error message.

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

I confirmed that checking the sought refs there works. However, in filter_refs, it's easy to give a more specific error message that the server doesn't allow requests for unadvertised objects, and that code works for "git fetch-pack" too. To do the same in fetch_refs_via_pack, we'd have to duplicate a few lines of code from filter_refs and expose the allow_unadvertised_object_request variable, or just set a flag on the "struct ref" in filter_refs and check it in fetch_refs_via_pack.

What do you think? Do you not care about having a more specific error, in which case I can copy the code from builtin/fetch-pack.c to fetch_refs_via_pack? Or shall I add code to filter_refs to set a flag and add code to builtin/fetch-pack.c and fetch_refs_via_pack to check the flag? Or what?

Matt