Re: [PATCH] format-patch: fix from header in cover letter
Mirko Faina <mroik@delayed.space> writes:
> Subject: Re: [PATCH] format-patch: fix from header in cover letter
> From: Mroik <mroik@delayed.space>
The ident used here matches what is used on the signed-off-by line, which is good. We tend to prefer people to use their real names, though (and I suspect that your name probably has some multi-word structure, not a single-word).
> Fixes "From" header for the cover letter when `--from` is passed
The usual way to compose a log message of this project is to
- Give an observation on how the current system works in the
present tense (so no need to say "Currently X is Y", or
"Previously X was Y" to describe the state before your change;
just "X is Y" is enough), and discuss what you perceive as a
problem in it.
- Propose a solution (optional---often, problem description
trivially leads to an obvious solution in reader's minds).
- Give commands to somebody editing the codebase to "make it so",
instead of saying "This commit does X".
in this order. I am guessing that the problem you are fixing, which is not said explicitly in what you wrote, is that even when you run "git format-patch --from='Me <me@my.address>'", the option is not used for cover letter? If that is the case, perhaps I'd write something along this line:
"git format-patch" takes "--from=<user ident>" command line
option, and the given ident is used for patch e-mails, but for
the cover letter e-mail, the option is ignored and the committer
ident of the current user is used. Teach the make_cover_letter() function to honor the option,
instead of always using the current committer identity.if I were making this change.
> Signed-off-by: Mroik <mroik@delayed.space>
Again, the ident used here matches what is used on the in-body From: line, which is good.
> ---
> builtin/log.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
This can be tested easily without flakes, so we should add some test in one of the t/t*-format-patch*.sh files to help people notice when they breaks this fix by mistake in the future.
Show 10 quoted lines
> diff --git a/builtin/log.c b/builtin/log.c
> index d43ca693bf..df41b43fce 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -1345,7 +1345,7 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file,
> if (!cmit_fmt_is_mail(rev->commit_format))
> die(_("cover letter needs email format"));
>
> - committer = git_committer_info(0);
> + committer = cfg->from ? cfg->from : git_committer_info(0);This is a correct and minimal change, but if I were making this change, I'd probably rename the "committer" variable. Before the "--from" option came into play, the function has always assumed that it should make the cover letter message from the current user, using their committer ident, which meant that the "From:" address and the committer ident were synonymous from this function's point of view.
But now we are telling the function that its assumption is no longer good, and teaching it to honor the "From:" address given in the cfg. And the way this local variable is used is to point at a string that is the ident used for the "From:" address, which is not necessarily the same as committer ident in this new world order. So it is not reader-friendly to keep using "committer" as the name of the variable. Simply call it "from", perhaps?
> if (use_separate_file &&
> open_next_file(NULL, rev->numbered_files ? NULL : "cover-letter", rev, quiet))
Mirko Faina
