Re: [PATCH v2] receive-pack.c: consolidate find header logic
From: Junio C Hamano <hidden>
Date: 2021-12-30 23:01:18
"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 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?
quoted hunk
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 ...
quoted hunk
{ 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.
quoted hunk
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);
quoted hunk
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.