| Message ID | 9465c20d4bd398dacbd7df2c068513c9ec484dd8.1640629598.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Consolidate find_header logic into one function | expand |
"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?
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) { - 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; } - return NULL; + + ret = xmalloc(out_len+1); + memcpy(ret, val, out_len); + ret[out_len] = '\0'; + + return ret; } /* @@ -625,7 +619,8 @@ static int constant_memequal(const char *a, const char *b, size_t n) static const char *check_nonce(const char *buf, size_t len) { - char *nonce = find_header(buf, len, "nonce", NULL); + char *nonce = find_header_value(buf, "nonce", NULL); + timestamp_t stamp, ostamp; char *bohmac, *expect = NULL; const char *retval = NONCE_BAD; @@ -730,8 +725,7 @@ static int check_cert_push_options(const struct string_list *push_options) if (!len) return 1; - while ((option = find_header(buf, len, "push-option", &next_line))) { - len -= (next_line - buf); + while ((option = find_header_value(buf, "push-option", &next_line))) { buf = next_line; options_seen++; if (options_seen > push_options->nr diff --git a/commit.c b/commit.c index a348f085b2b..616a6780703 100644 --- a/commit.c +++ b/commit.c @@ -1645,7 +1645,8 @@ const char *find_commit_header(const char *msg, const char *key, size_t *out_len if (eol - line > key_len && !strncmp(line, key, key_len) && line[key_len] == ' ') { - *out_len = eol - line - key_len - 1; + if (out_len != NULL) + *out_len = eol - line - key_len - 1; return line + key_len + 1; } line = *eol ? eol + 1 : NULL;