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