Re: [PATCH 2/3] ident: rename commit_rewrite_person() to rewrite_ident_line()

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 30, 2022, 23:31 UTC
Message-ID: <xmqqa69tbue8.fsf@gitster.g>
In-Reply-To: <20220630142444.651948-3-siddharthasthana31@gmail.com>

Siddharth Asthana <siddharthasthana31@gmail.com> writes:

Show 9 quoted lines

> We will be using commit_rewrite_person() in git-cat-file to rewrite
> ident line in commit/tag object buffers.
>
> Following are the reason for renaming commit_rewrite_person():
> - the function can be used not only on a commit buffer, but also on a
>   tag object buffer, so having "commit" in its name is misleading.
> - the function works on the ident line in the commit/tag object buffers,
>   just like "split_ident_line()". Since these functions are related they
>   should have similar terms for uniformity.

"ident" is good (so is "person" in the original).

"rewrite" is not quite good, as you do not convey what kind of rewrite you are doing (it is not likely that you are upcasing their names, but from "rewrite" you cannot tell that is the case). We should have "mailmap" somewhere in its name.

"line" is not good at all. This function receives an entire buffer, not just a single line that was located by the caller and rewrites that line. "buffer" might be an improvement over "line", but as I alluded to in my review on [1/3], I think this function should limit itself only to the header part of the commit/tag buffer, so "header" would be the word you want in its name.

Once we say "mailmap", there is not much point in saying ident or person (as "mailmap" is only about the ident/person). So

  int apply_mailmap_to_header(struct strbuf *, struct string_list *mailmap)

or something?

> Signed-off-by: Siddharth Asthana <siddharthasthana31@gmail.com>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: John Cai <johncai86@gmail.com>

I think these are the other way around. With help from two people, you wrote the patch and the final step before sending it out to the list is your sign-off. Chronologically, in the above, your sign-off should come at the end.