Re: [PATCH v2] 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
> 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.

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?

> diff --git a/commit.c b/commit.c
> index a348f085b2b..2ed115e04a0 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -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)

If <len> is meant to be the length part of <ptr,len> pair, we should
have it immediately after the <ptr> parameter.

	find_header_mem(const char *msg, size_t len,
        		const char *key, size_t *out_len)

That makes it clear to the readers that <msg, len> are close friends.

Also, I have a feeling that ...

>  {
>  	int key_len = strlen(key);
>  	const char *line = msg;
>  
> -	while (line) {
> +	while (line && (len == 0 || line < msg + len)) {

... we do not want this special casing of "if !len".  By making the
caller responsible to _always_ supply the length of msg, we can lose
the conditional.

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

I.e. find_header_mem(msg, strlen(msg), key, out_len);

> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 9f4a0b816cf..b69ead8dcda 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -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)
>  {
> +	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);
>  }

Yup, something along that line.  Note that find_header() does make
it clear that the <msg, len> parameters form a pair.  We want to do
the same for the new helper.