From: John Cai via GitGitGadget <hidden> Date: 2021-12-27 18:26:42
This addresses the NEEDSWORK comment in builtin/receive-pack.c:
/**
* NEEDSWORK: reuse find_commit_header() from jk/commit-author-parsing
* after dropping "_commit" from its name and possibly moving it out
* of commit.c
**/
There is some duplicated logic between find_header and find_commit_header
that can be consolidated instead of having two places in the code that do
essentially the same thing. For the sake of simpler and more DRY code, use
find_commit_header and rename it to find_header since it is not limited to
finding headers for only commits.
This is my first attempt to address a NEEDSWORK issue, and I'm relatively
new to C so any feedback would be appreciated!
John Cai (2):
receive-pack.c: consolidate find header logic
commit.c: rename find_commit_header to find_header
builtin/am.c | 2 +-
builtin/commit.c | 2 +-
builtin/receive-pack.c | 48 ++++++++++++++++---------------------
commit.c | 7 +++---
commit.h | 2 +-
gpg-interface.c | 2 +-
pretty.c | 2 +-
sequencer.c | 2 +-
t/helper/test-fast-rebase.c | 2 +-
9 files changed, 32 insertions(+), 37 deletions(-)
base-commit: 2ae0a9cb8298185a94e5998086f380a355dd8907
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1125%2Fjohn-cai%2Fjc%2Freplace-find-header-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1125/john-cai/jc/replace-find-header-v1
Pull-Request: https://github.com/git/git/pull/1125
--
gitgitgadget
From: John Cai via GitGitGadget <hidden> Date: 2021-12-27 18:26:45
From: John Cai <redacted>
There are two functions that have very similar logic of finding a header
value. find_commit_header, and find_header. We can conslidate the logic
by using find_commit_header and replacing the logic in find_header.
This helps clean up the code, as the logic for finding header values can
stay in one place.
Signed-off-by: John Cai <redacted>
---
builtin/receive-pack.c | 48 ++++++++++++++++++------------------------
commit.c | 3 ++-
2 files changed, 23 insertions(+), 28 deletions(-)
From: John Cai via GitGitGadget <hidden> Date: 2021-12-27 18:26:46
From: John Cai <redacted>
Since find_commit_header's logic is not just limited to commit objects,
we can rename this to a more general find_header function so it's
clearer that it can be used in a more general way.
Signed-off-by: John Cai <redacted>
---
builtin/am.c | 2 +-
builtin/commit.c | 2 +-
builtin/receive-pack.c | 8 ++++----
commit.c | 4 ++--
commit.h | 2 +-
gpg-interface.c | 2 +-
pretty.c | 2 +-
sequencer.c | 2 +-
t/helper/test-fast-rebase.c | 2 +-
9 files changed, 13 insertions(+), 13 deletions(-)
@@ -296,7 +296,7 @@ void free_commit_extra_headers(struct commit_extra_header *extra);*Notethatsomeheaders(likemergetag)maybemulti-line.Itisthecaller's*responsibilitytoparsefurtherinthiscase!*/-constchar*find_commit_header(constchar*msg,constchar*key,+constchar*find_header(constchar*msg,constchar*key,size_t*out_len);/* Find the end of the log message, the right place for a new trailer. */
@@ -597,7 +597,7 @@ static int parse_payload_metadata(struct signature_check *sigc)BUG("invalid value for sigc->payload_type");}-ident_line=find_commit_header(sigc->payload,signer_header,&ident_len);+ident_line=find_header(sigc->payload,signer_header,&ident_len);if(!ident_line||!ident_len)return1;
From: John Cai via GitGitGadget <hidden> Date: 2021-12-29 06:19:30
From: John Cai <redacted>
There are two functions that have very similar logic of finding a header
value. find_commit_header, and find_header. We can conslidate the logic
by using find_commit_header and replacing the logic in find_header.
Introduce a new function find_header_max, which is equivalent to
find_commit_header except it takes a len parameter that determines how
many bytes to read. find_commit_header can then call find_header_max
with 0 as the len.
This cleans up duplicate logic, as the logic for finding header values
is now all in one place.
Signed-off-by: John Cai <redacted>
---
Consolidate find_header logic into one function
This addresses the NEEDSWORK comment in builtin/receive-pack.c:
/**
* NEEDSWORK: reuse find_commit_header() from jk/commit-author-parsing
* after dropping "_commit" from its name and possibly moving it out
* of commit.c
**/
There is some duplicated logic between find_header and
find_commit_header that can be consolidated instead of having two places
in the code that do essentially the same thing. For the sake of simpler
and more DRY code, use find_commit_header and rename it to find_header
since it is not limited to finding headers for only commits.
Changes since v1:
* got rid of renaming from find_commit_header -> find_header since the
renaming did not provide much value
* simplified logic in find_header
* introduce find_header_max function, which is what find_commit_header
was before except it adds a "len" parameter to determine how much of
the buffer to read.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1125%2Fjohn-cai%2Fjc%2Freplace-find-header-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1125/john-cai/jc/replace-find-header-v2
Pull-Request: https://github.com/git/git/pull/1125
Range-diff vs v1:
1: 9465c20d4bd ! 1: 5e0d90d642b receive-pack.c: consolidate find header logic
@@ Commit message
There are two functions that have very similar logic of finding a header
value. find_commit_header, and find_header. We can conslidate the logic
by using find_commit_header and replacing the logic in find_header.
- This helps clean up the code, as the logic for finding header values can
- stay in one place.
+
+ Introduce a new function find_header_max, which is equivalent to
+ find_commit_header except it takes a len parameter that determines how
+ many bytes to read. find_commit_header can then call find_header_max
+ with 0 as the len.
+
+ This cleans up duplicate logic, as the logic for finding header values
+ is now all in one place.
Signed-off-by: John Cai [off-list ref]
@@ builtin/receive-pack.c: static char *prepare_push_cert_nonce(const char *path, t
- * after dropping "_commit" from its name and possibly moving it out
- * of commit.c
- */
--static char *find_header(const char *msg, size_t len, const char *key,
-- const char **next_line)
-+static char *find_header_value(const char *msg, const char *key, const char **next_line)
+ static char *find_header(const char *msg, size_t len, const char *key,
+ const char **next_line)
{
- int key_len = strlen(key);
- const char *line = msg;
@@ builtin/receive-pack.c: static char *prepare_push_cert_nonce(const char *path, t
- return xmemdupz(line + offset, (eol - line) - offset);
- }
- line = *eol ? eol + 1 : NULL;
+- }
+- return NULL;
+ size_t out_len;
-+ const char *eol;
-+ char *ret;
++ const char *val = find_header_max(msg, key, len, &out_len);
+
-+ const char *val = find_commit_header(msg, key, &out_len);
+ if (val == NULL)
+ return NULL;
+
-+ eol = strchrnul(val, '\n');
-+ if (next_line) {
-+ *next_line = *eol ? eol + 1: eol;
- }
-- return NULL;
-+
-+ ret = xmalloc(out_len+1);
-+ memcpy(ret, val, out_len);
-+ ret[out_len] = '\0';
++ if (next_line)
++ *next_line = val + out_len + 1;
+
-+ return ret;
++ return xmemdupz(val, out_len);
}
/*
-@@ builtin/receive-pack.c: static int constant_memequal(const char *a, const char *b, size_t n)
+
+ ## commit.c ##
+@@ commit.c: struct commit_list **commit_list_append(struct commit *commit,
+ return &new_commit->next;
+ }
- static const char *check_nonce(const char *buf, size_t len)
+-const char *find_commit_header(const char *msg, const char *key, size_t *out_len)
++const char *find_header_max(const char *msg, const char *key,
++ size_t len,
++ size_t *out_len)
{
-- char *nonce = find_header(buf, len, "nonce", NULL);
-+ char *nonce = find_header_value(buf, "nonce", NULL);
-+
- timestamp_t stamp, ostamp;
- char *bohmac, *expect = NULL;
- const char *retval = NONCE_BAD;
-@@ builtin/receive-pack.c: static int check_cert_push_options(const struct string_list *push_options)
- if (!len)
- return 1;
+ int key_len = strlen(key);
+ const char *line = msg;
-- while ((option = find_header(buf, len, "push-option", &next_line))) {
-- len -= (next_line - buf);
-+ while ((option = find_header_value(buf, "push-option", &next_line))) {
- buf = next_line;
- options_seen++;
- if (options_seen > push_options->nr
-
- ## commit.c ##
+- while (line) {
++ while (line && (len == 0 || line < msg + len)) {
+ const char *eol = strchrnul(line, '\n');
+
+ if (line == eol)
@@ commit.c: const char *find_commit_header(const char *msg, const char *key, size_t *out_len
- if (eol - line > key_len &&
- !strncmp(line, key, key_len) &&
- line[key_len] == ' ') {
-- *out_len = eol - line - key_len - 1;
-+ if (out_len != NULL)
-+ *out_len = eol - line - key_len - 1;
- return line + key_len + 1;
- }
- line = *eol ? eol + 1 : NULL;
+ return NULL;
+ }
+
++const char *find_commit_header(const char *msg, const char *key, size_t *out_len)
++{
++ return find_header_max(msg, key, 0, out_len);
++}
+ /*
+ * Inspect the given string and determine the true "end" of the log message, in
+ * order to find where to put a new Signed-off-by trailer. Ignored are
+
+ ## commit.h ##
+@@ commit.h: void free_commit_extra_headers(struct commit_extra_header *extra);
+
+ /*
+ * Search the commit object contents given by "msg" for the header "key".
++ * Reads up to "len" bytes of "msg".
+ * Returns a pointer to the start of the header contents, or NULL. The length
+ * of the header, up to the first newline, is returned via out_len.
+ *
+ * Note that some headers (like mergetag) may be multi-line. It is the caller's
+ * responsibility to parse further in this case!
+ */
++const char *find_header_max(const char *msg, const char *key,
++ size_t len,
++ size_t *out_len);
++
+ const char *find_commit_header(const char *msg, const char *key,
+ size_t *out_len);
+
2: 384a635daa2 < -: ----------- commit.c: rename find_commit_header to find_header
builtin/receive-pack.c | 33 ++++++++++-----------------------
commit.c | 10 ++++++++--
commit.h | 5 +++++
3 files changed, 23 insertions(+), 25 deletions(-)
From: John Cai via GitGitGadget <hidden> Date: 2021-12-31 06:17:49
From: John Cai <redacted>
There are two functions that have very similar logic of finding a header
value. find_commit_header, and find_header. We can conslidate the logic
by using find_commit_header and replacing the logic in find_header.
Introduce a new function find_header_max, which is equivalent to
find_commit_header except it takes a len parameter that determines how
many bytes to read. find_commit_header can then call find_header_max
with 0 as the len.
This cleans up duplicate logic, as the logic for finding header values
is now all in one place.
Signed-off-by: John Cai <redacted>
---
Consolidate find_header logic into one function
This addresses the NEEDSWORK comment in builtin/receive-pack.c:
/**
* NEEDSWORK: reuse find_commit_header() from jk/commit-author-parsing
* after dropping "_commit" from its name and possibly moving it out
* of commit.c
**/
There is some duplicated logic between find_header and
find_commit_header that can be consolidated instead of having two places
in the code that do essentially the same thing. For the sake of simpler
and more DRY code, use find_commit_header and rename it to find_header
since it is not limited to finding headers for only commits.
Changes since v2:
* s/find_header_max/find_header_mem
* moved "len" argument right next to "msg" agument in find_header_mem
* removed special condition in find_header_mem to check for line == 0
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1125%2Fjohn-cai%2Fjc%2Freplace-find-header-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1125/john-cai/jc/replace-find-header-v3
Pull-Request: https://github.com/git/git/pull/1125
Range-diff vs v2:
1: 5e0d90d642b ! 1: 463df42e7e0 receive-pack.c: consolidate find header logic
@@ builtin/receive-pack.c: static char *prepare_push_cert_nonce(const char *path, t
- }
- return NULL;
+ size_t out_len;
-+ const char *val = find_header_max(msg, key, len, &out_len);
++ const char *val = find_header_mem(msg, len, key, &out_len);
+
+ if (val == NULL)
+ return NULL;
@@ commit.c: struct commit_list **commit_list_append(struct commit *commit,
}
-const char *find_commit_header(const char *msg, const char *key, size_t *out_len)
-+const char *find_header_max(const char *msg, const char *key,
-+ size_t len,
-+ size_t *out_len)
++const char *find_header_mem(const char *msg, size_t len,
++ const char *key, size_t *out_len)
{
int key_len = strlen(key);
const char *line = msg;
- while (line) {
-+ while (line && (len == 0 || line < msg + len)) {
++ while (line && line < msg + len) {
const char *eol = strchrnul(line, '\n');
if (line == eol)
@@ commit.c: const char *find_commit_header(const char *msg, const char *key, size_
+const char *find_commit_header(const char *msg, const char *key, size_t *out_len)
+{
-+ return find_header_max(msg, key, 0, out_len);
++ return find_header_mem(msg, strlen(msg), key, out_len);
+}
/*
* Inspect the given string and determine the true "end" of the log message, in
@@ commit.h: void free_commit_extra_headers(struct commit_extra_header *extra);
* Note that some headers (like mergetag) may be multi-line. It is the caller's
* responsibility to parse further in this case!
*/
-+const char *find_header_max(const char *msg, const char *key,
-+ size_t len,
++const char *find_header_mem(const char *msg, size_t len,
++ const char *key,
+ size_t *out_len);
+
const char *find_commit_header(const char *msg, const char *key,
builtin/receive-pack.c | 33 ++++++++++-----------------------
commit.c | 9 +++++++--
commit.h | 5 +++++
3 files changed, 22 insertions(+), 25 deletions(-)
Between line[0] and msg[len], there may not be a LF nor NUL at all,
and strchrnul() will scan beyond the range we were given (which is
msg[0]..msg[len]).
But that is something we share with the find_header() if I am not
mistaken, so I am OK to leave the code as posted and leave it
outside the scope of this series to clean it up to make it safer.
The reason why it is probably safe for the current set of callers
(and presumably any reasonable new callers we may add later) is that
they computed "len" by scanning the block of memory starting at (or
before) "msg" before calling us, and we know that the block of
memory is properly NUL-terminated. find_header() is called by
check_nonce() and check_cert_push_options(), both of which tell
us to scan in a strbuf, which is designed to be scannable for NUL
safely by having an extra NUL at the end beyond the end.
Between line[0] and msg[len], there may not be a LF nor NUL at all,
and strchrnul() will scan beyond the range we were given (which is
msg[0]..msg[len]).
But that is something we share with the find_header() if I am not
mistaken, so I am OK to leave the code as posted and leave it
outside the scope of this series to clean it up to make it safer.
Good catch. Thanks for pointing that out-I didn’t notice it. I’ve added a NEEDSWORK
Block to this so we can address it in a later patch series.
The reason why it is probably safe for the current set of callers
(and presumably any reasonable new callers we may add later) is that
they computed "len" by scanning the block of memory starting at (or
before) "msg" before calling us, and we know that the block of
memory is properly NUL-terminated. find_header() is called by
check_nonce() and check_cert_push_options(), both of which tell
us to scan in a strbuf, which is designed to be scannable for NUL
safely by having an extra NUL at the end beyond the end.
From: John Cai via GitGitGadget <hidden> Date: 2022-01-05 15:21:44
From: John Cai <redacted>
There are two functions that have very similar logic of finding a header
value. find_commit_header, and find_header. We can conslidate the logic
by introducing a new function find_header_mem, which is equivalent to
find_commit_header except it takes a len parameter that determines how
many bytes will be read. find_commit_header and find_header can then both
call find_header_mem.
This reduces duplicate logic, as the logic for finding header values
can now all live in one place.
Signed-off-by: John Cai <redacted>
---
Consolidate find_header logic into one function
This addresses the NEEDSWORK comment in builtin/receive-pack.c:
/**
* NEEDSWORK: reuse find_commit_header() from jk/commit-author-parsing
* after dropping "_commit" from its name and possibly moving it out
* of commit.c
**/
There are two functions that have very similar logic of finding a header
value. find_commit_header, and find_header. We can conslidate the logic
by introducing a new function find_header_mem, which is equivalent to
find_commit_header except it takes a len parameter that determines how
many bytes will be read. find_commit_header and find_header can then
both call find_header_mem.
This reduces duplicate logic, as the logic for finding header values can
now all live in one place.
Changes since v4:
* added NEEDSWORK block detailing what needs to be done to clean up
find_header_mem
Changes since v3:
* fixed verbiage in commit message
* adjusted style of an if block (based on Junio's feedback)
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1125%2Fjohn-cai%2Fjc%2Freplace-find-header-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1125/john-cai/jc/replace-find-header-v4
Pull-Request: https://github.com/git/git/pull/1125
Range-diff vs v3:
1: 463df42e7e0 ! 1: a7b00022b00 receive-pack.c: consolidate find header logic
@@ Commit message
There are two functions that have very similar logic of finding a header
value. find_commit_header, and find_header. We can conslidate the logic
- by using find_commit_header and replacing the logic in find_header.
-
- Introduce a new function find_header_max, which is equivalent to
+ by introducing a new function find_header_mem, which is equivalent to
find_commit_header except it takes a len parameter that determines how
- many bytes to read. find_commit_header can then call find_header_max
- with 0 as the len.
+ many bytes will be read. find_commit_header and find_header can then both
+ call find_header_mem.
- This cleans up duplicate logic, as the logic for finding header values
- is now all in one place.
+ This reduces duplicate logic, as the logic for finding header values
+ can now all live in one place.
Signed-off-by: John Cai [off-list ref]
@@ builtin/receive-pack.c: static char *prepare_push_cert_nonce(const char *path, t
+ size_t out_len;
+ const char *val = find_header_mem(msg, len, key, &out_len);
+
-+ if (val == NULL)
++ if (!val)
+ return NULL;
+
+ if (next_line)
@@ commit.c: struct commit_list **commit_list_append(struct commit *commit,
const char *line = msg;
- while (line) {
++ /*
++ * NEEDSWORK: Between line[0] and msg[len], there may not be a LF nor NUL
++ * at all, and strchrnul() will scan beyond the range we were given
++ * Make this operation safer and abide by the contract to only read up to len.
++ */
+ while (line && line < msg + len) {
const char *eol = strchrnul(line, '\n');
builtin/receive-pack.c | 33 ++++++++++-----------------------
commit.c | 14 ++++++++++++--
commit.h | 5 +++++
3 files changed, 27 insertions(+), 25 deletions(-)
From: John Cai via GitGitGadget <hidden> Date: 2022-01-06 00:56:00
From: John Cai <redacted>
There are two functions that have very similar logic of finding a header
value. find_commit_header, and find_header. We can conslidate the logic
by introducing a new function find_header_mem, which is equivalent to
find_commit_header except it takes a len parameter that determines how
many bytes will be read. find_commit_header and find_header can then both
call find_header_mem.
This reduces duplicate logic, as the logic for finding header values
can now all live in one place.
Signed-off-by: John Cai <redacted>
---
Consolidate find_header logic into one function
This addresses the NEEDSWORK comment in builtin/receive-pack.c:
/**
* NEEDSWORK: reuse find_commit_header() from jk/commit-author-parsing
* after dropping "_commit" from its name and possibly moving it out
* of commit.c
**/
There are two functions that have very similar logic of finding a header
value. find_commit_header, and find_header. We can conslidate the logic
by introducing a new function find_header_mem, which is equivalent to
find_commit_header except it takes a len parameter that determines how
many bytes will be read. find_commit_header and find_header can then
both call find_header_mem.
This reduces duplicate logic, as the logic for finding header values can
now all live in one place.
Changes since v4:
* adjust verbiage of NEEDSWORK comment block
Changes since v3:
* added NEEDSWORK block detailing what needs to be done to clean up
find_header_mem
* fixed verbiage in commit message
* adjusted style of an if block (based on Junio's feedback)
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1125%2Fjohn-cai%2Fjc%2Freplace-find-header-v5
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1125/john-cai/jc/replace-find-header-v5
Pull-Request: https://github.com/git/git/pull/1125
Range-diff vs v4:
1: a7b00022b00 ! 1: 02da3136c43 receive-pack.c: consolidate find header logic
@@ commit.c: struct commit_list **commit_list_append(struct commit *commit,
- while (line) {
+ /*
-+ * NEEDSWORK: Between line[0] and msg[len], there may not be a LF nor NUL
-+ * at all, and strchrnul() will scan beyond the range we were given
-+ * Make this operation safer and abide by the contract to only read up to len.
++ * NEEDSWORK: It's possible for strchrnul() to scan beyond the range
++ * given by len. However, current callers are safe because they compute
++ * len by scanning a NUL-terminated block of memory starting at msg.
++ * Make this function safer by checking if the input is NUL-terminated
++ * or has a NL between line[0] and msg[len].
+ */
+ while (line && line < msg + len) {
const char *eol = strchrnul(line, '\n');
builtin/receive-pack.c | 33 ++++++++++-----------------------
commit.c | 16 ++++++++++++++--
commit.h | 5 +++++
3 files changed, 29 insertions(+), 25 deletions(-)
From: John Cai via GitGitGadget <hidden> Date: 2022-01-06 20:07:41
From: John Cai <redacted>
There are two functions that have very similar logic of finding a header
value. find_commit_header, and find_header. We can conslidate the logic
by introducing a new function find_header_mem, which is equivalent to
find_commit_header except it takes a len parameter that determines how
many bytes will be read. find_commit_header and find_header can then both
call find_header_mem.
This reduces duplicate logic, as the logic for finding header values
can now all live in one place.
Signed-off-by: John Cai <redacted>
---
Consolidate find_header logic into one function
This addresses the NEEDSWORK comment in builtin/receive-pack.c:
/**
* NEEDSWORK: reuse find_commit_header() from jk/commit-author-parsing
* after dropping "_commit" from its name and possibly moving it out
* of commit.c
**/
There are two functions that have very similar logic of finding a header
value. find_commit_header, and find_header. We can conslidate the logic
by introducing a new function find_header_mem, which is equivalent to
find_commit_header except it takes a len parameter that determines how
many bytes will be read. find_commit_header and find_header can then
both call find_header_mem.
This reduces duplicate logic, as the logic for finding header values can
now all live in one place.
Changes since v4:
* adjust verbiage of NEEDSWORK comment block
Changes since v3:
* added NEEDSWORK block detailing what needs to be done to clean up
find_header_mem
* fixed verbiage in commit message
* adjusted style of an if block (based on Junio's feedback)
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1125%2Fjohn-cai%2Fjc%2Freplace-find-header-v6
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1125/john-cai/jc/replace-find-header-v6
Pull-Request: https://github.com/git/git/pull/1125
Range-diff vs v5:
1: 02da3136c43 ! 1: 2b01d92b951 receive-pack.c: consolidate find header logic
@@ commit.c: struct commit_list **commit_list_append(struct commit *commit,
+ * NEEDSWORK: It's possible for strchrnul() to scan beyond the range
+ * given by len. However, current callers are safe because they compute
+ * len by scanning a NUL-terminated block of memory starting at msg.
-+ * Make this function safer by checking if the input is NUL-terminated
-+ * or has a NL between line[0] and msg[len].
++ * Nonetheless, it would be better to ensure the function does not look
++ * at msg beyond the len provided by the caller.
+ */
+ while (line && line < msg + len) {
const char *eol = strchrnul(line, '\n');
builtin/receive-pack.c | 33 ++++++++++-----------------------
commit.c | 16 ++++++++++++++--
commit.h | 5 +++++
3 files changed, 29 insertions(+), 25 deletions(-)
Hi Junio,
On 6 Jan 2022, at 15:07, John Cai via GitGitGadget wrote:
quoted hunk
From: John Cai <redacted>
There are two functions that have very similar logic of finding a header
value. find_commit_header, and find_header. We can conslidate the logic
by introducing a new function find_header_mem, which is equivalent to
find_commit_header except it takes a len parameter that determines how
many bytes will be read. find_commit_header and find_header can then both
call find_header_mem.
This reduces duplicate logic, as the logic for finding header values
can now all live in one place.
Signed-off-by: John Cai <redacted>
---
Consolidate find_header logic into one function
This addresses the NEEDSWORK comment in builtin/receive-pack.c:
/**
* NEEDSWORK: reuse find_commit_header() from jk/commit-author-parsing
* after dropping "_commit" from its name and possibly moving it out
* of commit.c
**/
There are two functions that have very similar logic of finding a header
value. find_commit_header, and find_header. We can conslidate the logic
by introducing a new function find_header_mem, which is equivalent to
find_commit_header except it takes a len parameter that determines how
many bytes will be read. find_commit_header and find_header can then
both call find_header_mem.
This reduces duplicate logic, as the logic for finding header values can
now all live in one place.
Changes since v4:
* adjust verbiage of NEEDSWORK comment block
Changes since v3:
* added NEEDSWORK block detailing what needs to be done to clean up
find_header_mem
* fixed verbiage in commit message
* adjusted style of an if block (based on Junio's feedback)
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1125%2Fjohn-cai%2Fjc%2Freplace-find-header-v6
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1125/john-cai/jc/replace-find-header-v6
Pull-Request: https://github.com/git/git/pull/1125
Range-diff vs v5:
1: 02da3136c43 ! 1: 2b01d92b951 receive-pack.c: consolidate find header logic
@@ commit.c: struct commit_list **commit_list_append(struct commit *commit,
+ * NEEDSWORK: It's possible for strchrnul() to scan beyond the range
+ * given by len. However, current callers are safe because they compute
+ * len by scanning a NUL-terminated block of memory starting at msg.
-+ * Make this function safer by checking if the input is NUL-terminated
-+ * or has a NL between line[0] and msg[len].
++ * Nonetheless, it would be better to ensure the function does not look
++ * at msg beyond the len provided by the caller.
+ */
+ while (line && line < msg + len) {
const char *eol = strchrnul(line, '\n');
builtin/receive-pack.c | 33 ++++++++++-----------------------
commit.c | 16 ++++++++++++++--
commit.h | 5 +++++
3 files changed, 29 insertions(+), 25 deletions(-)
+ while (line && line < msg + len) {
const char *eol = strchrnul(line, '\n');
if (line == eol)
@@ -1653,6 +1661,10 @@ const char *find_commit_header(const char *msg, const char *key, size_t *out_len return NULL; }+const char *find_commit_header(const char *msg, const char *key, size_t *out_len)+{+ return find_header_mem(msg, strlen(msg), key, out_len);+} /* * Inspect the given string and determine the true "end" of the log message, in * order to find where to put a new Signed-off-by trailer. Ignored are