{"thread":{"id":"57886","subject":"[PATCH] fetch-pack: make unexpected peek result non-fatal","startedAt":"2022-05-16T11:02:31Z","lastAt":"2022-05-17T06:20:42Z","messageCount":3,"participants":["Jonathan Tan","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"455315","messageId":"20220516110221.3490982-1-jonathantanmy@google.com","threadId":"57886","inReplyTo":null,"subject":"[PATCH] fetch-pack: make unexpected peek result non-fatal","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-05-16T11:02:20Z","receivedAt":"2022-05-16T11:02:31Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When a Git server responds to a fetch request, it may send optional\nsections before the packfile section. To handle this, the Git client\ncalls packet_reader_peek() (see process_section_header()) in order to\nsee what's next without consuming the line.\n\nHowever, as implemented, Git errors out whenever what's peeked is not an\nordinary line. This is not only unexpected (here, we only need to know\nwhether the upcoming line is the section header we want) but causes\nerrors to include the name of a section header that is irrelevant to the\ncause of the error. For example, at $DAYJOB, we have seen \"fatal: error\nreading section header 'shallow-info'\" error messages when none of the\nrepositories involved are shallow.\n\nTherefore, fix this so that the peek returns 1 if the upcoming line is\nthe wanted section header and nothing else. Because of this change,\nreader->line may now be NULL later in the function, so update the error\nmessage printing code accordingly.\n\nSigned-off-by: Jonathan Tan \n---\n fetch-pack.c | 19 +++++++++++--------\n 1 file changed, 11 insertions(+), 8 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 4e1e88eea0..6d0d271259 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1370,17 +1370,20 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,\n static int process_section_header(struct packet_reader *reader,\n \t\t\t\t const char *section, int peek)\n {\n-\tint ret;\n-\n-\tif (packet_reader_peek(reader) != PACKET_READ_NORMAL)\n-\t\tdie(_(\"error reading section header '%s'\"), section);\n+\tint ret = 0;\n \n-\tret = !strcmp(reader->line, section);\n+\tif (packet_reader_peek(reader) == PACKET_READ_NORMAL &&\n+\t !strcmp(reader->line, section))\n+\t\tret = 1;\n \n \tif (!peek) {\n-\t\tif (!ret)\n-\t\t\tdie(_(\"expected '%s', received '%s'\"),\n-\t\t\t section, reader->line);\n+\t\tif (!ret) {\n+\t\t\tif (reader->line)\n+\t\t\t\tdie(_(\"expected '%s', received '%s'\"),\n+\t\t\t\t section, reader->line);\n+\t\t\telse\n+\t\t\t\tdie(_(\"expected '%s'\"), section);\n+\t\t}\n \t\tpacket_reader_read(reader);\n \t}\n \n-- \n2.36.0.550.gb090851708-goog\n\n"},{"id":"455325","messageId":"xmqqr14t4fwf.fsf@gitster.g","threadId":"57886","inReplyTo":"20220516110221.3490982-1-jonathantanmy@google.com","subject":"Re: [PATCH] fetch-pack: make unexpected peek result non-fatal","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-16T16:10:40Z","receivedAt":"2022-05-16T16:10:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> When a Git server responds to a fetch request, it may send optional\n> sections before the packfile section. To handle this, the Git client\n> calls packet_reader_peek() (see process_section_header()) in order to\n> see what's next without consuming the line.\n>\n> However, as implemented, Git errors out whenever what's peeked is not an\n> ordinary line. This is not only unexpected (here, we only need to know\n> whether the upcoming line is the section header we want) but causes\n> errors to include the name of a section header that is irrelevant to the\n> cause of the error. For example, at $DAYJOB, we have seen \"fatal: error\n> reading section header 'shallow-info'\" error messages when none of the\n> repositories involved are shallow.\n>\n> Therefore, fix this so that the peek returns 1 if the upcoming line is\n> the wanted section header and nothing else. Because of this change,\n> reader->line may now be NULL later in the function, so update the error\n> message printing code accordingly.\n\n[not a suggestion to change anything; just making sure I got the\nabove correct]\n\nWe used to die unless READ_NORMAL was returned, so the code after\npacket_reader_peek() can rely on reader->line to be populated. Now\nthat code must handle cases where an earlier _peek() gave something\nother than READ_NORMAL and in such an error case reader->line points\nat NULL. So any code that assumed reader->line was populated needs\nto be updated.\n\nMakes sense.\n\n> Signed-off-by: Jonathan Tan \n> ---\n> fetch-pack.c | 19 +++++++++++--------\n> 1 file changed, 11 insertions(+), 8 deletions(-)\n>\n> diff --git a/fetch-pack.c b/fetch-pack.c\n> index 4e1e88eea0..6d0d271259 100644\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n> @@ -1370,17 +1370,20 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,\n> static int process_section_header(struct packet_reader *reader,\n> \t\t\t\t const char *section, int peek)\n> {\n> -\tint ret;\n> -\n> -\tif (packet_reader_peek(reader) != PACKET_READ_NORMAL)\n> -\t\tdie(_(\"error reading section header '%s'\"), section);\n> +\tint ret = 0;\n> \n> -\tret = !strcmp(reader->line, section);\n> +\tif (packet_reader_peek(reader) == PACKET_READ_NORMAL &&\n> +\t !strcmp(reader->line, section))\n> +\t\tret = 1;\n> \n> \tif (!peek) {\n> -\t\tif (!ret)\n> -\t\t\tdie(_(\"expected '%s', received '%s'\"),\n> -\t\t\t section, reader->line);\n> +\t\tif (!ret) {\n> +\t\t\tif (reader->line)\n> +\t\t\t\tdie(_(\"expected '%s', received '%s'\"),\n> +\t\t\t\t section, reader->line);\n> +\t\t\telse\n> +\t\t\t\tdie(_(\"expected '%s'\"), section);\n> +\t\t}\n> \t\tpacket_reader_read(reader);\n> \t}\n"},{"id":"455375","messageId":"20220517062035.3648639-1-jonathantanmy@google.com","threadId":"57886","inReplyTo":"xmqqr14t4fwf.fsf@gitster.g","subject":"Re: [PATCH] fetch-pack: make unexpected peek result non-fatal","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-05-17T06:20:34Z","receivedAt":"2022-05-17T06:20:42Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Junio C Hamano writes:\n> We used to die unless READ_NORMAL was returned, so the code after\n> packet_reader_peek() can rely on reader->line to be populated. Now\n> that code must handle cases where an earlier _peek() gave something\n> other than READ_NORMAL and in such an error case reader->line points\n> at NULL. So any code that assumed reader->line was populated needs\n> to be updated.\n> \n> Makes sense.\n\nThanks for taking a look. Yes, that's correct.\n"}]}