[3/2] commit: remove find_header_mem()

Commit Message

René Scharfe June 19, 2024, 5:13 p.m. UTC

cfc5cf428b (receive-pack.c: consolidate find header logic, 2022-01-06)
introduced find_header_mem() and turned find_commit_header() into a thin
wrapper.  Since then, the latter has become the last remaining caller of
the former.  Remove it to restore find_commit_header() to the state
before cfc5cf428b, get rid of a strlen(3) call and resolve a NEEDSWORK
note in the process.

Signed-off-by: René Scharfe <[e-mail]>
---
 commit.c | 16 ++--------------
 commit.h |  5 -----
 2 files changed, 2 insertions(+), 19 deletions(-)

--
2.45.2

Comments

Jeff King June 19, 2024, 5:31 p.m. UTC | #1

On Wed, Jun 19, 2024 at 07:13:19PM +0200, René Scharfe wrote:

> cfc5cf428b (receive-pack.c: consolidate find header logic, 2022-01-06)
> introduced find_header_mem() and turned find_commit_header() into a thin
> wrapper.  Since then, the latter has become the last remaining caller of
> the former.  Remove it to restore find_commit_header() to the state
> before cfc5cf428b, get rid of a strlen(3) call and resolve a NEEDSWORK
> note in the process.

That of course made me wonder what happened to the other caller(s) of
find_header_mem(). The answer is that it went away in your 020456cb74
(receive-pack: use find_commit_header() in check_nonce(), 2024-02-09)

> -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)
>  {
>  	int key_len = strlen(key);
>  	const char *line = msg;

Not new in your patch, but assigning strlen() to int tingled my
spider-sense. It's OK, though, because "key" is always a small string
literal.

> -	/*
> -	 * 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) {
> +	while (line) {
>  		const char *eol = strchrnul(line, '\n');

OK, we no longer know the length of the message, but we don't need to
because it's NUL terminated, and strchrnul() will find the correct eol.
The length check might have saved us if we accidentally pushed "line"
past the NUL terminator, but it looks like we take care not to do so in
the loop body:

	line = *eol ? eol + 1 : NULL;

So the conversion looks good to me.

-Peff

Junio C Hamano June 20, 2024, 6:12 p.m. UTC | #2

Jeff King <[e-mail]> writes:

>> +const char *find_commit_header(const char *msg, const char *key, size_t *out_len)
>>  {
>>  	int key_len = strlen(key);
>>  	const char *line = msg;
>
> Not new in your patch, but assigning strlen() to int tingled my
> spider-sense. It's OK, though, because "key" is always a small string
> literal.

Yup.  All callers of find_commit_header() give in-program constants
and never an externally sourced random string there.

> So the conversion looks good to me.

Thanks.  Will queue.

Patch

diff --git a/commit.c b/commit.c
index 1d08951007..aacc401e72 100644
--- a/commit.c
+++ b/commit.c
@@ -1870,20 +1870,12 @@  struct commit_list **commit_list_append(struct commit *commit,
 	return &new_commit->next;
 }

-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)
 {
 	int key_len = strlen(key);
 	const char *line = msg;

-	/*
-	 * 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) {
+	while (line) {
 		const char *eol = strchrnul(line, '\n');

 		if (line == eol)
@@ -1900,10 +1892,6 @@  const char *find_header_mem(const char *msg, size_t 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
diff --git a/commit.h b/commit.h
index 62fe0d77a7..3084f591fd 100644
--- a/commit.h
+++ b/commit.h
@@ -280,17 +280,12 @@  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);