Web lists-archives.com

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 [1].

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

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@xxxxxxxxxx/

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