{"thread":{"id":"45228","subject":"[PATCH 1/2] commit: be more precise when searching for headers","startedAt":"2017-02-25T19:22:23Z","lastAt":"2017-02-27T22:57:27Z","messageCount":9,"participants":["René Scharfe","Jeff King","Junio C Hamano","Jakub Narębski"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"312648","messageId":"23989e76-24ba-90a4-91a9-9f66bfccb7c9@web.de","threadId":"45228","inReplyTo":null,"subject":"[PATCH 1/2] commit: be more precise when searching for headers","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-02-25T19:21:52Z","receivedAt":"2017-02-25T19:22:23Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Search for a space character only within the current line in\nread_commit_extra_header_lines() instead of searching in the whole\nbuffer (and possibly beyond, if it's not NUL-terminated) and then\ndiscarding any results after the end of the current line.\n\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\n commit.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex 2cf85158b4..173c6d3818 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1354,8 +1354,8 @@ static struct commit_extra_header *read_commit_extra_header_lines(\n \t\tstrbuf_reset(&buf);\n \t\tit = NULL;\n \n-\t\teof = strchr(line, ' ');\n-\t\tif (next <= eof)\n+\t\teof = memchr(line, ' ', next - line);\n+\t\tif (!eof)\n \t\t\teof = next;\n \n \t\tif (standard_header_field(line, eof - line) ||\n-- \n2.12.0\n\n"},{"id":"312650","messageId":"b1d5c882-38b8-dd2d-2e5f-aafb8dfada81@web.de","threadId":"45228","inReplyTo":"23989e76-24ba-90a4-91a9-9f66bfccb7c9@web.de","subject":"[PATCH 2/2] commit: don't check for space twice when looking for header","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-02-25T19:27:40Z","receivedAt":"2017-02-25T19:28:05Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Both standard_header_field() and excluded_header_field() check if\nthere's a space after the buffer that's handed to them.  We already\ncheck in the caller if that space is present.  Don't bother calling\nthe functions if it's missing, as they are guaranteed to return 0 in\nthat case, and remove the now redundant checks from them.\n\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\n commit.c | 18 ++++++++----------\n 1 file changed, 8 insertions(+), 10 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex 173c6d3818..fab8269731 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1308,11 +1308,11 @@ void for_each_mergetag(each_mergetag_fn fn, struct commit *commit, void *data)\n \n static inline int standard_header_field(const char *field, size_t len)\n {\n-\treturn ((len == 4 && !memcmp(field, \"tree \", 5)) ||\n-\t\t(len == 6 && !memcmp(field, \"parent \", 7)) ||\n-\t\t(len == 6 && !memcmp(field, \"author \", 7)) ||\n-\t\t(len == 9 && !memcmp(field, \"committer \", 10)) ||\n-\t\t(len == 8 && !memcmp(field, \"encoding \", 9)));\n+\treturn ((len == 4 && !memcmp(field, \"tree\", 4)) ||\n+\t\t(len == 6 && !memcmp(field, \"parent\", 6)) ||\n+\t\t(len == 6 && !memcmp(field, \"author\", 6)) ||\n+\t\t(len == 9 && !memcmp(field, \"committer\", 9)) ||\n+\t\t(len == 8 && !memcmp(field, \"encoding\", 8)));\n }\n \n static int excluded_header_field(const char *field, size_t len, const char **exclude)\n@@ -1322,8 +1322,7 @@ static int excluded_header_field(const char *field, size_t len, const char **exc\n \n \twhile (*exclude) {\n \t\tsize_t xlen = strlen(*exclude);\n-\t\tif (len == xlen &&\n-\t\t    !memcmp(field, *exclude, xlen) && field[xlen] == ' ')\n+\t\tif (len == xlen && !memcmp(field, *exclude, xlen))\n \t\t\treturn 1;\n \t\texclude++;\n \t}\n@@ -1357,9 +1356,8 @@ static struct commit_extra_header *read_commit_extra_header_lines(\n \t\teof = memchr(line, ' ', next - line);\n \t\tif (!eof)\n \t\t\teof = next;\n-\n-\t\tif (standard_header_field(line, eof - line) ||\n-\t\t    excluded_header_field(line, eof - line, exclude))\n+\t\telse if (standard_header_field(line, eof - line) ||\n+\t\t\t excluded_header_field(line, eof - line, exclude))\n \t\t\tcontinue;\n \n \t\tit = xcalloc(1, sizeof(*it));\n-- \n2.12.0\n\n"},{"id":"312651","messageId":"20170225201238.wkujdwg4jyvovelm@sigill.intra.peff.net","threadId":"45228","inReplyTo":"23989e76-24ba-90a4-91a9-9f66bfccb7c9@web.de","subject":"Re: [PATCH 1/2] commit: be more precise when searching for headers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-25T20:12:38Z","receivedAt":"2017-02-25T20:13:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Feb 25, 2017 at 08:21:52PM +0100, René Scharfe wrote:\n\n> Search for a space character only within the current line in\n> read_commit_extra_header_lines() instead of searching in the whole\n> buffer (and possibly beyond, if it's not NUL-terminated) and then\n> discarding any results after the end of the current line.\n> [...]\n> -\t\teof = strchr(line, ' ');\n> -\t\tif (next <= eof)\n> +\t\teof = memchr(line, ' ', next - line);\n> +\t\tif (!eof)\n>  \t\t\teof = next;\n\nNice. More efficient, and I think the intent is more clear.\n\n-Peff\n"},{"id":"312653","messageId":"20170225201522.uan52fwey6zjosym@sigill.intra.peff.net","threadId":"45228","inReplyTo":"b1d5c882-38b8-dd2d-2e5f-aafb8dfada81@web.de","subject":"Re: [PATCH 2/2] commit: don't check for space twice when looking for header","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-25T20:15:22Z","receivedAt":"2017-02-25T20:15:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Feb 25, 2017 at 08:27:40PM +0100, René Scharfe wrote:\n\n> Both standard_header_field() and excluded_header_field() check if\n> there's a space after the buffer that's handed to them.  We already\n> check in the caller if that space is present.  Don't bother calling\n> the functions if it's missing, as they are guaranteed to return 0 in\n> that case, and remove the now redundant checks from them.\n\nMakes sense, and I couldn't spot any errors in your logic or in the\ncode.\n\n>  static inline int standard_header_field(const char *field, size_t len)\n>  {\n> -\treturn ((len == 4 && !memcmp(field, \"tree \", 5)) ||\n> -\t\t(len == 6 && !memcmp(field, \"parent \", 7)) ||\n> -\t\t(len == 6 && !memcmp(field, \"author \", 7)) ||\n> -\t\t(len == 9 && !memcmp(field, \"committer \", 10)) ||\n> -\t\t(len == 8 && !memcmp(field, \"encoding \", 9)));\n> +\treturn ((len == 4 && !memcmp(field, \"tree\", 4)) ||\n> +\t\t(len == 6 && !memcmp(field, \"parent\", 6)) ||\n> +\t\t(len == 6 && !memcmp(field, \"author\", 6)) ||\n> +\t\t(len == 9 && !memcmp(field, \"committer\", 9)) ||\n> +\t\t(len == 8 && !memcmp(field, \"encoding\", 8)));\n\nUnrelated, but this could probably be spelled with a macro and strlen()\nto avoid the magic numbers. It would probably be measurably slower for a\ncompiler which doesn't pre-compute strlen() on a string literal, though.\n\n-Peff\n"},{"id":"312666","messageId":"6462e74c-1ee7-7ed0-0695-9889df803943@web.de","threadId":"45228","inReplyTo":"20170225201522.uan52fwey6zjosym@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] commit: don't check for space twice when looking for header","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-02-25T21:39:29Z","receivedAt":"2017-02-25T21:40:05Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 25.02.2017 um 21:15 schrieb Jeff King:\n> On Sat, Feb 25, 2017 at 08:27:40PM +0100, René Scharfe wrote:\n>\n>> Both standard_header_field() and excluded_header_field() check if\n>> there's a space after the buffer that's handed to them.  We already\n>> check in the caller if that space is present.  Don't bother calling\n>> the functions if it's missing, as they are guaranteed to return 0 in\n>> that case, and remove the now redundant checks from them.\n>\n> Makes sense, and I couldn't spot any errors in your logic or in the\n> code.\n\nThanks for checking!\n\n>>  static inline int standard_header_field(const char *field, size_t len)\n>>  {\n>> -\treturn ((len == 4 && !memcmp(field, \"tree \", 5)) ||\n>> -\t\t(len == 6 && !memcmp(field, \"parent \", 7)) ||\n>> -\t\t(len == 6 && !memcmp(field, \"author \", 7)) ||\n>> -\t\t(len == 9 && !memcmp(field, \"committer \", 10)) ||\n>> -\t\t(len == 8 && !memcmp(field, \"encoding \", 9)));\n>> +\treturn ((len == 4 && !memcmp(field, \"tree\", 4)) ||\n>> +\t\t(len == 6 && !memcmp(field, \"parent\", 6)) ||\n>> +\t\t(len == 6 && !memcmp(field, \"author\", 6)) ||\n>> +\t\t(len == 9 && !memcmp(field, \"committer\", 9)) ||\n>> +\t\t(len == 8 && !memcmp(field, \"encoding\", 8)));\n>\n> Unrelated, but this could probably be spelled with a macro and strlen()\n> to avoid the magic numbers. It would probably be measurably slower for a\n> compiler which doesn't pre-compute strlen() on a string literal, though.\n\nsizeof(string_constant) - 1 might be a better choice here than strlen().\n\nRené\n"},{"id":"312668","messageId":"20170225215144.hb7huqmv3seaoaqf@sigill.intra.peff.net","threadId":"45228","inReplyTo":"6462e74c-1ee7-7ed0-0695-9889df803943@web.de","subject":"Re: [PATCH 2/2] commit: don't check for space twice when looking for header","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-25T21:51:44Z","receivedAt":"2017-02-25T21:58:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Feb 25, 2017 at 10:39:29PM +0100, René Scharfe wrote:\n\n> > > +\t\t(len == 8 && !memcmp(field, \"encoding\", 8)));\n> > \n> > Unrelated, but this could probably be spelled with a macro and strlen()\n> > to avoid the magic numbers. It would probably be measurably slower for a\n> > compiler which doesn't pre-compute strlen() on a string literal, though.\n> \n> sizeof(string_constant) - 1 might be a better choice here than strlen().\n\nYeah. If you use a macro, that works. If it's an inline function you'd\nneed strlen(). That's a tradeoff we've already made in skip_prefix_mem()\nand strip_suffix(), but it's not like we expect this list to grow much,\nso it may not be worth fussing with.\n\n-Peff\n"},{"id":"312764","messageId":"xmqq8torxx15.fsf@gitster.mtv.corp.google.com","threadId":"45228","inReplyTo":"23989e76-24ba-90a4-91a9-9f66bfccb7c9@web.de","subject":"Re: [PATCH 1/2] commit: be more precise when searching for headers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-27T19:18:30Z","receivedAt":"2017-02-27T19:48:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> Search for a space character only within the current line in\n> read_commit_extra_header_lines() instead of searching in the whole\n> buffer (and possibly beyond, if it's not NUL-terminated) and then\n> discarding any results after the end of the current line.\n>\n> Signed-off-by: Rene Scharfe <l.s.r@web.de>\n> ---\n>  commit.c | 4 ++--\n>  1 file changed, 2 insertions(+), 2 deletions(-)\n\nMakes sense.\n\n> diff --git a/commit.c b/commit.c\n> index 2cf85158b4..173c6d3818 100644\n> --- a/commit.c\n> +++ b/commit.c\n> @@ -1354,8 +1354,8 @@ static struct commit_extra_header *read_commit_extra_header_lines(\n>  \t\tstrbuf_reset(&buf);\n>  \t\tit = NULL;\n>  \n> -\t\teof = strchr(line, ' ');\n> -\t\tif (next <= eof)\n> +\t\teof = memchr(line, ' ', next - line);\n> +\t\tif (!eof)\n>  \t\t\teof = next;\n>  \n>  \t\tif (standard_header_field(line, eof - line) ||\n"},{"id":"312791","messageId":"1dd0884c-032c-fb04-67f6-8b181fd65eea@gmail.com","threadId":"45228","inReplyTo":"b1d5c882-38b8-dd2d-2e5f-aafb8dfada81@web.de","subject":"Re: [PATCH 2/2] commit: don't check for space twice when looking for header","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2017-02-27T22:27:22Z","receivedAt":"2017-02-27T22:29:27Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"W dniu 25.02.2017 o 20:27, René Scharfe pisze:\n> Both standard_header_field() and excluded_header_field() check if\n> there's a space after the buffer that's handed to them.  We already\n> check in the caller if that space is present.  Don't bother calling\n> the functions if it's missing, as they are guaranteed to return 0 in\n> that case, and remove the now redundant checks from them.\n> \n> Signed-off-by: Rene Scharfe <l.s.r@web.de>\n> ---\n>  commit.c | 18 ++++++++----------\n>  1 file changed, 8 insertions(+), 10 deletions(-)\n> \n> diff --git a/commit.c b/commit.c\n> index 173c6d3818..fab8269731 100644\n> --- a/commit.c\n> +++ b/commit.c\n> @@ -1308,11 +1308,11 @@ void for_each_mergetag(each_mergetag_fn fn, struct commit *commit, void *data)\n>  \n>  static inline int standard_header_field(const char *field, size_t len)\n>  {\n> -\treturn ((len == 4 && !memcmp(field, \"tree \", 5)) ||\n> -\t\t(len == 6 && !memcmp(field, \"parent \", 7)) ||\n> -\t\t(len == 6 && !memcmp(field, \"author \", 7)) ||\n> -\t\t(len == 9 && !memcmp(field, \"committer \", 10)) ||\n> -\t\t(len == 8 && !memcmp(field, \"encoding \", 9)));\n> +\treturn ((len == 4 && !memcmp(field, \"tree\", 4)) ||\n> +\t\t(len == 6 && !memcmp(field, \"parent\", 6)) ||\n> +\t\t(len == 6 && !memcmp(field, \"author\", 6)) ||\n> +\t\t(len == 9 && !memcmp(field, \"committer\", 9)) ||\n> +\t\t(len == 8 && !memcmp(field, \"encoding\", 8)));\n\nI agree (for what it is worth from me) with the rest of changes,\nbut I think current code is better self-documenting for this\nfunction.\n\n>  }\n>  \n>  static int excluded_header_field(const char *field, size_t len, const char **exclude)\n> @@ -1322,8 +1322,7 @@ static int excluded_header_field(const char *field, size_t len, const char **exc\n>  \n>  \twhile (*exclude) {\n>  \t\tsize_t xlen = strlen(*exclude);\n> -\t\tif (len == xlen &&\n> -\t\t    !memcmp(field, *exclude, xlen) && field[xlen] == ' ')\n> +\t\tif (len == xlen && !memcmp(field, *exclude, xlen))\n>  \t\t\treturn 1;\n>  \t\texclude++;\n>  \t}\n> @@ -1357,9 +1356,8 @@ static struct commit_extra_header *read_commit_extra_header_lines(\n>  \t\teof = memchr(line, ' ', next - line);\n>  \t\tif (!eof)\n>  \t\t\teof = next;\n> -\n> -\t\tif (standard_header_field(line, eof - line) ||\n> -\t\t    excluded_header_field(line, eof - line, exclude))\n> +\t\telse if (standard_header_field(line, eof - line) ||\n> +\t\t\t excluded_header_field(line, eof - line, exclude))\n>  \t\t\tcontinue;\n>  \n>  \t\tit = xcalloc(1, sizeof(*it));\n> \n\n"},{"id":"312800","messageId":"cd9f3874-7428-9906-f805-31fe4661d4b6@web.de","threadId":"45228","inReplyTo":"1dd0884c-032c-fb04-67f6-8b181fd65eea@gmail.com","subject":"Re: [PATCH 2/2] commit: don't check for space twice when looking for header","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-02-27T22:54:22Z","receivedAt":"2017-02-27T22:57:27Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 27.02.2017 um 23:27 schrieb Jakub Narębski:\n> W dniu 25.02.2017 o 20:27, René Scharfe pisze:\n>> Both standard_header_field() and excluded_header_field() check if\n>> there's a space after the buffer that's handed to them.  We already\n>> check in the caller if that space is present.  Don't bother calling\n>> the functions if it's missing, as they are guaranteed to return 0 in\n>> that case, and remove the now redundant checks from them.\n>>\n>> Signed-off-by: Rene Scharfe <l.s.r@web.de>\n>> ---\n>>  commit.c | 18 ++++++++----------\n>>  1 file changed, 8 insertions(+), 10 deletions(-)\n>>\n>> diff --git a/commit.c b/commit.c\n>> index 173c6d3818..fab8269731 100644\n>> --- a/commit.c\n>> +++ b/commit.c\n>> @@ -1308,11 +1308,11 @@ void for_each_mergetag(each_mergetag_fn fn, struct commit *commit, void *data)\n>>\n>>  static inline int standard_header_field(const char *field, size_t len)\n>>  {\n>> -\treturn ((len == 4 && !memcmp(field, \"tree \", 5)) ||\n>> -\t\t(len == 6 && !memcmp(field, \"parent \", 7)) ||\n>> -\t\t(len == 6 && !memcmp(field, \"author \", 7)) ||\n>> -\t\t(len == 9 && !memcmp(field, \"committer \", 10)) ||\n>> -\t\t(len == 8 && !memcmp(field, \"encoding \", 9)));\n>> +\treturn ((len == 4 && !memcmp(field, \"tree\", 4)) ||\n>> +\t\t(len == 6 && !memcmp(field, \"parent\", 6)) ||\n>> +\t\t(len == 6 && !memcmp(field, \"author\", 6)) ||\n>> +\t\t(len == 9 && !memcmp(field, \"committer\", 9)) ||\n>> +\t\t(len == 8 && !memcmp(field, \"encoding\", 8)));\n>\n> I agree (for what it is worth from me) with the rest of changes,\n> but I think current code is better self-documenting for this\n> function.\n\nHaving a function that is given a buffer/length pair and accessing the \nbyte after it raises questions, though. :)\n\nNicer than keeping the space would be to use excluded_header_field() for \nstandard headers as well as a next step, I think -- but that would be a \nbit slower.\n\n>\n>>  }\n>>\n>>  static int excluded_header_field(const char *field, size_t len, const char **exclude)\n>> @@ -1322,8 +1322,7 @@ static int excluded_header_field(const char *field, size_t len, const char **exc\n>>\n>>  \twhile (*exclude) {\n>>  \t\tsize_t xlen = strlen(*exclude);\n>> -\t\tif (len == xlen &&\n>> -\t\t    !memcmp(field, *exclude, xlen) && field[xlen] == ' ')\n>> +\t\tif (len == xlen && !memcmp(field, *exclude, xlen))\n>>  \t\t\treturn 1;\n>>  \t\texclude++;\n>>  \t}\n>> @@ -1357,9 +1356,8 @@ static struct commit_extra_header *read_commit_extra_header_lines(\n>>  \t\teof = memchr(line, ' ', next - line);\n>>  \t\tif (!eof)\n>>  \t\t\teof = next;\n>> -\n>> -\t\tif (standard_header_field(line, eof - line) ||\n>> -\t\t    excluded_header_field(line, eof - line, exclude))\n>> +\t\telse if (standard_header_field(line, eof - line) ||\n>> +\t\t\t excluded_header_field(line, eof - line, exclude))\n>>  \t\t\tcontinue;\n>>\n>>  \t\tit = xcalloc(1, sizeof(*it));\n>>\n>\n"}]}