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

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

[Jonathan Tan]

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.

(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>
---

This situation has been noticed occasionally in my company - this is a
small change that would make the situation slightly easier to
understand.

 fetch-pack.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fetch-pack.c b/fetch-pack.c
index d07d85ce3..688523bfd 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(_("git fetch-pack: got remote error '%s'"), arg);
 	die(_("git fetch-pack: expected ACK/NAK, got '%s'"), line);
 }
 
-- 
2.12.2.715.g7642488e1d-goog

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

[Jonathan Nieder]

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@google.com>
> ---
>  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@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);
 }
 

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

[Jonathan Tan]

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);
 }
 
-- 
2.12.2.715.g7642488e1d-goog

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

[Jonathan Tan]

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

Thanks - used your error message suggestion in the latest version [1].

[1] 
https://public-inbox.org/git/20170412180602.12713-1-jonathantanmy@google.com/

> 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@google.com/

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

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

[Jonathan Tan]

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

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

[Jonathan Nieder]

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

For what it's worth,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks for writing it.  Sorry I didn't reply sooner.

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

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

> 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?

No.  

Actually "no" is not a honest answer; "I do not know yet" is.

It's just that I haven't fully caught up with the list traffic from
the past two weeks.  I've finished a single pass and picked up most
things that got discussed by multiple people.  But I haven't read
small patches that did not result in large-ish discussions---some of
these may have been quiet topics because they were obviously good,
and others may be quiet because they were totally unintesting.

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

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

> 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?

OK, now I've read it and it makes sense.  Thanks for working on it.