Re: [PATCH 1/2] receive-pack.c: consolidate find header logic

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 27, 2021, 22:33 UTC
Message-ID: <xmqqtuetvft6.fsf@gitster.g>
In-Reply-To: <9465c20d4bd398dacbd7df2c068513c9ec484dd8.1640629598.git.gitgitgadget@gmail.com>

"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: John Cai <johncai86@gmail.com>
>
> 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

"consolidate".

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

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.

Show 22 quoted lines

> Signed-off-by: John Cai <johncai86@gmail.com>
> ---
>  builtin/receive-pack.c | 48 ++++++++++++++++++------------------------
>  commit.c               |  3 ++-
>  2 files changed, 23 insertions(+), 28 deletions(-)
>
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 4f92e6f059d..939d4b28b7c 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -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)

I do not think this change is quite right. &msg[len] in the original may not be the end of a NUL-terminated string, i.e. the caller does not want this helper to scan through to the end of the buffer but wants it to stop much earlier at the "len" the caller specifies.

Show 28 quoted lines

>  {
> -	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;

Also, find_commit_header() has already figured out what the next line should be. If it is not just telling us, we are forced to recompute it with an extra strchrnul(), but is that really the case?

HOWEVER.

Doesn't out_len have enough information to let us compute next_line without scanning the line again?

Show 5 quoted lines

> -	return NULL;
> +
> +	ret = xmalloc(out_len+1);
> +	memcpy(ret, val, out_len);
> +	ret[out_len] = '\0';

In any case, it is not necessary to open code xmemdupz() into these three lines, no?