From: Jonathan Tan Date: Wed, 12 Apr 2017 18:27:57 GMT Subject: Re: [PATCH] fetch-pack: show clearer error message upon ERR Message-ID: In-Reply-To: <20170411214748.GR8741@aiede.mtv.corp.google.com> 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 Thanks - used your error message suggestion in the latest version [1]. [1] https://public-inbox.org/git/20170412180602.12713-1-jonathantanmy@google.com/ > 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).