Re: [PATCH 1/2] receive-pack.c: consolidate find header logic
From: Junio C Hamano <hidden>
Date: 2021-12-27 22:33:55
"John Cai via GitGitGadget" [off-list ref] writes:
From: John Cai <redacted> 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.
quoted hunk
Signed-off-by: John Cai <redacted> --- 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.
quoted hunk
{ - 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?
quoted hunk
- 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?