Re: [PATCH] fetch-pack: show clearer error message upon ERR
- Date: Wed, 12 Apr 2017 11:27:57 -0700
- From: Jonathan Tan <jonathantanmy@xxxxxxxxxx>
- Subject: Re: [PATCH] fetch-pack: show clearer error message upon ERR
On 04/11/2017 02:47 PM, Jonathan Nieder wrote:
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@xxxxxxxxx>
Thanks - used your error message suggestion in the latest version .
* 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 ). The change in
this patch seems to be at best, correct, and at worst, harmless, so I
didn't pursue this further.
* 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
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).