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

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

Mroik <mroik@delayed.space> writes:

> From: Mirko Faina <mroik@delayed.space>
>
> "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.

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.

> Teach the make_cover_letter() function to honor the option, instead of
> always using the current committer identity.

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

> diff --git a/Documentation/git-format-patch.adoc b/Documentation/git-format-patch.adoc
> index 9a7807ca71..05c4192dbc 100644
> --- a/Documentation/git-format-patch.adoc
> +++ b/Documentation/git-format-patch.adoc
> @@ -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.

After reading the above three times, a natural question that comes
to mind is what ident is used when this option is not given at all
(i.e., "When this option is not given, the committer identity of the
current user is used for all messages").

> diff --git a/builtin/log.c b/builtin/log.c
> index d43ca693bf..42648dda54 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c

The code changes all look good.

> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> index 21d6d0cd9e..2135b65cee 100755
> --- a/t/t4014-format-patch.sh
> +++ b/t/t4014-format-patch.sh
> @@ -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 <author@example.com>" -o patches &&
> +	echo "From: Foo Bar <author@example.com>" >expect &&
> +	grep "^From:" patches/0000-cover-letter.patch >patch.head &&
> +	test_cmp expect patch.head
> +'

OK.

We know what filename the cover letter gets, so it is really the
matter of how we want to verify the output.  We know that a line
that matches "^From:" must appear once and only once in the cover
letter output, so finding that line and comparing it with what we
expect to see is a reasonably way to do this.  Good.

Thanks.