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.

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 <[email protected]>

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.

---
 builtin/receive-pack.c | 48 ++++++++++++++++++------------------------
 commit.c               |  3 ++-
 2 files changed, 23 insertions(+), 28 deletions(-)

index 4f92e6f059d..939d4b28b7c 100644
@@ -581,32 +581,26 @@ static char *prepare_push_cert_nonce(const char *path, timestamp_t stamp)
 	return strbuf_detach(&buf, NULL);
 }
 
-/*
- * NEEDSWORK: reuse find_commit_header() from jk/commit-author-parsing
- * 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)
 {
-	int key_len = strlen(key);
-	const char *line = msg;
-
-	while (line && line < msg + len) {
-		const char *eol = strchrnul(line, '\n');
-
-		if ((msg + len <= eol) || line == eol)
-			return NULL;
-		if (line + key_len < eol &&
-		    !memcmp(line, key, key_len) && line[key_len] == ' ') {
-			int offset = key_len + 1;
-			if (next_line)
-				*next_line = *eol ? eol + 1 : eol;
-			return xmemdupz(line + offset, (eol - line) - offset);
-		}
-		line = *eol ? eol + 1 : NULL;
+	size_t out_len;
+	const char *eol;
+	char *ret;
+
+	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';
+
+	return ret;
 }
 
 /*

@@ -625,7 +619,8 @@ static int constant_memequal(const char *a, const char *b, size_t n)
 
 static const char *check_nonce(const char *buf, size_t 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;

@@ -730,8 +725,7 @@ static int check_cert_push_options(const struct string_list *push_options)
 	if (!len)
 		return 1;
 
-	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

index a348f085b2b..616a6780703 100644
@@ -1645,7 +1645,8 @@ 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;

gitgitgadget

From: John Cai <[email protected]>

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.

---
 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(-)

index 8677ea2348a..1ed5071ff49 100644
@@ -1312,7 +1312,7 @@ static void get_commit_info(struct am_state *state, struct commit *commit)
 
 	buffer = logmsg_reencode(commit, NULL, get_commit_output_encoding());
 
-	ident_line = find_commit_header(buffer, "author", &ident_len);
+	ident_line = find_header(buffer, "author", &ident_len);
 	if (!ident_line)
 		die(_("missing author line in commit %s"),
 		      oid_to_hex(&commit->object.oid));

index 883c16256c8..6fbb154ee32 100644
@@ -614,7 +614,7 @@ static void determine_author_info(struct strbuf *author_ident)
 		size_t len;
 		const char *a;
 
-		a = find_commit_header(author_message_buffer, "author", &len);
+		a = find_header(author_message_buffer, "author", &len);
 		if (!a)
 			die(_("commit '%s' lacks author header"), author_message);
 		if (split_ident_line(&ident, a, len) < 0)

index 939d4b28b7c..f0fc826b665 100644
@@ -581,13 +581,13 @@ static char *prepare_push_cert_nonce(const char *path, timestamp_t stamp)
 	return strbuf_detach(&buf, NULL);
 }
 
-static char *find_header_value(const char *msg, const char *key, const char **next_line)
+static char *find_header_nextline(const char *msg, const char *key, const char **next_line)
 {
 	size_t out_len;
 	const char *eol;
 	char *ret;
 
-	const char *val = find_commit_header(msg, key, &out_len);
+	const char *val = find_header(msg, key, &out_len);
 	if (val == NULL)
 		return NULL;
 

@@ -619,7 +619,7 @@ static int constant_memequal(const char *a, const char *b, size_t n)
 
 static const char *check_nonce(const char *buf, size_t len)
 {
-	char *nonce = find_header_value(buf, "nonce", NULL);
+	char *nonce = find_header_nextline(buf, "nonce", NULL);
 
 	timestamp_t stamp, ostamp;
 	char *bohmac, *expect = NULL;

@@ -725,7 +725,7 @@ static int check_cert_push_options(const struct string_list *push_options)
 	if (!len)
 		return 1;
 
-	while ((option = find_header_value(buf, "push-option", &next_line))) {
+	while ((option = find_header_nextline(buf, "push-option", &next_line))) {
 		buf = next_line;
 		options_seen++;
 		if (options_seen > push_options->nr

index 616a6780703..bdedbe295f5 100644
@@ -734,7 +734,7 @@ void record_author_date(struct author_date_slab *author_date,
 	char *date_end;
 	timestamp_t date;
 
-	ident_line = find_commit_header(buffer, "author", &ident_len);
+	ident_line = find_header(buffer, "author", &ident_len);
 	if (!ident_line)
 		goto fail_exit; /* no author line */
 	if (split_ident_line(&ident, ident_line, ident_len) ||

@@ -1631,7 +1631,7 @@ struct commit_list **commit_list_append(struct commit *commit,
 	return &new_commit->next;
 }
 
-const char *find_commit_header(const char *msg, const char *key, size_t *out_len)
+const char *find_header(const char *msg, const char *key, size_t *out_len)
 {
 	int key_len = strlen(key);
 	const char *line = msg;

index 3ea32766bcb..18a1460c4b4 100644
@@ -296,7 +296,7 @@ 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_commit_header(const char *msg, const char *key,
+const char *find_header(const char *msg, const char *key,
 			       size_t *out_len);
 
 /* Find the end of the log message, the right place for a new trailer. */

index b52eb0e2e04..c1bc9fa0459 100644
@@ -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)
 		return 1;
 

index ee6114e3f0a..306c647d2d7 100644
@@ -633,7 +633,7 @@ static void add_merge_info(const struct pretty_print_context *pp,
 static char *get_header(const char *msg, const char *key)
 {
 	size_t len;
-	const char *v = find_commit_header(msg, key, &len);
+	const char *v = find_header(msg, key, &len);
 	return v ? xmemdupz(v, len) : NULL;
 }
 

index e314af4d60a..ac535868121 100644
@@ -912,7 +912,7 @@ static char *get_author(const char *message)
 	size_t len;
 	const char *a;
 
-	a = find_commit_header(message, "author", &len);
+	a = find_header(message, "author", &len);
 	if (a)
 		return xmemdupz(a, len);
 

index fc2d4609043..576c2366258 100644
@@ -44,7 +44,7 @@ static char *get_author(const char *message)
 	size_t len;
 	const char *a;
 
-	a = find_commit_header(message, "author", &len);
+	a = find_header(message, "author", &len);
 	if (a)
 		return xmemdupz(a, len);
 

gitgitgadget

"consolidate".

It does make sense to split the renaming and this change into two separate steps like this series does. The renaming done in 2/2 however makes readers wonder if our existing code paths that handle tag objects becomes cleaner by using the function (and if not, if the perceived benefit of making it into a more generic name is a mere mirage), though.

From: John Cai <[email protected]>

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.

---
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.

@@ 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(-)

index 9f4a0b816cf..b69ead8dcda 100644
@@ -581,32 +581,19 @@ static char *prepare_push_cert_nonce(const char *path, timestamp_t stamp)
 	return strbuf_detach(&buf, NULL);
 }
 
-/*
- * NEEDSWORK: reuse find_commit_header() from jk/commit-author-parsing
- * 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)
 {
-	int key_len = strlen(key);
-	const char *line = msg;
-
-	while (line && line < msg + len) {
-		const char *eol = strchrnul(line, '\n');
-
-		if ((msg + len <= eol) || line == eol)
-			return NULL;
-		if (line + key_len < eol &&
-		    !memcmp(line, key, key_len) && line[key_len] == ' ') {
-			int offset = key_len + 1;
-			if (next_line)
-				*next_line = *eol ? eol + 1 : eol;
-			return xmemdupz(line + offset, (eol - line) - offset);
-		}
-		line = *eol ? eol + 1 : NULL;
-	}
-	return NULL;
+	size_t out_len;
+	const char *val = find_header_max(msg, key, len, &out_len);
+
+	if (val == NULL)
+		return NULL;
+
+	if (next_line)
+		*next_line = val + out_len + 1;
+
+	return xmemdupz(val, out_len);
 }
 
 /*

index a348f085b2b..2ed115e04a0 100644
@@ -1631,12 +1631,14 @@ struct commit_list **commit_list_append(struct commit *commit,
 	return &new_commit->next;
 }
 
-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)
 {
 	int key_len = strlen(key);
 	const char *line = msg;
 
-	while (line) {
+	while (line && (len == 0 || line < msg + len)) {
 		const char *eol = strchrnul(line, '\n');
 
 		if (line == eol)

@@ -1653,6 +1655,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_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

index 3ea32766bcb..41ec89af5b5 100644
@@ -290,12 +290,17 @@ 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);
 

base-commit: 55b058a8bbcc54bd93c733035c995abc7967e539

gitgitgadget

find_header_max() is not the name of the function that finds the largest header? That is misleading.

<git-compat-util.h> defines a few helper functions that take a counted string, and they are named with _mem() suffix after the name of their NUL-terminated counterparts (if exists). skip_prefix() has skip_prefix_mem(), strip_suffix() has strip_suffix_mem().

find_header_mem() or something along that line, perhaps?

From: John Cai <[email protected]>

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.

---
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(-)

index 9f4a0b816cf..b661c51a538 100644
@@ -581,32 +581,19 @@ static char *prepare_push_cert_nonce(const char *path, timestamp_t stamp)
 	return strbuf_detach(&buf, NULL);
 }
 
-/*
- * NEEDSWORK: reuse find_commit_header() from jk/commit-author-parsing
- * 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)
 {
-	int key_len = strlen(key);
-	const char *line = msg;
-
-	while (line && line < msg + len) {
-		const char *eol = strchrnul(line, '\n');
-
-		if ((msg + len <= eol) || line == eol)
-			return NULL;
-		if (line + key_len < eol &&
-		    !memcmp(line, key, key_len) && line[key_len] == ' ') {
-			int offset = key_len + 1;
-			if (next_line)
-				*next_line = *eol ? eol + 1 : eol;
-			return xmemdupz(line + offset, (eol - line) - offset);
-		}
-		line = *eol ? eol + 1 : NULL;
-	}
-	return NULL;
+	size_t out_len;
+	const char *val = find_header_mem(msg, len, key, &out_len);
+
+	if (val == NULL)
+		return NULL;
+
+	if (next_line)
+		*next_line = val + out_len + 1;
+
+	return xmemdupz(val, out_len);
 }
 
 /*

index a348f085b2b..8ac32a4d7b5 100644
@@ -1631,12 +1631,13 @@ struct commit_list **commit_list_append(struct commit *commit,
 	return &new_commit->next;
 }
 
-const char *find_commit_header(const char *msg, const char *key, 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 && line < msg + len) {
 		const char *eol = strchrnul(line, '\n');
 
 		if (line == eol)

@@ -1653,6 +1654,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

index 3ea32766bcb..38cc5426615 100644
@@ -290,12 +290,17 @@ 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_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,
 			       size_t *out_len);
 

base-commit: 55b058a8bbcc54bd93c733035c995abc7967e539

gitgitgadget

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.

From: John Cai <[email protected]>

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.

---
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.

@@ 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(-)

index 9f4a0b816cf..5c2732a0d07 100644
@@ -581,32 +581,19 @@ static char *prepare_push_cert_nonce(const char *path, timestamp_t stamp)
 	return strbuf_detach(&buf, NULL);
 }
 
-/*
- * NEEDSWORK: reuse find_commit_header() from jk/commit-author-parsing
- * 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)
 {
-	int key_len = strlen(key);
-	const char *line = msg;
-
-	while (line && line < msg + len) {
-		const char *eol = strchrnul(line, '\n');
-
-		if ((msg + len <= eol) || line == eol)
-			return NULL;
-		if (line + key_len < eol &&
-		    !memcmp(line, key, key_len) && line[key_len] == ' ') {
-			int offset = key_len + 1;
-			if (next_line)
-				*next_line = *eol ? eol + 1 : eol;
-			return xmemdupz(line + offset, (eol - line) - offset);
-		}
-		line = *eol ? eol + 1 : NULL;
-	}
-	return NULL;
+	size_t out_len;
+	const char *val = find_header_mem(msg, len, key, &out_len);
+
+	if (!val)
+		return NULL;
+
+	if (next_line)
+		*next_line = val + out_len + 1;
+
+	return xmemdupz(val, out_len);
 }
 
 /*

index a348f085b2b..5ece03e6373 100644
@@ -1631,12 +1631,18 @@ struct commit_list **commit_list_append(struct commit *commit,
 	return &new_commit->next;
 }
 
-const char *find_commit_header(const char *msg, const char *key, 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) {
+	/*
+	 * 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');
 
 		if (line == eol)

@@ -1653,6 +1659,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

index 3ea32766bcb..38cc5426615 100644
@@ -290,12 +290,17 @@ 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_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,
 			       size_t *out_len);
 

base-commit: c8b2ade48c204690119936ada89cd938c476c5c2

gitgitgadget

This sounds unnecessarily alarming. Can't we also explain that the current callers are safe?

From: John Cai <[email protected]>

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.

---
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(-)

index 9f4a0b816cf..5c2732a0d07 100644
@@ -581,32 +581,19 @@ static char *prepare_push_cert_nonce(const char *path, timestamp_t stamp)
 	return strbuf_detach(&buf, NULL);
 }
 
-/*
- * NEEDSWORK: reuse find_commit_header() from jk/commit-author-parsing
- * 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)
 {
-	int key_len = strlen(key);
-	const char *line = msg;
-
-	while (line && line < msg + len) {
-		const char *eol = strchrnul(line, '\n');
-
-		if ((msg + len <= eol) || line == eol)
-			return NULL;
-		if (line + key_len < eol &&
-		    !memcmp(line, key, key_len) && line[key_len] == ' ') {
-			int offset = key_len + 1;
-			if (next_line)
-				*next_line = *eol ? eol + 1 : eol;
-			return xmemdupz(line + offset, (eol - line) - offset);
-		}
-		line = *eol ? eol + 1 : NULL;
-	}
-	return NULL;
+	size_t out_len;
+	const char *val = find_header_mem(msg, len, key, &out_len);
+
+	if (!val)
+		return NULL;
+
+	if (next_line)
+		*next_line = val + out_len + 1;
+
+	return xmemdupz(val, out_len);
 }
 
 /*

index a348f085b2b..f5224e65de2 100644
@@ -1631,12 +1631,20 @@ struct commit_list **commit_list_append(struct commit *commit,
 	return &new_commit->next;
 }
 
-const char *find_commit_header(const char *msg, const char *key, 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) {
+	/*
+	 * 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');
 
 		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

index 3ea32766bcb..38cc5426615 100644
@@ -290,12 +290,17 @@ 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_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,
 			       size_t *out_len);
 

base-commit: c8b2ade48c204690119936ada89cd938c476c5c2

gitgitgadget

OK. Because the caller didn't die with SIGBUS owhen they scanned the memory to figure out len, we know scanning between msg[0] and msg[len] is safe.

This comment however feels wrong. The function is _safe_ if the input is NUL-terminated already. And it is very expected that the msg[] has LF between msg[0] and msg[len]---after all that is why we use strchrnul() to scan it.

Perhaps replace it with something like "But nevertheless it would be better to make sure the function does not look at msg beyond len that was provided by the caller", perhaps?

The most problematic case happens when msg points into somewhere inside a page and &msg[len] is still inside that page, but (1) there is no LF nor NUL in the page after &msg[0], and (2) the page after the page &msg[0] and &msg[len] are in is not mapped. Letting strchrnul() scan msg[] beyond len will lead us to attempt to read the first byte of the next page and killed with SIGBUS (I recall that I had to debug a mysterious breakage in strlen or strchr on SunOS, which tried to grab 8-byte at a time and crossed page boundary to get an unnecessary SIGBUS).

From: John Cai <[email protected]>

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.

---
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(-)

index 9f4a0b816cf..5c2732a0d07 100644
@@ -581,32 +581,19 @@ static char *prepare_push_cert_nonce(const char *path, timestamp_t stamp)
 	return strbuf_detach(&buf, NULL);
 }
 
-/*
- * NEEDSWORK: reuse find_commit_header() from jk/commit-author-parsing
- * 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)
 {
-	int key_len = strlen(key);
-	const char *line = msg;
-
-	while (line && line < msg + len) {
-		const char *eol = strchrnul(line, '\n');
-
-		if ((msg + len <= eol) || line == eol)
-			return NULL;
-		if (line + key_len < eol &&
-		    !memcmp(line, key, key_len) && line[key_len] == ' ') {
-			int offset = key_len + 1;
-			if (next_line)
-				*next_line = *eol ? eol + 1 : eol;
-			return xmemdupz(line + offset, (eol - line) - offset);
-		}
-		line = *eol ? eol + 1 : NULL;
-	}
-	return NULL;
+	size_t out_len;
+	const char *val = find_header_mem(msg, len, key, &out_len);
+
+	if (!val)
+		return NULL;
+
+	if (next_line)
+		*next_line = val + out_len + 1;
+
+	return xmemdupz(val, out_len);
 }
 
 /*

index a348f085b2b..28391c3468d 100644
@@ -1631,12 +1631,20 @@ struct commit_list **commit_list_append(struct commit *commit,
 	return &new_commit->next;
 }
 
-const char *find_commit_header(const char *msg, const char *key, 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) {
+	/*
+	 * 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.
+	 * 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');
 
 		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

index 3ea32766bcb..38cc5426615 100644
@@ -290,12 +290,17 @@ 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_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,
 			       size_t *out_len);
 

base-commit: c8b2ade48c204690119936ada89cd938c476c5c2

gitgitgadget

Hi Junio,