Hi,
Jonathan Tan wrote:
> Currently, fetch-pack prints a confusing error message ("expected
> ACK/NAK") when the server it's communicating with sends a pkt-line
> starting with "ERR". Replace it with a less confusing error message.
Yay!
> (Git will send "ERR" lines when a "want" line references an object that
> it does not have. This is uncommon, but can happen if a repository is
> garbage-collected during a negotiation.)
>
> Signed-off-by: Jonathan Tan <jonathantanmy@xxxxxxxxxx>
> ---
> fetch-pack.c | 2 ++
> 1 file changed, 2 insertions(+)
[...]
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -276,6 +276,8 @@ static enum ack_type get_ack(int fd, unsigned char *result_sha1)
> return ACK;
> }
> }
> + if (skip_prefix(line, "ERR ", &arg))
> + die(_("git fetch-pack: got remote error '%s'"), arg);
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.
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?
* 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?
* 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?
diff --git i/fetch-pack.c w/fetch-pack.c
index 688523bfd8..4bed270c54 100644
--- i/fetch-pack.c
+++ w/fetch-pack.c
@@ -277,7 +277,7 @@ static enum ack_type get_ack(int fd, unsigned char *result_sha1)
}
}
if (skip_prefix(line, "ERR ", &arg))
- die(_("git fetch-pack: got remote error '%s'"), arg);
+ die(_("remote error: %s"), arg);
die(_("git fetch-pack: expected ACK/NAK, got '%s'"), line);
}