From: Jonathan Tan <jonathantanmy@google.com>
Date: Mon, 17 Apr 2017 17:49:35 GMT
Subject: Re: [PATCH v2] fetch-pack: show clearer error message upon ERR
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:
> 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);
>  }
>
>