{"thread":{"id":"57155","subject":"[PATCH 0/2] Consolidate find_header logic into one function","startedAt":"2021-12-27T18:26:42Z","lastAt":"2022-01-08T07:11:13Z","messageCount":16,"participants":["John Cai via GitGitGadget","Junio C Hamano","John Cai"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"445003","messageId":"pull.1125.git.git.1640629598.gitgitgadget@gmail.com","threadId":"57155","inReplyTo":null,"subject":"[PATCH 0/2] Consolidate find_header logic into one function","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-12-27T18:26:36Z","receivedAt":"2021-12-27T18:26:42Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"This addresses the NEEDSWORK comment in builtin/receive-pack.c:\n\n /**\n * NEEDSWORK: reuse find_commit_header() from jk/commit-author-parsing\n * after dropping \"_commit\" from its name and possibly moving it out\n * of commit.c\n **/\n\n\nThere is some duplicated logic between find_header and find_commit_header\nthat can be consolidated instead of having two places in the code that do\nessentially the same thing. For the sake of simpler and more DRY code, use\nfind_commit_header and rename it to find_header since it is not limited to\nfinding headers for only commits.\n\nThis is my first attempt to address a NEEDSWORK issue, and I'm relatively\nnew to C so any feedback would be appreciated!\n\nJohn Cai (2):\n receive-pack.c: consolidate find header logic\n commit.c: rename find_commit_header to find_header\n\n builtin/am.c | 2 +-\n builtin/commit.c | 2 +-\n builtin/receive-pack.c | 48 ++++++++++++++++---------------------\n commit.c | 7 +++---\n commit.h | 2 +-\n gpg-interface.c | 2 +-\n pretty.c | 2 +-\n sequencer.c | 2 +-\n t/helper/test-fast-rebase.c | 2 +-\n 9 files changed, 32 insertions(+), 37 deletions(-)\n\n\nbase-commit: 2ae0a9cb8298185a94e5998086f380a355dd8907\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1125%2Fjohn-cai%2Fjc%2Freplace-find-header-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1125/john-cai/jc/replace-find-header-v1\nPull-Request: https://github.com/git/git/pull/1125\n-- \ngitgitgadget\n"},{"id":"445004","messageId":"9465c20d4bd398dacbd7df2c068513c9ec484dd8.1640629598.git.gitgitgadget@gmail.com","threadId":"57155","inReplyTo":"pull.1125.git.git.1640629598.gitgitgadget@gmail.com","subject":"[PATCH 1/2] receive-pack.c: consolidate find header logic","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-12-27T18:26:37Z","receivedAt":"2021-12-27T18:26:45Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai \n\nThere are two functions that have very similar logic of finding a header\nvalue. find_commit_header, and find_header. We can conslidate the logic\nby using find_commit_header and replacing the logic in find_header.\nThis helps clean up the code, as the logic for finding header values can\nstay in one place.\n\nSigned-off-by: John Cai \n---\n builtin/receive-pack.c | 48 ++++++++++++++++++------------------------\n commit.c | 3 ++-\n 2 files changed, 23 insertions(+), 28 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 4f92e6f059d..939d4b28b7c 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -581,32 +581,26 @@ static char *prepare_push_cert_nonce(const char *path, timestamp_t stamp)\n \treturn strbuf_detach(&buf, NULL);\n }\n \n-/*\n- * NEEDSWORK: reuse find_commit_header() from jk/commit-author-parsing\n- * after dropping \"_commit\" from its name and possibly moving it out\n- * of commit.c\n- */\n-static char *find_header(const char *msg, size_t len, const char *key,\n-\t\t\t const char **next_line)\n+static char *find_header_value(const char *msg, const char *key, const char **next_line)\n {\n-\tint key_len = strlen(key);\n-\tconst char *line = msg;\n-\n-\twhile (line && line < msg + len) {\n-\t\tconst char *eol = strchrnul(line, '\\n');\n-\n-\t\tif ((msg + len <= eol) || line == eol)\n-\t\t\treturn NULL;\n-\t\tif (line + key_len < eol &&\n-\t\t !memcmp(line, key, key_len) && line[key_len] == ' ') {\n-\t\t\tint offset = key_len + 1;\n-\t\t\tif (next_line)\n-\t\t\t\t*next_line = *eol ? eol + 1 : eol;\n-\t\t\treturn xmemdupz(line + offset, (eol - line) - offset);\n-\t\t}\n-\t\tline = *eol ? eol + 1 : NULL;\n+\tsize_t out_len;\n+\tconst char *eol;\n+\tchar *ret;\n+\n+\tconst char *val = find_commit_header(msg, key, &out_len);\n+\tif (val == NULL)\n+\t\treturn NULL;\n+\n+\teol = strchrnul(val, '\\n');\n+\tif (next_line) {\n+\t\t*next_line = *eol ? eol + 1: eol;\n \t}\n-\treturn NULL;\n+\n+\tret = xmalloc(out_len+1);\n+\tmemcpy(ret, val, out_len);\n+\tret[out_len] = '\\0';\n+\n+\treturn ret;\n }\n \n /*\n@@ -625,7 +619,8 @@ static int constant_memequal(const char *a, const char *b, size_t n)\n \n static const char *check_nonce(const char *buf, size_t len)\n {\n-\tchar *nonce = find_header(buf, len, \"nonce\", NULL);\n+\tchar *nonce = find_header_value(buf, \"nonce\", NULL);\n+\n \ttimestamp_t stamp, ostamp;\n \tchar *bohmac, *expect = NULL;\n \tconst char *retval = NONCE_BAD;\n@@ -730,8 +725,7 @@ static int check_cert_push_options(const struct string_list *push_options)\n \tif (!len)\n \t\treturn 1;\n \n-\twhile ((option = find_header(buf, len, \"push-option\", &next_line))) {\n-\t\tlen -= (next_line - buf);\n+\twhile ((option = find_header_value(buf, \"push-option\", &next_line))) {\n \t\tbuf = next_line;\n \t\toptions_seen++;\n \t\tif (options_seen > push_options->nr\ndiff --git a/commit.c b/commit.c\nindex a348f085b2b..616a6780703 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1645,7 +1645,8 @@ const char *find_commit_header(const char *msg, const char *key, size_t *out_len\n \t\tif (eol - line > key_len &&\n \t\t !strncmp(line, key, key_len) &&\n \t\t line[key_len] == ' ') {\n-\t\t\t*out_len = eol - line - key_len - 1;\n+\t\t\tif (out_len != NULL)\n+\t\t\t\t*out_len = eol - line - key_len - 1;\n \t\t\treturn line + key_len + 1;\n \t\t}\n \t\tline = *eol ? eol + 1 : NULL;\n-- \ngitgitgadget\n\n"},{"id":"445005","messageId":"384a635daa21871311ffadc81c1e9a8c56f474b1.1640629598.git.gitgitgadget@gmail.com","threadId":"57155","inReplyTo":"pull.1125.git.git.1640629598.gitgitgadget@gmail.com","subject":"[PATCH 2/2] commit.c: rename find_commit_header to find_header","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-12-27T18:26:38Z","receivedAt":"2021-12-27T18:26:46Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai \n\nSince find_commit_header's logic is not just limited to commit objects,\nwe can rename this to a more general find_header function so it's\nclearer that it can be used in a more general way.\n\nSigned-off-by: John Cai \n---\n builtin/am.c | 2 +-\n builtin/commit.c | 2 +-\n builtin/receive-pack.c | 8 ++++----\n commit.c | 4 ++--\n commit.h | 2 +-\n gpg-interface.c | 2 +-\n pretty.c | 2 +-\n sequencer.c | 2 +-\n t/helper/test-fast-rebase.c | 2 +-\n 9 files changed, 13 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 8677ea2348a..1ed5071ff49 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1312,7 +1312,7 @@ static void get_commit_info(struct am_state *state, struct commit *commit)\n \n \tbuffer = logmsg_reencode(commit, NULL, get_commit_output_encoding());\n \n-\tident_line = find_commit_header(buffer, \"author\", &ident_len);\n+\tident_line = find_header(buffer, \"author\", &ident_len);\n \tif (!ident_line)\n \t\tdie(_(\"missing author line in commit %s\"),\n \t\t oid_to_hex(&commit->object.oid));\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 883c16256c8..6fbb154ee32 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -614,7 +614,7 @@ static void determine_author_info(struct strbuf *author_ident)\n \t\tsize_t len;\n \t\tconst char *a;\n \n-\t\ta = find_commit_header(author_message_buffer, \"author\", &len);\n+\t\ta = find_header(author_message_buffer, \"author\", &len);\n \t\tif (!a)\n \t\t\tdie(_(\"commit '%s' lacks author header\"), author_message);\n \t\tif (split_ident_line(&ident, a, len) < 0)\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 939d4b28b7c..f0fc826b665 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -581,13 +581,13 @@ static char *prepare_push_cert_nonce(const char *path, timestamp_t stamp)\n \treturn strbuf_detach(&buf, NULL);\n }\n \n-static char *find_header_value(const char *msg, const char *key, const char **next_line)\n+static char *find_header_nextline(const char *msg, const char *key, const char **next_line)\n {\n \tsize_t out_len;\n \tconst char *eol;\n \tchar *ret;\n \n-\tconst char *val = find_commit_header(msg, key, &out_len);\n+\tconst char *val = find_header(msg, key, &out_len);\n \tif (val == NULL)\n \t\treturn NULL;\n \n@@ -619,7 +619,7 @@ static int constant_memequal(const char *a, const char *b, size_t n)\n \n static const char *check_nonce(const char *buf, size_t len)\n {\n-\tchar *nonce = find_header_value(buf, \"nonce\", NULL);\n+\tchar *nonce = find_header_nextline(buf, \"nonce\", NULL);\n \n \ttimestamp_t stamp, ostamp;\n \tchar *bohmac, *expect = NULL;\n@@ -725,7 +725,7 @@ static int check_cert_push_options(const struct string_list *push_options)\n \tif (!len)\n \t\treturn 1;\n \n-\twhile ((option = find_header_value(buf, \"push-option\", &next_line))) {\n+\twhile ((option = find_header_nextline(buf, \"push-option\", &next_line))) {\n \t\tbuf = next_line;\n \t\toptions_seen++;\n \t\tif (options_seen > push_options->nr\ndiff --git a/commit.c b/commit.c\nindex 616a6780703..bdedbe295f5 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -734,7 +734,7 @@ void record_author_date(struct author_date_slab *author_date,\n \tchar *date_end;\n \ttimestamp_t date;\n \n-\tident_line = find_commit_header(buffer, \"author\", &ident_len);\n+\tident_line = find_header(buffer, \"author\", &ident_len);\n \tif (!ident_line)\n \t\tgoto fail_exit; /* no author line */\n \tif (split_ident_line(&ident, ident_line, ident_len) ||\n@@ -1631,7 +1631,7 @@ struct commit_list **commit_list_append(struct commit *commit,\n \treturn &new_commit->next;\n }\n \n-const char *find_commit_header(const char *msg, const char *key, size_t *out_len)\n+const char *find_header(const char *msg, const char *key, size_t *out_len)\n {\n \tint key_len = strlen(key);\n \tconst char *line = msg;\ndiff --git a/commit.h b/commit.h\nindex 3ea32766bcb..18a1460c4b4 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -296,7 +296,7 @@ void free_commit_extra_headers(struct commit_extra_header *extra);\n * Note that some headers (like mergetag) may be multi-line. It is the caller's\n * responsibility to parse further in this case!\n */\n-const char *find_commit_header(const char *msg, const char *key,\n+const char *find_header(const char *msg, const char *key,\n \t\t\t size_t *out_len);\n \n /* Find the end of the log message, the right place for a new trailer. */\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex b52eb0e2e04..c1bc9fa0459 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -597,7 +597,7 @@ static int parse_payload_metadata(struct signature_check *sigc)\n \t\tBUG(\"invalid value for sigc->payload_type\");\n \t}\n \n-\tident_line = find_commit_header(sigc->payload, signer_header, &ident_len);\n+\tident_line = find_header(sigc->payload, signer_header, &ident_len);\n \tif (!ident_line || !ident_len)\n \t\treturn 1;\n \ndiff --git a/pretty.c b/pretty.c\nindex ee6114e3f0a..306c647d2d7 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -633,7 +633,7 @@ static void add_merge_info(const struct pretty_print_context *pp,\n static char *get_header(const char *msg, const char *key)\n {\n \tsize_t len;\n-\tconst char *v = find_commit_header(msg, key, &len);\n+\tconst char *v = find_header(msg, key, &len);\n \treturn v ? xmemdupz(v, len) : NULL;\n }\n \ndiff --git a/sequencer.c b/sequencer.c\nindex e314af4d60a..ac535868121 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -912,7 +912,7 @@ static char *get_author(const char *message)\n \tsize_t len;\n \tconst char *a;\n \n-\ta = find_commit_header(message, \"author\", &len);\n+\ta = find_header(message, \"author\", &len);\n \tif (a)\n \t\treturn xmemdupz(a, len);\n \ndiff --git a/t/helper/test-fast-rebase.c b/t/helper/test-fast-rebase.c\nindex fc2d4609043..576c2366258 100644\n--- a/t/helper/test-fast-rebase.c\n+++ b/t/helper/test-fast-rebase.c\n@@ -44,7 +44,7 @@ static char *get_author(const char *message)\n \tsize_t len;\n \tconst char *a;\n \n-\ta = find_commit_header(message, \"author\", &len);\n+\ta = find_header(message, \"author\", &len);\n \tif (a)\n \t\treturn xmemdupz(a, len);\n \n-- \ngitgitgadget\n"},{"id":"445018","messageId":"xmqqtuetvft6.fsf@gitster.g","threadId":"57155","inReplyTo":"9465c20d4bd398dacbd7df2c068513c9ec484dd8.1640629598.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] receive-pack.c: consolidate find header logic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-27T22:33:25Z","receivedAt":"2021-12-27T22:33:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"John Cai via GitGitGadget\" writes:\n\n> From: John Cai \n>\n> There are two functions that have very similar logic of finding a header\n> value. find_commit_header, and find_header. We can conslidate the logic\n\n\"consolidate\".\n\n> by using find_commit_header and replacing the logic in find_header.\n> This helps clean up the code, as the logic for finding header values can\n> stay in one place.\n\nIt does make sense to split the renaming and this change into two\nseparate steps like this series does. The renaming done in 2/2\nhowever makes readers wonder if our existing code paths that handle\ntag objects becomes cleaner by using the function (and if not, if\nthe perceived benefit of making it into a more generic name is a\nmere mirage), though.\n\n> Signed-off-by: John Cai \n> ---\n> builtin/receive-pack.c | 48 ++++++++++++++++++------------------------\n> commit.c | 3 ++-\n> 2 files changed, 23 insertions(+), 28 deletions(-)\n>\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index 4f92e6f059d..939d4b28b7c 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -581,32 +581,26 @@ static char *prepare_push_cert_nonce(const char *path, timestamp_t stamp)\n> \treturn strbuf_detach(&buf, NULL);\n> }\n> \n> -/*\n> - * NEEDSWORK: reuse find_commit_header() from jk/commit-author-parsing\n> - * after dropping \"_commit\" from its name and possibly moving it out\n> - * of commit.c\n> - */\n> -static char *find_header(const char *msg, size_t len, const char *key,\n> -\t\t\t const char **next_line)\n> +static char *find_header_value(const char *msg, const char *key, const char **next_line)\n\nI do not think this change is quite right. &msg[len] in the\noriginal may not be the end of a NUL-terminated string, i.e. the\ncaller does not want this helper to scan through to the end of the\nbuffer but wants it to stop much earlier at the \"len\" the caller\nspecifies.\n\n> {\n> -\tint key_len = strlen(key);\n> -\tconst char *line = msg;\n> -\n> -\twhile (line && line < msg + len) {\n> -\t\tconst char *eol = strchrnul(line, '\\n');\n> -\n> -\t\tif ((msg + len <= eol) || line == eol)\n> -\t\t\treturn NULL;\n> -\t\tif (line + key_len < eol &&\n> -\t\t !memcmp(line, key, key_len) && line[key_len] == ' ') {\n> -\t\t\tint offset = key_len + 1;\n> -\t\t\tif (next_line)\n> -\t\t\t\t*next_line = *eol ? eol + 1 : eol;\n> -\t\t\treturn xmemdupz(line + offset, (eol - line) - offset);\n> -\t\t}\n> -\t\tline = *eol ? eol + 1 : NULL;\n> +\tsize_t out_len;\n> +\tconst char *eol;\n> +\tchar *ret;\n> +\n> +\tconst char *val = find_commit_header(msg, key, &out_len);\n> +\tif (val == NULL)\n> +\t\treturn NULL;\n> +\n> +\teol = strchrnul(val, '\\n');\n> +\tif (next_line) {\n> +\t\t*next_line = *eol ? eol + 1: eol;\n\nAlso, find_commit_header() has already figured out what the next\nline should be. If it is not just telling us, we are forced to\nrecompute it with an extra strchrnul(), but is that really the case?\n\nHOWEVER.\n\nDoesn't out_len have enough information to let us compute next_line\nwithout scanning the line again?\n\n> -\treturn NULL;\n> +\n> +\tret = xmalloc(out_len+1);\n> +\tmemcpy(ret, val, out_len);\n> +\tret[out_len] = '\\0';\n\nIn any case, it is not necessary to open code xmemdupz() into these\nthree lines, no?\n"},{"id":"445164","messageId":"pull.1125.v2.git.git.1640758765723.gitgitgadget@gmail.com","threadId":"57155","inReplyTo":"pull.1125.git.git.1640629598.gitgitgadget@gmail.com","subject":"[PATCH v2] receive-pack.c: consolidate find header logic","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-12-29T06:19:25Z","receivedAt":"2021-12-29T06:19:30Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai \n\nThere are two functions that have very similar logic of finding a header\nvalue. find_commit_header, and find_header. We can conslidate the logic\nby using find_commit_header and replacing the logic in find_header.\n\nIntroduce a new function find_header_max, which is equivalent to\nfind_commit_header except it takes a len parameter that determines how\nmany bytes to read. find_commit_header can then call find_header_max\nwith 0 as the len.\n\nThis cleans up duplicate logic, as the logic for finding header values\nis now all in one place.\n\nSigned-off-by: John Cai \n---\n Consolidate find_header logic into one function\n \n This addresses the NEEDSWORK comment in builtin/receive-pack.c:\n \n /**\n * NEEDSWORK: reuse find_commit_header() from jk/commit-author-parsing\n * after dropping \"_commit\" from its name and possibly moving it out\n * of commit.c\n **/\n \n \n There is some duplicated logic between find_header and\n find_commit_header that can be consolidated instead of having two places\n in the code that do essentially the same thing. For the sake of simpler\n and more DRY code, use find_commit_header and rename it to find_header\n since it is not limited to finding headers for only commits.\n \n Changes since v1:\n \n * got rid of renaming from find_commit_header -> find_header since the\n renaming did not provide much value\n * simplified logic in find_header\n * introduce find_header_max function, which is what find_commit_header\n was before except it adds a \"len\" parameter to determine how much of\n the buffer to read.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1125%2Fjohn-cai%2Fjc%2Freplace-find-header-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1125/john-cai/jc/replace-find-header-v2\nPull-Request: https://github.com/git/git/pull/1125\n\nRange-diff vs v1:\n\n 1: 9465c20d4bd ! 1: 5e0d90d642b receive-pack.c: consolidate find header logic\n @@ Commit message\n There are two functions that have very similar logic of finding a header\n value. find_commit_header, and find_header. We can conslidate the logic\n by using find_commit_header and replacing the logic in find_header.\n - This helps clean up the code, as the logic for finding header values can\n - stay in one place.\n +\n + Introduce a new function find_header_max, which is equivalent to\n + find_commit_header except it takes a len parameter that determines how\n + many bytes to read. find_commit_header can then call find_header_max\n + with 0 as the len.\n +\n + This cleans up duplicate logic, as the logic for finding header values\n + is now all in one place.\n \n Signed-off-by: John Cai \n \n @@ builtin/receive-pack.c: static char *prepare_push_cert_nonce(const char *path, t\n - * after dropping \"_commit\" from its name and possibly moving it out\n - * of commit.c\n - */\n --static char *find_header(const char *msg, size_t len, const char *key,\n --\t\t\t const char **next_line)\n -+static char *find_header_value(const char *msg, const char *key, const char **next_line)\n + static char *find_header(const char *msg, size_t len, const char *key,\n + \t\t\t const char **next_line)\n {\n -\tint key_len = strlen(key);\n -\tconst char *line = msg;\n @@ builtin/receive-pack.c: static char *prepare_push_cert_nonce(const char *path, t\n -\t\t\treturn xmemdupz(line + offset, (eol - line) - offset);\n -\t\t}\n -\t\tline = *eol ? eol + 1 : NULL;\n +-\t}\n +-\treturn NULL;\n +\tsize_t out_len;\n -+\tconst char *eol;\n -+\tchar *ret;\n ++\tconst char *val = find_header_max(msg, key, len, &out_len);\n +\n -+\tconst char *val = find_commit_header(msg, key, &out_len);\n +\tif (val == NULL)\n +\t\treturn NULL;\n +\n -+\teol = strchrnul(val, '\\n');\n -+\tif (next_line) {\n -+\t\t*next_line = *eol ? eol + 1: eol;\n - \t}\n --\treturn NULL;\n -+\n -+\tret = xmalloc(out_len+1);\n -+\tmemcpy(ret, val, out_len);\n -+\tret[out_len] = '\\0';\n ++\tif (next_line)\n ++\t\t*next_line = val + out_len + 1;\n +\n -+\treturn ret;\n ++\treturn xmemdupz(val, out_len);\n }\n \n /*\n -@@ builtin/receive-pack.c: static int constant_memequal(const char *a, const char *b, size_t n)\n +\n + ## commit.c ##\n +@@ commit.c: struct commit_list **commit_list_append(struct commit *commit,\n + \treturn &new_commit->next;\n + }\n \n - static const char *check_nonce(const char *buf, size_t len)\n +-const char *find_commit_header(const char *msg, const char *key, size_t *out_len)\n ++const char *find_header_max(const char *msg, const char *key,\n ++\t\t\tsize_t len,\n ++\t\t\tsize_t *out_len)\n {\n --\tchar *nonce = find_header(buf, len, \"nonce\", NULL);\n -+\tchar *nonce = find_header_value(buf, \"nonce\", NULL);\n -+\n - \ttimestamp_t stamp, ostamp;\n - \tchar *bohmac, *expect = NULL;\n - \tconst char *retval = NONCE_BAD;\n -@@ builtin/receive-pack.c: static int check_cert_push_options(const struct string_list *push_options)\n - \tif (!len)\n - \t\treturn 1;\n + \tint key_len = strlen(key);\n + \tconst char *line = msg;\n \n --\twhile ((option = find_header(buf, len, \"push-option\", &next_line))) {\n --\t\tlen -= (next_line - buf);\n -+\twhile ((option = find_header_value(buf, \"push-option\", &next_line))) {\n - \t\tbuf = next_line;\n - \t\toptions_seen++;\n - \t\tif (options_seen > push_options->nr\n -\n - ## commit.c ##\n +-\twhile (line) {\n ++\twhile (line && (len == 0 || line < msg + len)) {\n + \t\tconst char *eol = strchrnul(line, '\\n');\n + \n + \t\tif (line == eol)\n @@ commit.c: const char *find_commit_header(const char *msg, const char *key, size_t *out_len\n - \t\tif (eol - line > key_len &&\n - \t\t !strncmp(line, key, key_len) &&\n - \t\t line[key_len] == ' ') {\n --\t\t\t*out_len = eol - line - key_len - 1;\n -+\t\t\tif (out_len != NULL)\n -+\t\t\t\t*out_len = eol - line - key_len - 1;\n - \t\t\treturn line + key_len + 1;\n - \t\t}\n - \t\tline = *eol ? eol + 1 : NULL;\n + \treturn NULL;\n + }\n + \n ++const char *find_commit_header(const char *msg, const char *key, size_t *out_len)\n ++{\n ++\treturn find_header_max(msg, key, 0, out_len);\n ++}\n + /*\n + * Inspect the given string and determine the true \"end\" of the log message, in\n + * order to find where to put a new Signed-off-by trailer. Ignored are\n +\n + ## commit.h ##\n +@@ commit.h: void free_commit_extra_headers(struct commit_extra_header *extra);\n + \n + /*\n + * Search the commit object contents given by \"msg\" for the header \"key\".\n ++ * Reads up to \"len\" bytes of \"msg\".\n + * Returns a pointer to the start of the header contents, or NULL. The length\n + * of the header, up to the first newline, is returned via out_len.\n + *\n + * Note that some headers (like mergetag) may be multi-line. It is the caller's\n + * responsibility to parse further in this case!\n + */\n ++const char *find_header_max(const char *msg, const char *key,\n ++\t\t\tsize_t len,\n ++\t\t\tsize_t *out_len);\n ++\n + const char *find_commit_header(const char *msg, const char *key,\n + \t\t\t size_t *out_len);\n + \n 2: 384a635daa2 < -: ----------- commit.c: rename find_commit_header to find_header\n\n\n builtin/receive-pack.c | 33 ++++++++++-----------------------\n commit.c | 10 ++++++++--\n commit.h | 5 +++++\n 3 files changed, 23 insertions(+), 25 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 9f4a0b816cf..b69ead8dcda 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -581,32 +581,19 @@ static char *prepare_push_cert_nonce(const char *path, timestamp_t stamp)\n \treturn strbuf_detach(&buf, NULL);\n }\n \n-/*\n- * NEEDSWORK: reuse find_commit_header() from jk/commit-author-parsing\n- * after dropping \"_commit\" from its name and possibly moving it out\n- * of commit.c\n- */\n static char *find_header(const char *msg, size_t len, const char *key,\n \t\t\t const char **next_line)\n {\n-\tint key_len = strlen(key);\n-\tconst char *line = msg;\n-\n-\twhile (line && line < msg + len) {\n-\t\tconst char *eol = strchrnul(line, '\\n');\n-\n-\t\tif ((msg + len <= eol) || line == eol)\n-\t\t\treturn NULL;\n-\t\tif (line + key_len < eol &&\n-\t\t !memcmp(line, key, key_len) && line[key_len] == ' ') {\n-\t\t\tint offset = key_len + 1;\n-\t\t\tif (next_line)\n-\t\t\t\t*next_line = *eol ? eol + 1 : eol;\n-\t\t\treturn xmemdupz(line + offset, (eol - line) - offset);\n-\t\t}\n-\t\tline = *eol ? eol + 1 : NULL;\n-\t}\n-\treturn NULL;\n+\tsize_t out_len;\n+\tconst char *val = find_header_max(msg, key, len, &out_len);\n+\n+\tif (val == NULL)\n+\t\treturn NULL;\n+\n+\tif (next_line)\n+\t\t*next_line = val + out_len + 1;\n+\n+\treturn xmemdupz(val, out_len);\n }\n \n /*\ndiff --git a/commit.c b/commit.c\nindex a348f085b2b..2ed115e04a0 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1631,12 +1631,14 @@ struct commit_list **commit_list_append(struct commit *commit,\n \treturn &new_commit->next;\n }\n \n-const char *find_commit_header(const char *msg, const char *key, size_t *out_len)\n+const char *find_header_max(const char *msg, const char *key,\n+\t\t\tsize_t len,\n+\t\t\tsize_t *out_len)\n {\n \tint key_len = strlen(key);\n \tconst char *line = msg;\n \n-\twhile (line) {\n+\twhile (line && (len == 0 || line < msg + len)) {\n \t\tconst char *eol = strchrnul(line, '\\n');\n \n \t\tif (line == eol)\n@@ -1653,6 +1655,10 @@ const char *find_commit_header(const char *msg, const char *key, size_t *out_len\n \treturn NULL;\n }\n \n+const char *find_commit_header(const char *msg, const char *key, size_t *out_len)\n+{\n+\treturn find_header_max(msg, key, 0, out_len);\n+}\n /*\n * Inspect the given string and determine the true \"end\" of the log message, in\n * order to find where to put a new Signed-off-by trailer. Ignored are\ndiff --git a/commit.h b/commit.h\nindex 3ea32766bcb..41ec89af5b5 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -290,12 +290,17 @@ void free_commit_extra_headers(struct commit_extra_header *extra);\n \n /*\n * Search the commit object contents given by \"msg\" for the header \"key\".\n+ * Reads up to \"len\" bytes of \"msg\".\n * Returns a pointer to the start of the header contents, or NULL. The length\n * of the header, up to the first newline, is returned via out_len.\n *\n * Note that some headers (like mergetag) may be multi-line. It is the caller's\n * responsibility to parse further in this case!\n */\n+const char *find_header_max(const char *msg, const char *key,\n+\t\t\tsize_t len,\n+\t\t\tsize_t *out_len);\n+\n const char *find_commit_header(const char *msg, const char *key,\n \t\t\t size_t *out_len);\n \n\nbase-commit: 55b058a8bbcc54bd93c733035c995abc7967e539\n-- \ngitgitgadget\n"},{"id":"445249","messageId":"xmqq1r1tofyf.fsf@gitster.g","threadId":"57155","inReplyTo":"pull.1125.v2.git.git.1640758765723.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] receive-pack.c: consolidate find header logic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-30T23:01:12Z","receivedAt":"2021-12-30T23:01:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"John Cai via GitGitGadget\" writes:\n\n> From: John Cai \n>\n> There are two functions that have very similar logic of finding a header\n> value. find_commit_header, and find_header. We can conslidate the logic\n> by using find_commit_header and replacing the logic in find_header.\n>\n> Introduce a new function find_header_max, which is equivalent to\n> find_commit_header except it takes a len parameter that determines how\n> many bytes to read. find_commit_header can then call find_header_max\n> with 0 as the len.\n\nfind_header_max() is not the name of the function that finds the\nlargest header? That is misleading.\n\n defines a few helper functions that take a\ncounted string, and they are named with _mem() suffix after the name\nof their NUL-terminated counterparts (if exists). skip_prefix() has\nskip_prefix_mem(), strip_suffix() has strip_suffix_mem().\n\nfind_header_mem() or something along that line, perhaps?\n\n> diff --git a/commit.c b/commit.c\n> index a348f085b2b..2ed115e04a0 100644\n> --- a/commit.c\n> +++ b/commit.c\n> @@ -1631,12 +1631,14 @@ struct commit_list **commit_list_append(struct commit *commit,\n> \treturn &new_commit->next;\n> }\n> \n> -const char *find_commit_header(const char *msg, const char *key, size_t *out_len)\n> +const char *find_header_max(const char *msg, const char *key,\n> +\t\t\tsize_t len,\n> +\t\t\tsize_t *out_len)\n\nIf is meant to be the length part of pair, we should\nhave it immediately after the parameter.\n\n\tfind_header_mem(const char *msg, size_t len,\n \t\tconst char *key, size_t *out_len)\n\nThat makes it clear to the readers that are close friends.\n\nAlso, I have a feeling that ...\n\n> {\n> \tint key_len = strlen(key);\n> \tconst char *line = msg;\n> \n> -\twhile (line) {\n> +\twhile (line && (len == 0 || line < msg + len)) {\n\n... we do not want this special casing of \"if !len\". By making the\ncaller responsible to _always_ supply the length of msg, we can lose\nthe conditional.\n\n> \t\tconst char *eol = strchrnul(line, '\\n');\n> \n> \t\tif (line == eol)\n> @@ -1653,6 +1655,10 @@ const char *find_commit_header(const char *msg, const char *key, size_t *out_len\n> \treturn NULL;\n> }\n> \n> +const char *find_commit_header(const char *msg, const char *key, size_t *out_len)\n> +{\n> +\treturn find_header_max(msg, key, 0, out_len);\n\nI.e. find_header_mem(msg, strlen(msg), key, out_len);\n\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index 9f4a0b816cf..b69ead8dcda 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -581,32 +581,19 @@ static char *prepare_push_cert_nonce(const char *path, timestamp_t stamp)\n> \treturn strbuf_detach(&buf, NULL);\n> }\n> \n> -/*\n> - * NEEDSWORK: reuse find_commit_header() from jk/commit-author-parsing\n> - * after dropping \"_commit\" from its name and possibly moving it out\n> - * of commit.c\n> - */\n> static char *find_header(const char *msg, size_t len, const char *key,\n> \t\t\t const char **next_line)\n> {\n> +\tsize_t out_len;\n> +\tconst char *val = find_header_max(msg, key, len, &out_len);\n> +\n> +\tif (val == NULL)\n> +\t\treturn NULL;\n> +\n> +\tif (next_line)\n> +\t\t*next_line = val + out_len + 1;\n> +\n> +\treturn xmemdupz(val, out_len);\n> }\n\nYup, something along that line. Note that find_header() does make\nit clear that the parameters form a pair. We want to do\nthe same for the new helper.\n"},{"id":"445279","messageId":"pull.1125.v3.git.git.1640931460758.gitgitgadget@gmail.com","threadId":"57155","inReplyTo":"pull.1125.v2.git.git.1640758765723.gitgitgadget@gmail.com","subject":"[PATCH v3] receive-pack.c: consolidate find header logic","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-12-31T06:17:40Z","receivedAt":"2021-12-31T06:17:49Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai \n\nThere are two functions that have very similar logic of finding a header\nvalue. find_commit_header, and find_header. We can conslidate the logic\nby using find_commit_header and replacing the logic in find_header.\n\nIntroduce a new function find_header_max, which is equivalent to\nfind_commit_header except it takes a len parameter that determines how\nmany bytes to read. find_commit_header can then call find_header_max\nwith 0 as the len.\n\nThis cleans up duplicate logic, as the logic for finding header values\nis now all in one place.\n\nSigned-off-by: John Cai \n---\n Consolidate find_header logic into one function\n \n This addresses the NEEDSWORK comment in builtin/receive-pack.c:\n \n /**\n * NEEDSWORK: reuse find_commit_header() from jk/commit-author-parsing\n * after dropping \"_commit\" from its name and possibly moving it out\n * of commit.c\n **/\n \n \n There is some duplicated logic between find_header and\n find_commit_header that can be consolidated instead of having two places\n in the code that do essentially the same thing. For the sake of simpler\n and more DRY code, use find_commit_header and rename it to find_header\n since it is not limited to finding headers for only commits.\n \n Changes since v2:\n \n * s/find_header_max/find_header_mem\n * moved \"len\" argument right next to \"msg\" agument in find_header_mem\n * removed special condition in find_header_mem to check for line == 0\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1125%2Fjohn-cai%2Fjc%2Freplace-find-header-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1125/john-cai/jc/replace-find-header-v3\nPull-Request: https://github.com/git/git/pull/1125\n\nRange-diff vs v2:\n\n 1: 5e0d90d642b ! 1: 463df42e7e0 receive-pack.c: consolidate find header logic\n @@ builtin/receive-pack.c: static char *prepare_push_cert_nonce(const char *path, t\n -\t}\n -\treturn NULL;\n +\tsize_t out_len;\n -+\tconst char *val = find_header_max(msg, key, len, &out_len);\n ++\tconst char *val = find_header_mem(msg, len, key, &out_len);\n +\n +\tif (val == NULL)\n +\t\treturn NULL;\n @@ commit.c: struct commit_list **commit_list_append(struct commit *commit,\n }\n \n -const char *find_commit_header(const char *msg, const char *key, size_t *out_len)\n -+const char *find_header_max(const char *msg, const char *key,\n -+\t\t\tsize_t len,\n -+\t\t\tsize_t *out_len)\n ++const char *find_header_mem(const char *msg, size_t len,\n ++\t\t\tconst char *key, size_t *out_len)\n {\n \tint key_len = strlen(key);\n \tconst char *line = msg;\n \n -\twhile (line) {\n -+\twhile (line && (len == 0 || line < msg + len)) {\n ++\twhile (line && line < msg + len) {\n \t\tconst char *eol = strchrnul(line, '\\n');\n \n \t\tif (line == eol)\n @@ commit.c: const char *find_commit_header(const char *msg, const char *key, size_\n \n +const char *find_commit_header(const char *msg, const char *key, size_t *out_len)\n +{\n -+\treturn find_header_max(msg, key, 0, out_len);\n ++\treturn find_header_mem(msg, strlen(msg), key, out_len);\n +}\n /*\n * Inspect the given string and determine the true \"end\" of the log message, in\n @@ commit.h: void free_commit_extra_headers(struct commit_extra_header *extra);\n * Note that some headers (like mergetag) may be multi-line. It is the caller's\n * responsibility to parse further in this case!\n */\n -+const char *find_header_max(const char *msg, const char *key,\n -+\t\t\tsize_t len,\n ++const char *find_header_mem(const char *msg, size_t len,\n ++\t\t\tconst char *key,\n +\t\t\tsize_t *out_len);\n +\n const char *find_commit_header(const char *msg, const char *key,\n\n\n builtin/receive-pack.c | 33 ++++++++++-----------------------\n commit.c | 9 +++++++--\n commit.h | 5 +++++\n 3 files changed, 22 insertions(+), 25 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 9f4a0b816cf..b661c51a538 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -581,32 +581,19 @@ static char *prepare_push_cert_nonce(const char *path, timestamp_t stamp)\n \treturn strbuf_detach(&buf, NULL);\n }\n \n-/*\n- * NEEDSWORK: reuse find_commit_header() from jk/commit-author-parsing\n- * after dropping \"_commit\" from its name and possibly moving it out\n- * of commit.c\n- */\n static char *find_header(const char *msg, size_t len, const char *key,\n \t\t\t const char **next_line)\n {\n-\tint key_len = strlen(key);\n-\tconst char *line = msg;\n-\n-\twhile (line && line < msg + len) {\n-\t\tconst char *eol = strchrnul(line, '\\n');\n-\n-\t\tif ((msg + len <= eol) || line == eol)\n-\t\t\treturn NULL;\n-\t\tif (line + key_len < eol &&\n-\t\t !memcmp(line, key, key_len) && line[key_len] == ' ') {\n-\t\t\tint offset = key_len + 1;\n-\t\t\tif (next_line)\n-\t\t\t\t*next_line = *eol ? eol + 1 : eol;\n-\t\t\treturn xmemdupz(line + offset, (eol - line) - offset);\n-\t\t}\n-\t\tline = *eol ? eol + 1 : NULL;\n-\t}\n-\treturn NULL;\n+\tsize_t out_len;\n+\tconst char *val = find_header_mem(msg, len, key, &out_len);\n+\n+\tif (val == NULL)\n+\t\treturn NULL;\n+\n+\tif (next_line)\n+\t\t*next_line = val + out_len + 1;\n+\n+\treturn xmemdupz(val, out_len);\n }\n \n /*\ndiff --git a/commit.c b/commit.c\nindex a348f085b2b..8ac32a4d7b5 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1631,12 +1631,13 @@ struct commit_list **commit_list_append(struct commit *commit,\n \treturn &new_commit->next;\n }\n \n-const char *find_commit_header(const char *msg, const char *key, size_t *out_len)\n+const char *find_header_mem(const char *msg, size_t len,\n+\t\t\tconst char *key, size_t *out_len)\n {\n \tint key_len = strlen(key);\n \tconst char *line = msg;\n \n-\twhile (line) {\n+\twhile (line && line < msg + len) {\n \t\tconst char *eol = strchrnul(line, '\\n');\n \n \t\tif (line == eol)\n@@ -1653,6 +1654,10 @@ const char *find_commit_header(const char *msg, const char *key, size_t *out_len\n \treturn NULL;\n }\n \n+const char *find_commit_header(const char *msg, const char *key, size_t *out_len)\n+{\n+\treturn find_header_mem(msg, strlen(msg), key, out_len);\n+}\n /*\n * Inspect the given string and determine the true \"end\" of the log message, in\n * order to find where to put a new Signed-off-by trailer. Ignored are\ndiff --git a/commit.h b/commit.h\nindex 3ea32766bcb..38cc5426615 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -290,12 +290,17 @@ void free_commit_extra_headers(struct commit_extra_header *extra);\n \n /*\n * Search the commit object contents given by \"msg\" for the header \"key\".\n+ * Reads up to \"len\" bytes of \"msg\".\n * Returns a pointer to the start of the header contents, or NULL. The length\n * of the header, up to the first newline, is returned via out_len.\n *\n * Note that some headers (like mergetag) may be multi-line. It is the caller's\n * responsibility to parse further in this case!\n */\n+const char *find_header_mem(const char *msg, size_t len,\n+\t\t\tconst char *key,\n+\t\t\tsize_t *out_len);\n+\n const char *find_commit_header(const char *msg, const char *key,\n \t\t\t size_t *out_len);\n \n\nbase-commit: 55b058a8bbcc54bd93c733035c995abc7967e539\n-- \ngitgitgadget\n"},{"id":"445376","messageId":"xmqq35m4gt5x.fsf@gitster.g","threadId":"57155","inReplyTo":"pull.1125.v3.git.git.1640931460758.gitgitgadget@gmail.com","subject":"Re: [PATCH v3] receive-pack.c: consolidate find header logic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-04T01:56:42Z","receivedAt":"2022-01-04T01:56:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"John Cai via GitGitGadget\" writes:\n\n> +\tsize_t out_len;\n> +\tconst char *val = find_header_mem(msg, len, key, &out_len);\n> +\n> +\tif (val == NULL)\n\nStyle:\n\n\tif (!val)\n\n> +\t\treturn NULL;\n> +\n> +\tif (next_line)\n> +\t\t*next_line = val + out_len + 1;\n> +\n> +\treturn xmemdupz(val, out_len);\n> }\n> \n> /*\n> diff --git a/commit.c b/commit.c\n> index a348f085b2b..8ac32a4d7b5 100644\n> --- a/commit.c\n> +++ b/commit.c\n> @@ -1631,12 +1631,13 @@ struct commit_list **commit_list_append(struct commit *commit,\n> \treturn &new_commit->next;\n> }\n> \n> -const char *find_commit_header(const char *msg, const char *key, size_t *out_len)\n> +const char *find_header_mem(const char *msg, size_t len,\n> +\t\t\tconst char *key, size_t *out_len)\n> {\n> \tint key_len = strlen(key);\n> \tconst char *line = msg;\n> \n> +\twhile (line && line < msg + len) {\n> \t\tconst char *eol = strchrnul(line, '\\n');\n\nBetween line[0] and msg[len], there may not be a LF nor NUL at all,\nand strchrnul() will scan beyond the range we were given (which is\nmsg[0]..msg[len]).\n\nBut that is something we share with the find_header() if I am not\nmistaken, so I am OK to leave the code as posted and leave it\noutside the scope of this series to clean it up to make it safer.\n\nThe reason why it is probably safe for the current set of callers\n(and presumably any reasonable new callers we may add later) is that\nthey computed \"len\" by scanning the block of memory starting at (or\nbefore) \"msg\" before calling us, and we know that the block of\nmemory is properly NUL-terminated. find_header() is called by\ncheck_nonce() and check_cert_push_options(), both of which tell\nus to scan in a strbuf, which is designed to be scannable for NUL\nsafely by having an extra NUL at the end beyond the end.\n\n"},{"id":"445422","messageId":"CAE7F284-4D3F-4412-ACC1-216BB93F74EB@gmail.com","threadId":"57155","inReplyTo":"xmqq35m4gt5x.fsf@gitster.g","subject":"Re: [PATCH v3] receive-pack.c: consolidate find header logic","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2022-01-04T15:12:11Z","receivedAt":"2022-01-04T15:12:27Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"\n\n> On Jan 3, 2022, at 8:56 PM, Junio C Hamano wrote:\n> \n> \"John Cai via GitGitGadget\" writes:\n> \n>> +\tsize_t out_len;\n>> +\tconst char *val = find_header_mem(msg, len, key, &out_len);\n>> +\n>> +\tif (val == NULL)\n> \n> Style:\n> \n> \tif (!val)\n> \n>> +\t\treturn NULL;\n>> +\n>> +\tif (next_line)\n>> +\t\t*next_line = val + out_len + 1;\n>> +\n>> +\treturn xmemdupz(val, out_len);\n>> }\n>> \n>> /*\n>> diff --git a/commit.c b/commit.c\n>> index a348f085b2b..8ac32a4d7b5 100644\n>> --- a/commit.c\n>> +++ b/commit.c\n>> @@ -1631,12 +1631,13 @@ struct commit_list **commit_list_append(struct commit *commit,\n>> \treturn &new_commit->next;\n>> }\n>> \n>> -const char *find_commit_header(const char *msg, const char *key, size_t *out_len)\n>> +const char *find_header_mem(const char *msg, size_t len,\n>> +\t\t\tconst char *key, size_t *out_len)\n>> {\n>> \tint key_len = strlen(key);\n>> \tconst char *line = msg;\n>> \n>> +\twhile (line && line < msg + len) {\n>> \t\tconst char *eol = strchrnul(line, '\\n');\n> \n> Between line[0] and msg[len], there may not be a LF nor NUL at all,\n> and strchrnul() will scan beyond the range we were given (which is\n> msg[0]..msg[len]).\n> \n> But that is something we share with the find_header() if I am not\n> mistaken, so I am OK to leave the code as posted and leave it\n> outside the scope of this series to clean it up to make it safer.\n\nGood catch. Thanks for pointing that out-I didn’t notice it. I’ve added a NEEDSWORK\nBlock to this so we can address it in a later patch series.\n\n> \n> The reason why it is probably safe for the current set of callers\n> (and presumably any reasonable new callers we may add later) is that\n> they computed \"len\" by scanning the block of memory starting at (or\n> before) \"msg\" before calling us, and we know that the block of\n> memory is properly NUL-terminated. find_header() is called by\n> check_nonce() and check_cert_push_options(), both of which tell\n> us to scan in a strbuf, which is designed to be scannable for NUL\n> safely by having an extra NUL at the end beyond the end.\n> \n\n"},{"id":"445524","messageId":"pull.1125.v4.git.git.1641396098056.gitgitgadget@gmail.com","threadId":"57155","inReplyTo":"pull.1125.v3.git.git.1640931460758.gitgitgadget@gmail.com","subject":"[PATCH v4] receive-pack.c: consolidate find header logic","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-01-05T15:21:37Z","receivedAt":"2022-01-05T15:21:44Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai \n\nThere are two functions that have very similar logic of finding a header\nvalue. find_commit_header, and find_header. We can conslidate the logic\nby introducing a new function find_header_mem, which is equivalent to\nfind_commit_header except it takes a len parameter that determines how\nmany bytes will be read. find_commit_header and find_header can then both\ncall find_header_mem.\n\nThis reduces duplicate logic, as the logic for finding header values\ncan now all live in one place.\n\nSigned-off-by: John Cai \n---\n Consolidate find_header logic into one function\n \n This addresses the NEEDSWORK comment in builtin/receive-pack.c:\n \n /**\n * NEEDSWORK: reuse find_commit_header() from jk/commit-author-parsing\n * after dropping \"_commit\" from its name and possibly moving it out\n * of commit.c\n **/\n \n \n There are two functions that have very similar logic of finding a header\n value. find_commit_header, and find_header. We can conslidate the logic\n by introducing a new function find_header_mem, which is equivalent to\n find_commit_header except it takes a len parameter that determines how\n many bytes will be read. find_commit_header and find_header can then\n both call find_header_mem.\n \n This reduces duplicate logic, as the logic for finding header values can\n now all live in one place.\n \n Changes since v4:\n \n * added NEEDSWORK block detailing what needs to be done to clean up\n find_header_mem\n \n Changes since v3:\n \n * fixed verbiage in commit message\n * adjusted style of an if block (based on Junio's feedback)\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1125%2Fjohn-cai%2Fjc%2Freplace-find-header-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1125/john-cai/jc/replace-find-header-v4\nPull-Request: https://github.com/git/git/pull/1125\n\nRange-diff vs v3:\n\n 1: 463df42e7e0 ! 1: a7b00022b00 receive-pack.c: consolidate find header logic\n @@ Commit message\n \n There are two functions that have very similar logic of finding a header\n value. find_commit_header, and find_header. We can conslidate the logic\n - by using find_commit_header and replacing the logic in find_header.\n -\n - Introduce a new function find_header_max, which is equivalent to\n + by introducing a new function find_header_mem, which is equivalent to\n find_commit_header except it takes a len parameter that determines how\n - many bytes to read. find_commit_header can then call find_header_max\n - with 0 as the len.\n + many bytes will be read. find_commit_header and find_header can then both\n + call find_header_mem.\n \n - This cleans up duplicate logic, as the logic for finding header values\n - is now all in one place.\n + This reduces duplicate logic, as the logic for finding header values\n + can now all live in one place.\n \n Signed-off-by: John Cai \n \n @@ builtin/receive-pack.c: static char *prepare_push_cert_nonce(const char *path, t\n +\tsize_t out_len;\n +\tconst char *val = find_header_mem(msg, len, key, &out_len);\n +\n -+\tif (val == NULL)\n ++\tif (!val)\n +\t\treturn NULL;\n +\n +\tif (next_line)\n @@ commit.c: struct commit_list **commit_list_append(struct commit *commit,\n \tconst char *line = msg;\n \n -\twhile (line) {\n ++\t/*\n ++\t * NEEDSWORK: Between line[0] and msg[len], there may not be a LF nor NUL\n ++\t * at all, and strchrnul() will scan beyond the range we were given\n ++\t * Make this operation safer and abide by the contract to only read up to len.\n ++\t */\n +\twhile (line && line < msg + len) {\n \t\tconst char *eol = strchrnul(line, '\\n');\n \n\n\n builtin/receive-pack.c | 33 ++++++++++-----------------------\n commit.c | 14 ++++++++++++--\n commit.h | 5 +++++\n 3 files changed, 27 insertions(+), 25 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 9f4a0b816cf..5c2732a0d07 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -581,32 +581,19 @@ static char *prepare_push_cert_nonce(const char *path, timestamp_t stamp)\n \treturn strbuf_detach(&buf, NULL);\n }\n \n-/*\n- * NEEDSWORK: reuse find_commit_header() from jk/commit-author-parsing\n- * after dropping \"_commit\" from its name and possibly moving it out\n- * of commit.c\n- */\n static char *find_header(const char *msg, size_t len, const char *key,\n \t\t\t const char **next_line)\n {\n-\tint key_len = strlen(key);\n-\tconst char *line = msg;\n-\n-\twhile (line && line < msg + len) {\n-\t\tconst char *eol = strchrnul(line, '\\n');\n-\n-\t\tif ((msg + len <= eol) || line == eol)\n-\t\t\treturn NULL;\n-\t\tif (line + key_len < eol &&\n-\t\t !memcmp(line, key, key_len) && line[key_len] == ' ') {\n-\t\t\tint offset = key_len + 1;\n-\t\t\tif (next_line)\n-\t\t\t\t*next_line = *eol ? eol + 1 : eol;\n-\t\t\treturn xmemdupz(line + offset, (eol - line) - offset);\n-\t\t}\n-\t\tline = *eol ? eol + 1 : NULL;\n-\t}\n-\treturn NULL;\n+\tsize_t out_len;\n+\tconst char *val = find_header_mem(msg, len, key, &out_len);\n+\n+\tif (!val)\n+\t\treturn NULL;\n+\n+\tif (next_line)\n+\t\t*next_line = val + out_len + 1;\n+\n+\treturn xmemdupz(val, out_len);\n }\n \n /*\ndiff --git a/commit.c b/commit.c\nindex a348f085b2b..5ece03e6373 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1631,12 +1631,18 @@ struct commit_list **commit_list_append(struct commit *commit,\n \treturn &new_commit->next;\n }\n \n-const char *find_commit_header(const char *msg, const char *key, size_t *out_len)\n+const char *find_header_mem(const char *msg, size_t len,\n+\t\t\tconst char *key, size_t *out_len)\n {\n \tint key_len = strlen(key);\n \tconst char *line = msg;\n \n-\twhile (line) {\n+\t/*\n+\t * NEEDSWORK: Between line[0] and msg[len], there may not be a LF nor NUL\n+\t * at all, and strchrnul() will scan beyond the range we were given\n+\t * Make this operation safer and abide by the contract to only read up to len.\n+\t */\n+\twhile (line && line < msg + len) {\n \t\tconst char *eol = strchrnul(line, '\\n');\n \n \t\tif (line == eol)\n@@ -1653,6 +1659,10 @@ const char *find_commit_header(const char *msg, const char *key, size_t *out_len\n \treturn NULL;\n }\n \n+const char *find_commit_header(const char *msg, const char *key, size_t *out_len)\n+{\n+\treturn find_header_mem(msg, strlen(msg), key, out_len);\n+}\n /*\n * Inspect the given string and determine the true \"end\" of the log message, in\n * order to find where to put a new Signed-off-by trailer. Ignored are\ndiff --git a/commit.h b/commit.h\nindex 3ea32766bcb..38cc5426615 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -290,12 +290,17 @@ void free_commit_extra_headers(struct commit_extra_header *extra);\n \n /*\n * Search the commit object contents given by \"msg\" for the header \"key\".\n+ * Reads up to \"len\" bytes of \"msg\".\n * Returns a pointer to the start of the header contents, or NULL. The length\n * of the header, up to the first newline, is returned via out_len.\n *\n * Note that some headers (like mergetag) may be multi-line. It is the caller's\n * responsibility to parse further in this case!\n */\n+const char *find_header_mem(const char *msg, size_t len,\n+\t\t\tconst char *key,\n+\t\t\tsize_t *out_len);\n+\n const char *find_commit_header(const char *msg, const char *key,\n \t\t\t size_t *out_len);\n \n\nbase-commit: c8b2ade48c204690119936ada89cd938c476c5c2\n-- \ngitgitgadget\n"},{"id":"445571","messageId":"xmqqczl6hrk5.fsf@gitster.g","threadId":"57155","inReplyTo":"pull.1125.v4.git.git.1641396098056.gitgitgadget@gmail.com","subject":"Re: [PATCH v4] receive-pack.c: consolidate find header logic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-05T20:10:34Z","receivedAt":"2022-01-05T20:10:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"John Cai via GitGitGadget\" writes:\n\n> Changes since v4:\n> \n> * added NEEDSWORK block detailing what needs to be done to clean up\n> find_header_mem\n> ...\n> -\twhile (line) {\n> ++\t/*\n> ++\t * NEEDSWORK: Between line[0] and msg[len], there may not be a LF nor NUL\n> ++\t * at all, and strchrnul() will scan beyond the range we were given\n> ++\t * Make this operation safer and abide by the contract to only read up to len.\n> ++\t */\n\nThis sounds unnecessarily alarming. Can't we also explain that the\ncurrent callers are safe?\n"},{"id":"445601","messageId":"pull.1125.v5.git.git.1641430309837.gitgitgadget@gmail.com","threadId":"57155","inReplyTo":"pull.1125.v4.git.git.1641396098056.gitgitgadget@gmail.com","subject":"[PATCH v5] receive-pack.c: consolidate find header logic","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-01-06T00:51:49Z","receivedAt":"2022-01-06T00:56:00Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai \n\nThere are two functions that have very similar logic of finding a header\nvalue. find_commit_header, and find_header. We can conslidate the logic\nby introducing a new function find_header_mem, which is equivalent to\nfind_commit_header except it takes a len parameter that determines how\nmany bytes will be read. find_commit_header and find_header can then both\ncall find_header_mem.\n\nThis reduces duplicate logic, as the logic for finding header values\ncan now all live in one place.\n\nSigned-off-by: John Cai \n---\n Consolidate find_header logic into one function\n \n This addresses the NEEDSWORK comment in builtin/receive-pack.c:\n \n /**\n * NEEDSWORK: reuse find_commit_header() from jk/commit-author-parsing\n * after dropping \"_commit\" from its name and possibly moving it out\n * of commit.c\n **/\n \n \n There are two functions that have very similar logic of finding a header\n value. find_commit_header, and find_header. We can conslidate the logic\n by introducing a new function find_header_mem, which is equivalent to\n find_commit_header except it takes a len parameter that determines how\n many bytes will be read. find_commit_header and find_header can then\n both call find_header_mem.\n \n This reduces duplicate logic, as the logic for finding header values can\n now all live in one place.\n \n Changes since v4:\n \n * adjust verbiage of NEEDSWORK comment block\n \n Changes since v3:\n \n * added NEEDSWORK block detailing what needs to be done to clean up\n find_header_mem\n * fixed verbiage in commit message\n * adjusted style of an if block (based on Junio's feedback)\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1125%2Fjohn-cai%2Fjc%2Freplace-find-header-v5\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1125/john-cai/jc/replace-find-header-v5\nPull-Request: https://github.com/git/git/pull/1125\n\nRange-diff vs v4:\n\n 1: a7b00022b00 ! 1: 02da3136c43 receive-pack.c: consolidate find header logic\n @@ commit.c: struct commit_list **commit_list_append(struct commit *commit,\n \n -\twhile (line) {\n +\t/*\n -+\t * NEEDSWORK: Between line[0] and msg[len], there may not be a LF nor NUL\n -+\t * at all, and strchrnul() will scan beyond the range we were given\n -+\t * Make this operation safer and abide by the contract to only read up to len.\n ++\t * NEEDSWORK: It's possible for strchrnul() to scan beyond the range\n ++\t * given by len. However, current callers are safe because they compute\n ++\t * len by scanning a NUL-terminated block of memory starting at msg.\n ++\t * Make this function safer by checking if the input is NUL-terminated\n ++\t * or has a NL between line[0] and msg[len].\n +\t */\n +\twhile (line && line < msg + len) {\n \t\tconst char *eol = strchrnul(line, '\\n');\n\n\n builtin/receive-pack.c | 33 ++++++++++-----------------------\n commit.c | 16 ++++++++++++++--\n commit.h | 5 +++++\n 3 files changed, 29 insertions(+), 25 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 9f4a0b816cf..5c2732a0d07 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -581,32 +581,19 @@ static char *prepare_push_cert_nonce(const char *path, timestamp_t stamp)\n \treturn strbuf_detach(&buf, NULL);\n }\n \n-/*\n- * NEEDSWORK: reuse find_commit_header() from jk/commit-author-parsing\n- * after dropping \"_commit\" from its name and possibly moving it out\n- * of commit.c\n- */\n static char *find_header(const char *msg, size_t len, const char *key,\n \t\t\t const char **next_line)\n {\n-\tint key_len = strlen(key);\n-\tconst char *line = msg;\n-\n-\twhile (line && line < msg + len) {\n-\t\tconst char *eol = strchrnul(line, '\\n');\n-\n-\t\tif ((msg + len <= eol) || line == eol)\n-\t\t\treturn NULL;\n-\t\tif (line + key_len < eol &&\n-\t\t !memcmp(line, key, key_len) && line[key_len] == ' ') {\n-\t\t\tint offset = key_len + 1;\n-\t\t\tif (next_line)\n-\t\t\t\t*next_line = *eol ? eol + 1 : eol;\n-\t\t\treturn xmemdupz(line + offset, (eol - line) - offset);\n-\t\t}\n-\t\tline = *eol ? eol + 1 : NULL;\n-\t}\n-\treturn NULL;\n+\tsize_t out_len;\n+\tconst char *val = find_header_mem(msg, len, key, &out_len);\n+\n+\tif (!val)\n+\t\treturn NULL;\n+\n+\tif (next_line)\n+\t\t*next_line = val + out_len + 1;\n+\n+\treturn xmemdupz(val, out_len);\n }\n \n /*\ndiff --git a/commit.c b/commit.c\nindex a348f085b2b..f5224e65de2 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1631,12 +1631,20 @@ struct commit_list **commit_list_append(struct commit *commit,\n \treturn &new_commit->next;\n }\n \n-const char *find_commit_header(const char *msg, const char *key, size_t *out_len)\n+const char *find_header_mem(const char *msg, size_t len,\n+\t\t\tconst char *key, size_t *out_len)\n {\n \tint key_len = strlen(key);\n \tconst char *line = msg;\n \n-\twhile (line) {\n+\t/*\n+\t * NEEDSWORK: It's possible for strchrnul() to scan beyond the range\n+\t * given by len. However, current callers are safe because they compute\n+\t * len by scanning a NUL-terminated block of memory starting at msg.\n+\t * Make this function safer by checking if the input is NUL-terminated\n+\t * or has a NL between line[0] and msg[len].\n+\t */\n+\twhile (line && line < msg + len) {\n \t\tconst char *eol = strchrnul(line, '\\n');\n \n \t\tif (line == eol)\n@@ -1653,6 +1661,10 @@ const char *find_commit_header(const char *msg, const char *key, size_t *out_len\n \treturn NULL;\n }\n \n+const char *find_commit_header(const char *msg, const char *key, size_t *out_len)\n+{\n+\treturn find_header_mem(msg, strlen(msg), key, out_len);\n+}\n /*\n * Inspect the given string and determine the true \"end\" of the log message, in\n * order to find where to put a new Signed-off-by trailer. Ignored are\ndiff --git a/commit.h b/commit.h\nindex 3ea32766bcb..38cc5426615 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -290,12 +290,17 @@ void free_commit_extra_headers(struct commit_extra_header *extra);\n \n /*\n * Search the commit object contents given by \"msg\" for the header \"key\".\n+ * Reads up to \"len\" bytes of \"msg\".\n * Returns a pointer to the start of the header contents, or NULL. The length\n * of the header, up to the first newline, is returned via out_len.\n *\n * Note that some headers (like mergetag) may be multi-line. It is the caller's\n * responsibility to parse further in this case!\n */\n+const char *find_header_mem(const char *msg, size_t len,\n+\t\t\tconst char *key,\n+\t\t\tsize_t *out_len);\n+\n const char *find_commit_header(const char *msg, const char *key,\n \t\t\t size_t *out_len);\n \n\nbase-commit: c8b2ade48c204690119936ada89cd938c476c5c2\n-- \ngitgitgadget\n"},{"id":"445646","messageId":"xmqqmtk8d55r.fsf@gitster.g","threadId":"57155","inReplyTo":"pull.1125.v5.git.git.1641430309837.gitgitgadget@gmail.com","subject":"Re: [PATCH v5] receive-pack.c: consolidate find header logic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-06T19:40:16Z","receivedAt":"2022-01-06T19:40:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"John Cai via GitGitGadget\" writes:\n\n> ++\t * NEEDSWORK: It's possible for strchrnul() to scan beyond the range\n> ++\t * given by len. However, current callers are safe because they compute\n> ++\t * len by scanning a NUL-terminated block of memory starting at msg.\n\nOK. Because the caller didn't die with SIGBUS owhen they scanned\nthe memory to figure out len, we know scanning between msg[0] and\nmsg[len] is safe.\n\n> ++\t * Make this function safer by checking if the input is NUL-terminated\n> ++\t * or has a NL between line[0] and msg[len].\n\nThis comment however feels wrong. The function is _safe_ if the\ninput is NUL-terminated already. And it is very expected that the\nmsg[] has LF between msg[0] and msg[len]---after all that is why we\nuse strchrnul() to scan it.\n\nPerhaps replace it with something like \"But nevertheless it would be\nbetter to make sure the function does not look at msg beyond len\nthat was provided by the caller\", perhaps?\n\nThe most problematic case happens when msg points into somewhere\ninside a page and &msg[len] is still inside that page, but (1) there\nis no LF nor NUL in the page after &msg[0], and (2) the page after\nthe page &msg[0] and &msg[len] are in is not mapped. Letting\nstrchrnul() scan msg[] beyond len will lead us to attempt to read\nthe first byte of the next page and killed with SIGBUS (I recall\nthat I had to debug a mysterious breakage in strlen or strchr on\nSunOS, which tried to grab 8-byte at a time and crossed page\nboundary to get an unnecessary SIGBUS).\n"},{"id":"445650","messageId":"pull.1125.v6.git.git.1641499655700.gitgitgadget@gmail.com","threadId":"57155","inReplyTo":"pull.1125.v5.git.git.1641430309837.gitgitgadget@gmail.com","subject":"[PATCH v6] receive-pack.c: consolidate find header logic","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-01-06T20:07:35Z","receivedAt":"2022-01-06T20:07:41Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai \n\nThere are two functions that have very similar logic of finding a header\nvalue. find_commit_header, and find_header. We can conslidate the logic\nby introducing a new function find_header_mem, which is equivalent to\nfind_commit_header except it takes a len parameter that determines how\nmany bytes will be read. find_commit_header and find_header can then both\ncall find_header_mem.\n\nThis reduces duplicate logic, as the logic for finding header values\ncan now all live in one place.\n\nSigned-off-by: John Cai \n---\n Consolidate find_header logic into one function\n \n This addresses the NEEDSWORK comment in builtin/receive-pack.c:\n \n /**\n * NEEDSWORK: reuse find_commit_header() from jk/commit-author-parsing\n * after dropping \"_commit\" from its name and possibly moving it out\n * of commit.c\n **/\n \n \n There are two functions that have very similar logic of finding a header\n value. find_commit_header, and find_header. We can conslidate the logic\n by introducing a new function find_header_mem, which is equivalent to\n find_commit_header except it takes a len parameter that determines how\n many bytes will be read. find_commit_header and find_header can then\n both call find_header_mem.\n \n This reduces duplicate logic, as the logic for finding header values can\n now all live in one place.\n \n Changes since v4:\n \n * adjust verbiage of NEEDSWORK comment block\n \n Changes since v3:\n \n * added NEEDSWORK block detailing what needs to be done to clean up\n find_header_mem\n * fixed verbiage in commit message\n * adjusted style of an if block (based on Junio's feedback)\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1125%2Fjohn-cai%2Fjc%2Freplace-find-header-v6\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1125/john-cai/jc/replace-find-header-v6\nPull-Request: https://github.com/git/git/pull/1125\n\nRange-diff vs v5:\n\n 1: 02da3136c43 ! 1: 2b01d92b951 receive-pack.c: consolidate find header logic\n @@ commit.c: struct commit_list **commit_list_append(struct commit *commit,\n +\t * NEEDSWORK: It's possible for strchrnul() to scan beyond the range\n +\t * given by len. However, current callers are safe because they compute\n +\t * len by scanning a NUL-terminated block of memory starting at msg.\n -+\t * Make this function safer by checking if the input is NUL-terminated\n -+\t * or has a NL between line[0] and msg[len].\n ++\t * Nonetheless, it would be better to ensure the function does not look\n ++\t * at msg beyond the len provided by the caller.\n +\t */\n +\twhile (line && line < msg + len) {\n \t\tconst char *eol = strchrnul(line, '\\n');\n\n\n builtin/receive-pack.c | 33 ++++++++++-----------------------\n commit.c | 16 ++++++++++++++--\n commit.h | 5 +++++\n 3 files changed, 29 insertions(+), 25 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 9f4a0b816cf..5c2732a0d07 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -581,32 +581,19 @@ static char *prepare_push_cert_nonce(const char *path, timestamp_t stamp)\n \treturn strbuf_detach(&buf, NULL);\n }\n \n-/*\n- * NEEDSWORK: reuse find_commit_header() from jk/commit-author-parsing\n- * after dropping \"_commit\" from its name and possibly moving it out\n- * of commit.c\n- */\n static char *find_header(const char *msg, size_t len, const char *key,\n \t\t\t const char **next_line)\n {\n-\tint key_len = strlen(key);\n-\tconst char *line = msg;\n-\n-\twhile (line && line < msg + len) {\n-\t\tconst char *eol = strchrnul(line, '\\n');\n-\n-\t\tif ((msg + len <= eol) || line == eol)\n-\t\t\treturn NULL;\n-\t\tif (line + key_len < eol &&\n-\t\t !memcmp(line, key, key_len) && line[key_len] == ' ') {\n-\t\t\tint offset = key_len + 1;\n-\t\t\tif (next_line)\n-\t\t\t\t*next_line = *eol ? eol + 1 : eol;\n-\t\t\treturn xmemdupz(line + offset, (eol - line) - offset);\n-\t\t}\n-\t\tline = *eol ? eol + 1 : NULL;\n-\t}\n-\treturn NULL;\n+\tsize_t out_len;\n+\tconst char *val = find_header_mem(msg, len, key, &out_len);\n+\n+\tif (!val)\n+\t\treturn NULL;\n+\n+\tif (next_line)\n+\t\t*next_line = val + out_len + 1;\n+\n+\treturn xmemdupz(val, out_len);\n }\n \n /*\ndiff --git a/commit.c b/commit.c\nindex a348f085b2b..28391c3468d 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1631,12 +1631,20 @@ struct commit_list **commit_list_append(struct commit *commit,\n \treturn &new_commit->next;\n }\n \n-const char *find_commit_header(const char *msg, const char *key, size_t *out_len)\n+const char *find_header_mem(const char *msg, size_t len,\n+\t\t\tconst char *key, size_t *out_len)\n {\n \tint key_len = strlen(key);\n \tconst char *line = msg;\n \n-\twhile (line) {\n+\t/*\n+\t * NEEDSWORK: It's possible for strchrnul() to scan beyond the range\n+\t * given by len. However, current callers are safe because they compute\n+\t * len by scanning a NUL-terminated block of memory starting at msg.\n+\t * Nonetheless, it would be better to ensure the function does not look\n+\t * at msg beyond the len provided by the caller.\n+\t */\n+\twhile (line && line < msg + len) {\n \t\tconst char *eol = strchrnul(line, '\\n');\n \n \t\tif (line == eol)\n@@ -1653,6 +1661,10 @@ const char *find_commit_header(const char *msg, const char *key, size_t *out_len\n \treturn NULL;\n }\n \n+const char *find_commit_header(const char *msg, const char *key, size_t *out_len)\n+{\n+\treturn find_header_mem(msg, strlen(msg), key, out_len);\n+}\n /*\n * Inspect the given string and determine the true \"end\" of the log message, in\n * order to find where to put a new Signed-off-by trailer. Ignored are\ndiff --git a/commit.h b/commit.h\nindex 3ea32766bcb..38cc5426615 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -290,12 +290,17 @@ void free_commit_extra_headers(struct commit_extra_header *extra);\n \n /*\n * Search the commit object contents given by \"msg\" for the header \"key\".\n+ * Reads up to \"len\" bytes of \"msg\".\n * Returns a pointer to the start of the header contents, or NULL. The length\n * of the header, up to the first newline, is returned via out_len.\n *\n * Note that some headers (like mergetag) may be multi-line. It is the caller's\n * responsibility to parse further in this case!\n */\n+const char *find_header_mem(const char *msg, size_t len,\n+\t\t\tconst char *key,\n+\t\t\tsize_t *out_len);\n+\n const char *find_commit_header(const char *msg, const char *key,\n \t\t\t size_t *out_len);\n \n\nbase-commit: c8b2ade48c204690119936ada89cd938c476c5c2\n-- \ngitgitgadget\n"},{"id":"445780","messageId":"AF6241F3-8AA9-4FD8-BCB1-21E0ED29E108@gmail.com","threadId":"57155","inReplyTo":"pull.1125.v6.git.git.1641499655700.gitgitgadget@gmail.com","subject":"Re: [PATCH v6] receive-pack.c: consolidate find header logic","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2022-01-08T04:54:38Z","receivedAt":"2022-01-08T04:55:20Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"Hi Junio,\n\n\nOn 6 Jan 2022, at 15:07, John Cai via GitGitGadget wrote:\n\n> From: John Cai \n>\n> There are two functions that have very similar logic of finding a header\n> value. find_commit_header, and find_header. We can conslidate the logic\n> by introducing a new function find_header_mem, which is equivalent to\n> find_commit_header except it takes a len parameter that determines how\n> many bytes will be read. find_commit_header and find_header can then both\n> call find_header_mem.\n>\n> This reduces duplicate logic, as the logic for finding header values\n> can now all live in one place.\n>\n> Signed-off-by: John Cai \n> ---\n> Consolidate find_header logic into one function\n>\n> This addresses the NEEDSWORK comment in builtin/receive-pack.c:\n>\n> /**\n> * NEEDSWORK: reuse find_commit_header() from jk/commit-author-parsing\n> * after dropping \"_commit\" from its name and possibly moving it out\n> * of commit.c\n> **/\n>\n>\n> There are two functions that have very similar logic of finding a header\n> value. find_commit_header, and find_header. We can conslidate the logic\n> by introducing a new function find_header_mem, which is equivalent to\n> find_commit_header except it takes a len parameter that determines how\n> many bytes will be read. find_commit_header and find_header can then\n> both call find_header_mem.\n>\n> This reduces duplicate logic, as the logic for finding header values can\n> now all live in one place.\n>\n> Changes since v4:\n>\n> * adjust verbiage of NEEDSWORK comment block\n>\n> Changes since v3:\n>\n> * added NEEDSWORK block detailing what needs to be done to clean up\n> find_header_mem\n> * fixed verbiage in commit message\n> * adjusted style of an if block (based on Junio's feedback)\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1125%2Fjohn-cai%2Fjc%2Freplace-find-header-v6\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1125/john-cai/jc/replace-find-header-v6\n> Pull-Request: https://github.com/git/git/pull/1125\n>\n> Range-diff vs v5:\n>\n> 1: 02da3136c43 ! 1: 2b01d92b951 receive-pack.c: consolidate find header logic\n> @@ commit.c: struct commit_list **commit_list_append(struct commit *commit,\n> +\t * NEEDSWORK: It's possible for strchrnul() to scan beyond the range\n> +\t * given by len. However, current callers are safe because they compute\n> +\t * len by scanning a NUL-terminated block of memory starting at msg.\n> -+\t * Make this function safer by checking if the input is NUL-terminated\n> -+\t * or has a NL between line[0] and msg[len].\n> ++\t * Nonetheless, it would be better to ensure the function does not look\n> ++\t * at msg beyond the len provided by the caller.\n> +\t */\n> +\twhile (line && line < msg + len) {\n> \t\tconst char *eol = strchrnul(line, '\\n');\n>\n>\n> builtin/receive-pack.c | 33 ++++++++++-----------------------\n> commit.c | 16 ++++++++++++++--\n> commit.h | 5 +++++\n> 3 files changed, 29 insertions(+), 25 deletions(-)\n>\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index 9f4a0b816cf..5c2732a0d07 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -581,32 +581,19 @@ static char *prepare_push_cert_nonce(const char *path, timestamp_t stamp)\n> \treturn strbuf_detach(&buf, NULL);\n> }\n>\n> -/*\n> - * NEEDSWORK: reuse find_commit_header() from jk/commit-author-parsing\n> - * after dropping \"_commit\" from its name and possibly moving it out\n> - * of commit.c\n> - */\n> static char *find_header(const char *msg, size_t len, const char *key,\n> \t\t\t const char **next_line)\n> {\n> -\tint key_len = strlen(key);\n> -\tconst char *line = msg;\n> -\n> -\twhile (line && line < msg + len) {\n> -\t\tconst char *eol = strchrnul(line, '\\n');\n> -\n> -\t\tif ((msg + len <= eol) || line == eol)\n> -\t\t\treturn NULL;\n> -\t\tif (line + key_len < eol &&\n> -\t\t !memcmp(line, key, key_len) && line[key_len] == ' ') {\n> -\t\t\tint offset = key_len + 1;\n> -\t\t\tif (next_line)\n> -\t\t\t\t*next_line = *eol ? eol + 1 : eol;\n> -\t\t\treturn xmemdupz(line + offset, (eol - line) - offset);\n> -\t\t}\n> -\t\tline = *eol ? eol + 1 : NULL;\n> -\t}\n> -\treturn NULL;\n> +\tsize_t out_len;\n> +\tconst char *val = find_header_mem(msg, len, key, &out_len);\n> +\n> +\tif (!val)\n> +\t\treturn NULL;\n> +\n> +\tif (next_line)\n> +\t\t*next_line = val + out_len + 1;\n> +\n> +\treturn xmemdupz(val, out_len);\n> }\n>\n> /*\n> diff --git a/commit.c b/commit.c\n> index a348f085b2b..28391c3468d 100644\n> --- a/commit.c\n> +++ b/commit.c\n> @@ -1631,12 +1631,20 @@ struct commit_list **commit_list_append(struct commit *commit,\n> \treturn &new_commit->next;\n> }\n>\n> -const char *find_commit_header(const char *msg, const char *key, size_t *out_len)\n> +const char *find_header_mem(const char *msg, size_t len,\n> +\t\t\tconst char *key, size_t *out_len)\n> {\n> \tint key_len = strlen(key);\n> \tconst char *line = msg;\n>\n> -\twhile (line) {\n> +\t/*\n> +\t * NEEDSWORK: It's possible for strchrnul() to scan beyond the range\n> +\t * given by len. However, current callers are safe because they compute\n> +\t * len by scanning a NUL-terminated block of memory starting at msg.\n> +\t * Nonetheless, it would be better to ensure the function does not look\n> +\t * at msg beyond the len provided by the caller.\n> +\t */\n\nLet me know if this verbiage makes sense.\n\n> +\twhile (line && line < msg + len) {\n> \t\tconst char *eol = strchrnul(line, '\\n');\n>\n> \t\tif (line == eol)\n> @@ -1653,6 +1661,10 @@ const char *find_commit_header(const char *msg, const char *key, size_t *out_len\n> \treturn NULL;\n> }\n>\n> +const char *find_commit_header(const char *msg, const char *key, size_t *out_len)\n> +{\n> +\treturn find_header_mem(msg, strlen(msg), key, out_len);\n> +}\n> /*\n> * Inspect the given string and determine the true \"end\" of the log message, in\n> * order to find where to put a new Signed-off-by trailer. Ignored are\n> diff --git a/commit.h b/commit.h\n> index 3ea32766bcb..38cc5426615 100644\n> --- a/commit.h\n> +++ b/commit.h\n> @@ -290,12 +290,17 @@ void free_commit_extra_headers(struct commit_extra_header *extra);\n>\n> /*\n> * Search the commit object contents given by \"msg\" for the header \"key\".\n> + * Reads up to \"len\" bytes of \"msg\".\n> * Returns a pointer to the start of the header contents, or NULL. The length\n> * of the header, up to the first newline, is returned via out_len.\n> *\n> * Note that some headers (like mergetag) may be multi-line. It is the caller's\n> * responsibility to parse further in this case!\n> */\n> +const char *find_header_mem(const char *msg, size_t len,\n> +\t\t\tconst char *key,\n> +\t\t\tsize_t *out_len);\n> +\n> const char *find_commit_header(const char *msg, const char *key,\n> \t\t\t size_t *out_len);\n>\n>\n> base-commit: c8b2ade48c204690119936ada89cd938c476c5c2\n> -- \n> gitgitgadget\n"},{"id":"445782","messageId":"xmqqwnja3do4.fsf@gitster.g","threadId":"57155","inReplyTo":"pull.1125.v6.git.git.1641499655700.gitgitgadget@gmail.com","subject":"Re: [PATCH v6] receive-pack.c: consolidate find header logic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-08T07:11:07Z","receivedAt":"2022-01-08T07:11:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"John Cai via GitGitGadget\" writes:\n\n> From: John Cai \n>\n> There are two functions that have very similar logic of finding a header\n> value. find_commit_header, and find_header. We can conslidate the logic\n> by introducing a new function find_header_mem, which is equivalent to\n> find_commit_header except it takes a len parameter that determines how\n> many bytes will be read. find_commit_header and find_header can then both\n> call find_header_mem.\n>\n> This reduces duplicate logic, as the logic for finding header values\n> can now all live in one place.\n>\n> Signed-off-by: John Cai \n> ---\n> Consolidate find_header logic into one function\n> +const char *find_header_mem(const char *msg, size_t len,\n> +\t\t\tconst char *key, size_t *out_len)\n> {\n> \tint key_len = strlen(key);\n> \tconst char *line = msg;\n> \n> -\twhile (line) {\n> +\t/*\n> +\t * NEEDSWORK: It's possible for strchrnul() to scan beyond the range\n> +\t * given by len. However, current callers are safe because they compute\n> +\t * len by scanning a NUL-terminated block of memory starting at msg.\n> +\t * Nonetheless, it would be better to ensure the function does not look\n> +\t * at msg beyond the len provided by the caller.\n> +\t */\n> +\twhile (line && line < msg + len) {\n> \t\tconst char *eol = strchrnul(line, '\\n');\n\nThanks; queued.\n"}]}