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

[Junio C Hamano <gitster@pobox.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.

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

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

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