Re: [PATCH v5 1/4] revision: improve commit_rewrite_person()
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jul 17, 2022, 22:11 UTC
- Message-ID
- <xmqqo7xnv17x.fsf@gitster.g>
- In-Reply-To
- <20220716074055.1786231-2-siddharthasthana31@gmail.com>
Siddharth Asthana <siddharthasthana31@gmail.com> writes:
Show 5 quoted lines
> +/* > + * Returns the difference between the new and old length of the ident line. > + */ > +static ssize_t rewrite_ident_line(const char *person, struct strbuf *buf, > + struct string_list *mailmap)
Line-folding is a good idea, but why do we use such a deep indentation? In this project, tab-width is 8.
> +static void commit_rewrite_person(struct strbuf *buf, const char **header, > + struct string_list *mailmap)
Likewise.
Show 17 quoted lines
> +{
> + size_t buf_offset = 0;
> +
> + if (!mailmap)
> + return;
> +
> + for (;;) {
> + const char *person, *line;
> + size_t i;
> +
> + line = buf->buf + buf_offset;
> + if (!*line || *line == '\n')
> + return; /* End of header */
> +
> + for (i = 0; header[i]; i++)
> + if (skip_prefix(line, header[i], &person)) {
> + rewrite_ident_line(person, buf, mailmap);If the return value of rewrite_ident_line() is never used, perhaps stop computing the return value in that function and make it return "void". I personally thought it was clever to return "how much does the ident part grew/shrunk?" from the helper and use it to adjust, but I do not mind to scrap the clever-ness if some folks may find it harder to understand.
> + break; > + } > + > + buf_offset = strchrnul(buf->buf + buf_offset, '\n') - buf->buf;
And this is a "easier-to-understand but need to scan the buffer once again, only to figure out what we ought to already know" version.
> + if (buf->buf[buf_offset] == '\n') > + ++buf_offset;
Prefer post-increment when there is no reason to favor pre-increment over post-increment, i.e. write this as "buf_offset++".
Show 21 quoted lines
> + }
> +}
> +
> static int commit_match(struct commit *commit, struct rev_info *opt)
> {
> int retval;
> @@ -3832,11 +3862,12 @@ static int commit_match(struct commit *commit, struct rev_info *opt)
> strbuf_addstr(&buf, message);
>
> if (opt->grep_filter.header_list && opt->mailmap) {
> + const char *commit_headers[] = { "author ", "committer ", NULL };
> +
> if (!buf.len)
> strbuf_addstr(&buf, message);
>
> - commit_rewrite_person(&buf, "\nauthor ", opt->mailmap);
> - commit_rewrite_person(&buf, "\ncommitter ", opt->mailmap);
> + commit_rewrite_person(&buf, commit_headers, opt->mailmap);
> }
>
> /* Append "fake" message parts as needed */