Re: [PATCH] format-patch: fix from header in cover letter

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

Mirko Faina <mroik@delayed.space> writes:

> Subject: Re: [PATCH] format-patch: fix from header in cover letter

Thanks.  That reads well.

> 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.

> 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))

Thanks.