Re: [PATCH] fetch-pack: show clearer error message upon ERR

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Apr 11, 2017, 21:47 UTC
Message-ID: <20170411214748.GR8741@aiede.mtv.corp.google.com>
In-Reply-To: <20170410210511.8505-1-jonathantanmy@google.com>

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!

Show 8 quoted lines

> (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@google.com>
> ---
>  fetch-pack.c | 2 ++
>  1 file changed, 2 insertions(+)

[...]

Show 8 quoted lines

> --- 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@gmail.com>

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);
 }