Re: [PATCH v2] fetch-pack: show clearer error message upon ERR
- From
- Jonathan Tan <jonathantanmy@google.com>
- Date
- Apr 17, 2017, 17:49 UTC
- Message-ID
- <21160598-dbb4-2f14-8e22-f6299a1d9004@google.com>
- In-Reply-To
- <20170412180602.12713-1-jonathantanmy@google.com>
Junio, I noticed that this did not make it into your "What's cooking" e-mail [1] - is there anything more that you would like to see in this patch?
Jonathan Nieder has reviewed an earlier version, and seems to be OK with it. He recommended a change in the error message, which I have changed in this new version. I have also done one of his follow-up ideas (updating the documentation) in this patch.
[1] http://public-inbox.org/git/xmqqmvbfij92.fsf@gitster.mtv.corp.google.com/
On 04/12/2017 11:06 AM, Jonathan Tan wrote:
Show 64 quoted lines
> 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.
>
> Also update the documentation describing the fetch-pack/upload-pack
> protocol (pack-protocol.txt) to indicate that "ERR" can be sent in the
> place of "ACK" or "NAK". In practice, this has been done for quite some
> time by other Git implementations (e.g. JGit sends "want $id not valid")
> and by Git itself (since commit bdb31ea: "upload-pack: report "not our
> ref" to client", 2017-02-23) whenever 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@google.com>
> ---
>
> Changes from v1:
> - used jrneider's suggested change to error message
> - added documentation about "ERR" in protocol
> - updated commit message to explain documentation change
>
> Documentation/technical/pack-protocol.txt | 7 ++++++-
> fetch-pack.c | 2 ++
> 2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt
> index c59ac9936..5b0ba3ef2 100644
> --- a/Documentation/technical/pack-protocol.txt
> +++ b/Documentation/technical/pack-protocol.txt
> @@ -351,14 +351,19 @@ ACK after 'done' if there is at least one common base and multi_ack or
> multi_ack_detailed is enabled. The server always sends NAK after 'done'
> if there is no common base found.
>
> +Instead of 'ACK' or 'NAK', the server may send an error message (for
> +example, if it does not recognize an object in a 'want' line received
> +from the client).
> +
> Then the server will start sending its packfile data.
>
> ----
> - server-response = *ack_multi ack / nak
> + server-response = *ack_multi ack / nak / error-line
> ack_multi = PKT-LINE("ACK" SP obj-id ack_status)
> ack_status = "continue" / "common" / "ready"
> ack = PKT-LINE("ACK" SP obj-id)
> nak = PKT-LINE("NAK")
> + error-line = PKT-LINE("ERR" SP explanation-text)
> ----
>
> A simple clone may look like this (with no 'have' lines):
> diff --git a/fetch-pack.c b/fetch-pack.c
> index d07d85ce3..4bed270c5 100644
> --- 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(_("remote error: %s"), arg);
> die(_("git fetch-pack: expected ACK/NAK, got '%s'"), line);
> }
>
>