Re: [PATCH] fetch-pack: show clearer error message upon ERR

From: Jonathan Tan <jonathantanmy@google.com>
Date: Apr 12, 2017, 18:27 UTC
Message-ID: <bc49db16-d434-6493-3020-25058bbcbad8@google.com>
In-Reply-To: <20170411214748.GR8741@aiede.mtv.corp.google.com>

On 04/11/2017 02:47 PM, Jonathan Nieder wrote:

Show 9 quoted lines

> nit about the error message: since this isn't a sign of an internal
> error, we don't need to tell the user that it happened in git
> fetch-pack.  How about using the same error used e.g. in
> run_remote_archiver and get_remote_heads?
>
> 		die(_("remote error: %s"), arg);
>
> With that change,
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks - used your error message suggestion in the latest version [1].

[1] https://public-inbox.org/git/20170412180602.12713-1-jonathantanmy@google.com/

Show 5 quoted lines

> Follow-up ideas:
>
> * would it be straightforward to verify this error message is produced
>   correctly in tests?  If not, is there some way to manually trigger it
>   as a sanity-check?

The best idea I can think of is to intercept fetch-pack's output and substitute a fake hash for a certain hash. This is probably easiest to do if we run upload-pack in stateless RPC mode (so that we can pipe fetch-pack's output into sed, and then into upload-pack) but that would require something like my test-un-pkt helper (in [2]). The change in this patch seems to be at best, correct, and at worst, harmless, so I didn't pursue this further.

[2] https://public-inbox.org/git/40f36d5eeb984adc220a4038fc77ed6ad1398fef.1491851452.git.jonathantanmy@google.com/

> * Documentation/technical/pack-protocol.txt only says that an error-line
>   can be produced in response to the initial git-upload-pack command.
>   Can it be updated to say where they are now allowed?

Done (in the latest version).

> * Are there other points in the protocol where it would be useful to
>   allow an error-line?  Is there some lower level place where they could
>   be handled?

For fetch-pack/upload-pack, I'm not sure - the sideband already has its own error reporting mechanism, so everything might already be covered (although I didn't look into detail). Lower-level handling would be ideal, but something that I'm not sure off-hand how to do - pkt-lines are used quite diversely (for both text and binary data, and with sideband indicator and without), so we would need to ensure that a solution would work with all those (and any future uses).