threads / patch / 45653

patchfetch-pack: show clearer error message upon ERR

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

## tl;dr

8 messages between Apr 10, 2017 and Apr 18, 2017. Diffs are folded; open one to read it.

replies: 7people: 3as markdown or json

Jonathan Tan· Apr 10, 2017, 21:05 UTC · lore

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(+)
Show changes to fetch-pack.c +2 −0
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
Jonathan Nieder· Apr 11, 2017, 21:47 UTC · re: Jonathan Tan · lore

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

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?
Show changes to diff +1 −1
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);
 }
Jonathan Tan· Apr 12, 2017, 18:27 UTC · re: Jonathan Nieder · lore

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

On 04/11/2017 02:47 PM, Jonathan Nieder wrote:
Show 9 quoted lines
> 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/

Show 5 quoted lines
> 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).

Jonathan Tan· Apr 12, 2017, 18:06 UTC · re: Jonathan Tan · lore

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

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(-)
Show changes to 2 files +8 −1

Documentation/technical/pack-protocol.txt, fetch-pack.c

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
Jonathan Tan· Apr 17, 2017, 17:49 UTC · re: Jonathan Tan · lore

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

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);
>  }
>
>
Junio C Hamano· Apr 18, 2017, 01:48 UTC · re: Jonathan Tan · lore

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

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.

Junio C Hamano· Apr 18, 2017, 04:50 UTC · re: Jonathan Tan · lore

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

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.
Jonathan Nieder· Apr 17, 2017, 22:56 UTC · re: Jonathan Tan · lore

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

Jonathan Tan wrote:
Show 24 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(-)
For what it's worth,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Thanks for writing it.  Sorry I didn't reply sooner.

← back to recent threads