{"thread":{"id":"45653","subject":"[PATCH] fetch-pack: show clearer error message upon ERR","startedAt":"2017-04-10T21:05:23Z","lastAt":"2017-04-18T04:50:12Z","messageCount":8,"participants":["Jonathan Tan","Jonathan Nieder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"316518","messageId":"20170410210511.8505-1-jonathantanmy@google.com","threadId":"45653","inReplyTo":null,"subject":"[PATCH] fetch-pack: show clearer error message upon ERR","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-04-10T21:05:11Z","receivedAt":"2017-04-10T21:05:23Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Currently, fetch-pack prints a confusing error message (\"expected\nACK/NAK\") when the server it's communicating with sends a pkt-line\nstarting with \"ERR\". Replace it with a less confusing error message.\n\n(Git will send \"ERR\" lines when a \"want\" line references an object that\nit does not have. This is uncommon, but can happen if a repository is\ngarbage-collected during a negotiation.)\n\nSigned-off-by: Jonathan Tan \n---\n\nThis situation has been noticed occasionally in my company - this is a\nsmall change that would make the situation slightly easier to\nunderstand.\n\n fetch-pack.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex d07d85ce3..688523bfd 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -276,6 +276,8 @@ static enum ack_type get_ack(int fd, unsigned char *result_sha1)\n \t\t\treturn ACK;\n \t\t}\n \t}\n+\tif (skip_prefix(line, \"ERR \", &arg))\n+\t\tdie(_(\"git fetch-pack: got remote error '%s'\"), arg);\n \tdie(_(\"git fetch-pack: expected ACK/NAK, got '%s'\"), line);\n }\n \n-- \n2.12.2.715.g7642488e1d-goog\n\n"},{"id":"316652","messageId":"20170411214748.GR8741@aiede.mtv.corp.google.com","threadId":"45653","inReplyTo":"20170410210511.8505-1-jonathantanmy@google.com","subject":"Re: [PATCH] fetch-pack: show clearer error message upon ERR","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-04-11T21:47:48Z","receivedAt":"2017-04-11T21:47:56Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJonathan Tan wrote:\n\n> Currently, fetch-pack prints a confusing error message (\"expected\n> ACK/NAK\") when the server it's communicating with sends a pkt-line\n> starting with \"ERR\". Replace it with a less confusing error message.\n\nYay!\n\n> (Git will send \"ERR\" lines when a \"want\" line references an object that\n> it does not have. This is uncommon, but can happen if a repository is\n> garbage-collected during a negotiation.)\n>\n> Signed-off-by: Jonathan Tan \n> ---\n> fetch-pack.c | 2 ++\n> 1 file changed, 2 insertions(+)\n[...]\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n> @@ -276,6 +276,8 @@ static enum ack_type get_ack(int fd, unsigned char *result_sha1)\n> \t\t\treturn ACK;\n> \t\t}\n> \t}\n> +\tif (skip_prefix(line, \"ERR \", &arg))\n> +\t\tdie(_(\"git fetch-pack: got remote error '%s'\"), arg);\n\nnit about the error message: since this isn't a sign of an internal\nerror, we don't need to tell the user that it happened in git\nfetch-pack. How about using the same error used e.g. in\nrun_remote_archiver and get_remote_heads?\n\n\t\tdie(_(\"remote error: %s\"), arg);\n\nWith that change,\nReviewed-by: Jonathan Nieder \n\nThanks.\n\nFollow-up ideas:\n\n* would it be straightforward to verify this error message is produced\n correctly in tests? If not, is there some way to manually trigger it\n as a sanity-check?\n\n* Documentation/technical/pack-protocol.txt only says that an error-line\n can be produced in response to the initial git-upload-pack command.\n Can it be updated to say where they are now allowed?\n\n* Are there other points in the protocol where it would be useful to\n allow an error-line? Is there some lower level place where they could\n be handled?\n\ndiff --git i/fetch-pack.c w/fetch-pack.c\nindex 688523bfd8..4bed270c54 100644\n--- i/fetch-pack.c\n+++ w/fetch-pack.c\n@@ -277,7 +277,7 @@ static enum ack_type get_ack(int fd, unsigned char *result_sha1)\n \t\t}\n \t}\n \tif (skip_prefix(line, \"ERR \", &arg))\n-\t\tdie(_(\"git fetch-pack: got remote error '%s'\"), arg);\n+\t\tdie(_(\"remote error: %s\"), arg);\n \tdie(_(\"git fetch-pack: expected ACK/NAK, got '%s'\"), line);\n }\n \n"},{"id":"316695","messageId":"20170412180602.12713-1-jonathantanmy@google.com","threadId":"45653","inReplyTo":"20170410210511.8505-1-jonathantanmy@google.com","subject":"[PATCH v2] fetch-pack: show clearer error message upon ERR","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-04-12T18:06:02Z","receivedAt":"2017-04-12T18:06:18Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Currently, fetch-pack prints a confusing error message (\"expected\nACK/NAK\") when the server it's communicating with sends a pkt-line\nstarting with \"ERR\". Replace it with a less confusing error message.\n\nAlso update the documentation describing the fetch-pack/upload-pack\nprotocol (pack-protocol.txt) to indicate that \"ERR\" can be sent in the\nplace of \"ACK\" or \"NAK\". In practice, this has been done for quite some\ntime by other Git implementations (e.g. JGit sends \"want $id not valid\")\nand by Git itself (since commit bdb31ea: \"upload-pack: report \"not our\nref\" to client\", 2017-02-23) whenever a \"want\" line references an object\nthat it does not have. (This is uncommon, but can happen if a repository\nis garbage-collected during a negotiation.)\n\nSigned-off-by: Jonathan Tan \n---\n\nChanges from v1:\n - used jrneider's suggested change to error message\n - added documentation about \"ERR\" in protocol\n - updated commit message to explain documentation change\n\n Documentation/technical/pack-protocol.txt | 7 ++++++-\n fetch-pack.c | 2 ++\n 2 files changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt\nindex c59ac9936..5b0ba3ef2 100644\n--- a/Documentation/technical/pack-protocol.txt\n+++ b/Documentation/technical/pack-protocol.txt\n@@ -351,14 +351,19 @@ ACK after 'done' if there is at least one common base and multi_ack or\n multi_ack_detailed is enabled. The server always sends NAK after 'done'\n if there is no common base found.\n \n+Instead of 'ACK' or 'NAK', the server may send an error message (for\n+example, if it does not recognize an object in a 'want' line received\n+from the client).\n+\n Then the server will start sending its packfile data.\n \n ----\n- server-response = *ack_multi ack / nak\n+ server-response = *ack_multi ack / nak / error-line\n ack_multi = PKT-LINE(\"ACK\" SP obj-id ack_status)\n ack_status = \"continue\" / \"common\" / \"ready\"\n ack = PKT-LINE(\"ACK\" SP obj-id)\n nak = PKT-LINE(\"NAK\")\n+ error-line = PKT-LINE(\"ERR\" SP explanation-text)\n ----\n \n A simple clone may look like this (with no 'have' lines):\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex d07d85ce3..4bed270c5 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -276,6 +276,8 @@ static enum ack_type get_ack(int fd, unsigned char *result_sha1)\n \t\t\treturn ACK;\n \t\t}\n \t}\n+\tif (skip_prefix(line, \"ERR \", &arg))\n+\t\tdie(_(\"remote error: %s\"), arg);\n \tdie(_(\"git fetch-pack: expected ACK/NAK, got '%s'\"), line);\n }\n \n-- \n2.12.2.715.g7642488e1d-goog\n\n"},{"id":"316696","messageId":"bc49db16-d434-6493-3020-25058bbcbad8@google.com","threadId":"45653","inReplyTo":"20170411214748.GR8741@aiede.mtv.corp.google.com","subject":"Re: [PATCH] fetch-pack: show clearer error message upon ERR","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-04-12T18:27:57Z","receivedAt":"2017-04-12T18:28:12Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On 04/11/2017 02:47 PM, Jonathan Nieder wrote:\n> nit about the error message: since this isn't a sign of an internal\n> error, we don't need to tell the user that it happened in git\n> fetch-pack. How about using the same error used e.g. in\n> run_remote_archiver and get_remote_heads?\n>\n> \t\tdie(_(\"remote error: %s\"), arg);\n>\n> With that change,\n> Reviewed-by: Jonathan Nieder \n\nThanks - used your error message suggestion in the latest version [1].\n\n[1] \nhttps://public-inbox.org/git/20170412180602.12713-1-jonathantanmy@google.com/\n\n> Follow-up ideas:\n>\n> * would it be straightforward to verify this error message is produced\n> correctly in tests? If not, is there some way to manually trigger it\n> as a sanity-check?\n\nThe best idea I can think of is to intercept fetch-pack's output and \nsubstitute a fake hash for a certain hash. This is probably easiest to \ndo if we run upload-pack in stateless RPC mode (so that we can pipe \nfetch-pack's output into sed, and then into upload-pack) but that would \nrequire something like my test-un-pkt helper (in [2]). The change in \nthis patch seems to be at best, correct, and at worst, harmless, so I \ndidn't pursue this further.\n\n[2] \nhttps://public-inbox.org/git/40f36d5eeb984adc220a4038fc77ed6ad1398fef.1491851452.git.jonathantanmy@google.com/\n\n> * Documentation/technical/pack-protocol.txt only says that an error-line\n> can be produced in response to the initial git-upload-pack command.\n> Can it be updated to say where they are now allowed?\n\nDone (in the latest version).\n\n> * Are there other points in the protocol where it would be useful to\n> allow an error-line? Is there some lower level place where they could\n> be handled?\n\nFor fetch-pack/upload-pack, I'm not sure - the sideband already has its \nown error reporting mechanism, so everything might already be covered \n(although I didn't look into detail). Lower-level handling would be \nideal, but something that I'm not sure off-hand how to do - pkt-lines \nare used quite diversely (for both text and binary data, and with \nsideband indicator and without), so we would need to ensure that a \nsolution would work with all those (and any future uses).\n"},{"id":"317017","messageId":"21160598-dbb4-2f14-8e22-f6299a1d9004@google.com","threadId":"45653","inReplyTo":"20170412180602.12713-1-jonathantanmy@google.com","subject":"Re: [PATCH v2] fetch-pack: show clearer error message upon ERR","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-04-17T17:49:35Z","receivedAt":"2017-04-17T17:49:43Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Junio, I noticed that this did not make it into your \"What's cooking\" \ne-mail [1] - is there anything more that you would like to see in this \npatch?\n\nJonathan Nieder has reviewed an earlier version, and seems to be OK with \nit. He recommended a change in the error message, which I have changed \nin this new version. I have also done one of his follow-up ideas \n(updating the documentation) in this patch.\n\n[1] \nhttp://public-inbox.org/git/xmqqmvbfij92.fsf@gitster.mtv.corp.google.com/\n\nOn 04/12/2017 11:06 AM, Jonathan Tan wrote:\n> Currently, fetch-pack prints a confusing error message (\"expected\n> ACK/NAK\") when the server it's communicating with sends a pkt-line\n> starting with \"ERR\". Replace it with a less confusing error message.\n>\n> Also update the documentation describing the fetch-pack/upload-pack\n> protocol (pack-protocol.txt) to indicate that \"ERR\" can be sent in the\n> place of \"ACK\" or \"NAK\". In practice, this has been done for quite some\n> time by other Git implementations (e.g. JGit sends \"want $id not valid\")\n> and by Git itself (since commit bdb31ea: \"upload-pack: report \"not our\n> ref\" to client\", 2017-02-23) whenever a \"want\" line references an object\n> that it does not have. (This is uncommon, but can happen if a repository\n> is garbage-collected during a negotiation.)\n>\n> Signed-off-by: Jonathan Tan \n> ---\n>\n> Changes from v1:\n> - used jrneider's suggested change to error message\n> - added documentation about \"ERR\" in protocol\n> - updated commit message to explain documentation change\n>\n> Documentation/technical/pack-protocol.txt | 7 ++++++-\n> fetch-pack.c | 2 ++\n> 2 files changed, 8 insertions(+), 1 deletion(-)\n>\n> diff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt\n> index c59ac9936..5b0ba3ef2 100644\n> --- a/Documentation/technical/pack-protocol.txt\n> +++ b/Documentation/technical/pack-protocol.txt\n> @@ -351,14 +351,19 @@ ACK after 'done' if there is at least one common base and multi_ack or\n> multi_ack_detailed is enabled. The server always sends NAK after 'done'\n> if there is no common base found.\n>\n> +Instead of 'ACK' or 'NAK', the server may send an error message (for\n> +example, if it does not recognize an object in a 'want' line received\n> +from the client).\n> +\n> Then the server will start sending its packfile data.\n>\n> ----\n> - server-response = *ack_multi ack / nak\n> + server-response = *ack_multi ack / nak / error-line\n> ack_multi = PKT-LINE(\"ACK\" SP obj-id ack_status)\n> ack_status = \"continue\" / \"common\" / \"ready\"\n> ack = PKT-LINE(\"ACK\" SP obj-id)\n> nak = PKT-LINE(\"NAK\")\n> + error-line = PKT-LINE(\"ERR\" SP explanation-text)\n> ----\n>\n> A simple clone may look like this (with no 'have' lines):\n> diff --git a/fetch-pack.c b/fetch-pack.c\n> index d07d85ce3..4bed270c5 100644\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n> @@ -276,6 +276,8 @@ static enum ack_type get_ack(int fd, unsigned char *result_sha1)\n> \t\t\treturn ACK;\n> \t\t}\n> \t}\n> +\tif (skip_prefix(line, \"ERR \", &arg))\n> +\t\tdie(_(\"remote error: %s\"), arg);\n> \tdie(_(\"git fetch-pack: expected ACK/NAK, got '%s'\"), line);\n> }\n>\n>\n"},{"id":"317048","messageId":"20170417225655.GB8486@aiede.mtv.corp.google.com","threadId":"45653","inReplyTo":"20170412180602.12713-1-jonathantanmy@google.com","subject":"Re: [PATCH v2] fetch-pack: show clearer error message upon ERR","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-04-17T22:56:55Z","receivedAt":"2017-04-17T22:57:12Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Tan wrote:\n\n> Currently, fetch-pack prints a confusing error message (\"expected\n> ACK/NAK\") when the server it's communicating with sends a pkt-line\n> starting with \"ERR\". Replace it with a less confusing error message.\n>\n> Also update the documentation describing the fetch-pack/upload-pack\n> protocol (pack-protocol.txt) to indicate that \"ERR\" can be sent in the\n> place of \"ACK\" or \"NAK\". In practice, this has been done for quite some\n> time by other Git implementations (e.g. JGit sends \"want $id not valid\")\n> and by Git itself (since commit bdb31ea: \"upload-pack: report \"not our\n> ref\" to client\", 2017-02-23) whenever a \"want\" line references an object\n> that it does not have. (This is uncommon, but can happen if a repository\n> is garbage-collected during a negotiation.)\n>\n> Signed-off-by: Jonathan Tan \n> ---\n>\n> Changes from v1:\n> - used jrneider's suggested change to error message\n> - added documentation about \"ERR\" in protocol\n> - updated commit message to explain documentation change\n>\n> Documentation/technical/pack-protocol.txt | 7 ++++++-\n> fetch-pack.c | 2 ++\n> 2 files changed, 8 insertions(+), 1 deletion(-)\n\nFor what it's worth,\nReviewed-by: Jonathan Nieder \n\nThanks for writing it. Sorry I didn't reply sooner.\n"},{"id":"317060","messageId":"xmqqshl6h4es.fsf@gitster.mtv.corp.google.com","threadId":"45653","inReplyTo":"21160598-dbb4-2f14-8e22-f6299a1d9004@google.com","subject":"Re: [PATCH v2] fetch-pack: show clearer error message upon ERR","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-18T01:48:27Z","receivedAt":"2017-04-18T01:48:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> Junio, I noticed that this did not make it into your \"What's cooking\"\n> e-mail [1] - is there anything more that you would like to see in this\n> patch?\n\nNo. \n\nActually \"no\" is not a honest answer; \"I do not know yet\" is.\n\nIt's just that I haven't fully caught up with the list traffic from\nthe past two weeks. I've finished a single pass and picked up most\nthings that got discussed by multiple people. But I haven't read\nsmall patches that did not result in large-ish discussions---some of\nthese may have been quiet topics because they were obviously good,\nand others may be quiet because they were totally unintesting.\n"},{"id":"317080","messageId":"xmqqvaq2e2v8.fsf@gitster.mtv.corp.google.com","threadId":"45653","inReplyTo":"21160598-dbb4-2f14-8e22-f6299a1d9004@google.com","subject":"Re: [PATCH v2] fetch-pack: show clearer error message upon ERR","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-18T04:50:03Z","receivedAt":"2017-04-18T04:50:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> Junio, I noticed that this did not make it into your \"What's cooking\"\n> e-mail [1] - is there anything more that you would like to see in this\n> patch?\n\nOK, now I've read it and it makes sense. Thanks for working on it.\n"}]}