Re: [PATCH v5 1/4] revision: improve commit_rewrite_person()

[Junio C Hamano <gitster@pobox.com>]

Siddharth Asthana <siddharthasthana31@gmail.com> writes:

> +/*
> + * 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.

> +{
> +	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++".

> +	}
> +}
> +
>  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 */