format-patch: fix from header in cover letter

From: Mroik <[email protected]>

Fixes "From" header for the cover letter when --from is passed

---
 builtin/log.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index d43ca693bf..df41b43fce 100644
@@ -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);
 
 	if (use_separate_file &&
 	    open_next_file(NULL, rev->numbered_files ? NULL : "cover-letter", rev, quiet))

2.53.0

Thanks. That reads well.

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

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 <[email protected]>'", 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.

Again, the ident used here matches what is used on the in-body From:
line, which is good.

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.

"git format-patch" takes "--from=<user ident>" command line option and uses the given ident for patch e-mails, but this is not applied to the cover letter, 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.

---
I'm sorry about the poor quality of the previous commit message, I realise it did a poor job at explaining what the patch did.

I've applied the changes you suggested and wrote the test. Thank you for the review.

 builtin/log.c           |  6 +++---
 t/t4014-format-patch.sh | 12 ++++++++++++
 2 files changed, 15 insertions(+), 3 deletions(-)

index d43ca693bf..42648dda54 100644
@@ -1332,7 +1332,7 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file,
 			      int quiet,
 			      const struct format_config *cfg)
 {
-	const char *committer;
+	const char *from;
 	struct shortlog log;
 	struct strbuf sb = STRBUF_INIT;
 	int i;

@@ -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);
+	from = cfg->from ? cfg->from : git_committer_info(0);
 
 	if (use_separate_file &&
 	    open_next_file(NULL, rev->numbered_files ? NULL : "cover-letter", rev, quiet))

@@ -1368,7 +1368,7 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file,
 	pp.date_mode.type = DATE_RFC2822;
 	pp.rev = rev;
 	pp.encode_email_headers = rev->encode_email_headers;
-	pp_user_info(&pp, NULL, &sb, committer, encoding);
+	pp_user_info(&pp, NULL, &sb, from, encoding);
 	prepare_cover_text(&pp, description_file, branch_name, &sb,
 			   encoding, need_8bit_cte, cfg);
 	fprintf(rev->diffopt.file, "%s\n", sb.buf);

index 21d6d0cd9e..cb04a9c47b 100755
@@ -1472,6 +1472,18 @@ test_expect_success '--from uses committer ident' '
 	test_cmp expect patch.head
 '
 
+test_expect_success '--from applies to cover letter' '
+	echo "This is a test text" >file_to_commit &&
+	git format-patch -1 --stdout --cover-letter --from="Foo Bar <[email protected]>" >patch &&
+	cat >expect <<-\EOF &&
+	From: Foo Bar <[email protected]>
+	From: Foo Bar <[email protected]>
+	From: A U Thor <[email protected]>
+	EOF
+	sed -ne "/^From:/p; /^[[:space:]]$/d" patch >patch.head &&
+	test_cmp expect patch.head
+'
+
 test_expect_success '--from omits redundant in-body header' '
 	git format-patch -1 --stdout --from="A U Thor <[email protected]>" >patch &&
 	cat >expect <<-\EOF &&

2.53.0

I think it makes sense to apply this identity to the cover letter, as well. After all, I would say that the whole intent is to allow the user to send those mails from their own mail address, and of course that would also require us to send the cover letter from that address.

One thing that made me stop though is the folowing sentence in git-format-patch(1):

Use ident in the From: header of each commit email.

The option explicitly mentions that we use "--from" for the commit emails, only, and that may be read as implying that it's not used for the cover letter.

I don't really know whether that wording is intentional, and I cannot come up with a good reason why it should be. But I'd say that the wording is something we should adjust.

  test_expect_success '--from applies to cover letter' '
  	test_when_finished "rm -rf patches" &&
  	git format-patch -1 --cover-letter --from="Foo Bar <[email protected]>" -o patches &&
  	echo "From: Foo Bar <[email protected]>" >expect &&
  	grep "^From:" patches/0000-cover-letter.patch >patch.head &&
  	test_cmp expect patch.head
  '

I think that wording comes from me back in 2013, and I just never gave any thought to cover-letter generation by format-patch itself. I never use it (and I probably forgot it even existed, given that most people would use send-email's cover letter generation, and I do my own thing with mutt).

So I think the direction of the patch is good, and I agree it would make sense to clarify the documentation.

-Peff

Funny: the send-email manual's main mention of cover letters apart from setting Cc/To fields is in an example…

           $ git format-patch --cover-letter -M origin/master -o outgoing/
           $ edit outgoing/0000-*
           $ git send-email outgoing/*

…using format-patch! (Which is how I generate cover letters, and probably part of why.)

D. Ben Knoble

I think I may be showing my clueless-ness about send-email here. I thought it had its own cover-letter code, but looking at it briefly, I may just be wrong. I don't see any code, so I guess people generate the cover-letter independently with format-patch.

Maybe nobody noticed because most people do not use "--from=<foo>" with a <foo> that does not match the committer ident in the first place. I'm not really sure why you'd want the two to differ. Which makes me wonder why I added that feature in the first place (as opposed to just "--from" with no options).

Mysteries of the cosmos, I guess.

-Peff

I usually commit everything under the name Mroik. Having the "--from" option is useful to send a one off contribution, otherwise I'd have to change my committer ident everytime I decide to use a different name (I know most people just use their full name so it's not common to have to use "--from").

Mirko

Yeah, maybe—send-email takes format-patch options, so send-email --cover--letter should also work?

:shrug: :)

D. Ben Knoble

Figures. If "--from" was done by somebody who did not know or care about the cover letter, it is totally understandable omission the patch addresses. The log message may want to refer to a9080475 (teach format-patch to place other authors into in-body "From", 2013-07-03).

I use it myself and I really hate the way it lists the patches. We should have done "log --oneline --reverse" instead of "shortlog", as it is hard to understand the reference the cover letter message makes to individual patches like "the first two patches do X", etc.

Yes. It does make sense to document the change in thinking in the proposed log message and in documentation.

Thanks.

Agreed. I have long hated the shortlog version. According to the commit history of my personal scripts, I've been sending with:

[1/3]: subject of the first patch
[2/3]: the second patch
[3/3]: and so on

since at least 2009, and nobody has complained. ;)

Sadly I don't think there is an easy way to do so with "log --format", as it can't just be a single output placeholder (you have to do the whole traversal to get the "/3" part, so the caller needs to know this and queue up the output).

I do it locally by piping the format-patch output through something like:

  sed -ne 's/^Subject: //p' |
  sed -e 's/\[PATCH /[/' \
      -e 's/]/]:/' \
      -e 's/^/  /'

Of course that does not handle rfc822 header continuations, which we started to do at some point for long subjects. So now the first part is some gnarly perl. :-/

Probably some combination of "git log --format=%s", "nl", "wc -l" would be less horrid. I stuck with format-patch because of its somewhat magical command-line parsing of revisions. E.g., saying "git format-patch @{upstream}" will give you the patches on the topic branch, rather than all history down to the roots.

But anyway, if this were happening internally in format-patch's cover-letter code, it should be quite easy to do (we have the complete list of patches and their subjects there already).

-Peff

A good bite-sized #leftoverbits project for somebody relatively new to the codebase, perhaps.

Thanks.

From: Mirko Faina <[email protected]>

"git format-patch" takes "--from=<user ident>" command line option and uses the given ident for patch e-mails, but this is not applied to the cover letter, 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.

---
 Documentation/git-format-patch.adoc | 10 +++++-----
 builtin/log.c                       |  6 +++---
 t/t4014-format-patch.sh             |  8 ++++++++
 3 files changed, 16 insertions(+), 8 deletions(-)

index 9a7807ca71..05c4192dbc 100644
@@ -282,11 +282,11 @@ e.g., `--rfc='-(WIP)'` results in "PATCH (WIP)".
 
 --from::
 --from=<ident>::
-	Use `ident` in the `From:` header of each commit email. If the
-	author ident of the commit is not textually identical to the
-	provided `ident`, place a `From:` header in the body of the
-	message with the original author. If no `ident` is given, use
-	the committer ident.
+	Use `ident` in the `From:` header of each email. In case of a
+	commit email, if the author ident of the commit is not textually
+	identical to the provided `ident`, place a `From:` header in the
+	body of the message with the original author. If no `ident` is
+	given, use the committer ident.
 +
 Note that this option is only useful if you are actually sending the
 emails and want to identify yourself as the sender, but retain the

index d43ca693bf..42648dda54 100644
@@ -1332,7 +1332,7 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file,
 			      int quiet,
 			      const struct format_config *cfg)
 {
-	const char *committer;
+	const char *from;
 	struct shortlog log;
 	struct strbuf sb = STRBUF_INIT;
 	int i;

@@ -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);
+	from = cfg->from ? cfg->from : git_committer_info(0);
 
 	if (use_separate_file &&
 	    open_next_file(NULL, rev->numbered_files ? NULL : "cover-letter", rev, quiet))

@@ -1368,7 +1368,7 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file,
 	pp.date_mode.type = DATE_RFC2822;
 	pp.rev = rev;
 	pp.encode_email_headers = rev->encode_email_headers;
-	pp_user_info(&pp, NULL, &sb, committer, encoding);
+	pp_user_info(&pp, NULL, &sb, from, encoding);
 	prepare_cover_text(&pp, description_file, branch_name, &sb,
 			   encoding, need_8bit_cte, cfg);
 	fprintf(rev->diffopt.file, "%s\n", sb.buf);

index 21d6d0cd9e..2135b65cee 100755
@@ -1472,6 +1472,14 @@ test_expect_success '--from uses committer ident' '
 	test_cmp expect patch.head
 '
 
+test_expect_success '--from applies to cover letter' '
+	test_when_finished "rm -rf patches" &&
+	git format-patch -1 --cover-letter --from="Foo Bar <[email protected]>" -o patches &&
+	echo "From: Foo Bar <[email protected]>" >expect &&
+	grep "^From:" patches/0000-cover-letter.patch >patch.head &&
+	test_cmp expect patch.head
+'
+
 test_expect_success '--from omits redundant in-body header' '
 	git format-patch -1 --stdout --from="A U Thor <[email protected]>" >patch &&
 	cat >expect <<-\EOF &&

2.53.0

I'm sorry, I forgot to mark as v3 and to change the sender name.

Heh, the sender name (as long as it is clear who the messages is from) does not really matter very much. Thanks for working on this.

It is worth noting that ever since this "--from" option was introduced, it was not used when generating the cover letter in the proposed log message in the above paragraph somewhere.

Also the reasoning behind the rename of "committer" to "from" is worth noting here.

"git format-patch" takes "--from=<user ident>" command line option and uses the given ident for patch e-mails, but this is not applied to the cover letter, the option is ignored and the committer ident of the current user is used. This has been the case ever since "--from" has been introduced as it has never been used in make_cover_letter().

Teach the make_cover_letter() function to honor the option, instead of always using the current committer identity. Change variable name from "committer" to "from" to better reflect the purpose of the variable.

---
 Documentation/git-format-patch.adoc | 11 ++++++-----
 builtin/log.c                       |  6 +++---
 t/t4014-format-patch.sh             |  8 ++++++++
 3 files changed, 17 insertions(+), 8 deletions(-)

index 9a7807ca71..6ac453958c 100644
@@ -282,11 +282,12 @@ e.g., `--rfc='-(WIP)'` results in "PATCH (WIP)".
 
 --from::
 --from=<ident>::
-	Use `ident` in the `From:` header of each commit email. If the
-	author ident of the commit is not textually identical to the
-	provided `ident`, place a `From:` header in the body of the
-	message with the original author. If no `ident` is given, use
-	the committer ident.
+	Use `ident` in the `From:` header of each email. In case of a
+	commit email, if the author ident of the commit is not textually
+	identical to the provided `ident`, place a `From:` header in the
+	body of the message with the original author. If no `ident` is
+	given, or if the option is not passed at all, use the ident of
+	the current committer.
 +
 Note that this option is only useful if you are actually sending the
 emails and want to identify yourself as the sender, but retain the

index d43ca693bf..42648dda54 100644
@@ -1332,7 +1332,7 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file,
 			      int quiet,
 			      const struct format_config *cfg)
 {
-	const char *committer;
+	const char *from;
 	struct shortlog log;
 	struct strbuf sb = STRBUF_INIT;
 	int i;

@@ -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);
+	from = cfg->from ? cfg->from : git_committer_info(0);
 
 	if (use_separate_file &&
 	    open_next_file(NULL, rev->numbered_files ? NULL : "cover-letter", rev, quiet))

@@ -1368,7 +1368,7 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file,
 	pp.date_mode.type = DATE_RFC2822;
 	pp.rev = rev;
 	pp.encode_email_headers = rev->encode_email_headers;
-	pp_user_info(&pp, NULL, &sb, committer, encoding);
+	pp_user_info(&pp, NULL, &sb, from, encoding);
 	prepare_cover_text(&pp, description_file, branch_name, &sb,
 			   encoding, need_8bit_cte, cfg);
 	fprintf(rev->diffopt.file, "%s\n", sb.buf);

index 21d6d0cd9e..2135b65cee 100755
@@ -1472,6 +1472,14 @@ test_expect_success '--from uses committer ident' '
 	test_cmp expect patch.head
 '
 
+test_expect_success '--from applies to cover letter' '
+	test_when_finished "rm -rf patches" &&
+	git format-patch -1 --cover-letter --from="Foo Bar <[email protected]>" -o patches &&
+	echo "From: Foo Bar <[email protected]>" >expect &&
+	grep "^From:" patches/0000-cover-letter.patch >patch.head &&
+	test_cmp expect patch.head
+'
+
 test_expect_success '--from omits redundant in-body header' '
 	git format-patch -1 --stdout --from="A U Thor <[email protected]>" >patch &&
 	cat >expect <<-\EOF &&

2.53.0