Re: [PATCH v3 4/4] format-patch: add format.cover-letter configuration

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 7, 2013, 08:14 UTC
Message-ID: <7v4nfi92q1.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1365318630-11882-5-git-send-email-felipe.contreras@gmail.com>

Felipe Contreras <felipe.contreras@gmail.com> writes:

Show 19 quoted lines

> Also, add a new option: 'auto', so if there's more than one patch, the
> cover letter is generated, otherwise it's not.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  Documentation/config.txt           |  6 ++++++
>  Documentation/git-format-patch.txt |  5 +++--
>  builtin/log.c                      | 22 ++++++++++++++++------
>  3 files changed, 25 insertions(+), 8 deletions(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index c8e2178..c10195c 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1092,6 +1092,12 @@ format.signoff::
>      the rights to submit this work under the same open source license.
>      Please see the 'SubmittingPatches' document for further discussion.
>  
> +format.cover-letter::

I thought I saw somebody already pointed out that this was wrong.

Show 5 quoted lines

> +	Allows to configure the --cover-letter option of format-patch by
> +	default.
> +	In addition, you can set it to 'auto' to automatically
> +	determine based on the number of patches (generate if there's more than
> +	one).

I find this somewhat a strange phrasing. It _sets_ the default for the cover-letter option, either to true or false or auto, so there is no "in addition" about it.

Perhaps

format.coverLetter::
	Setting this variable to `true` (or `false`) tells the
	`format-patch` command to pretend the `--cover-letter` (or
	`--no-cover-letter`) was given from the command line.  When
	set to `auto`, `format-patch` assumes `--cover-letter` if
	and only if it is working on more than one commit.

or something?

It also is somewhat strange why the command line argument does not accept `--cover-letter=auto` option. In general, a configuration variable should not be more flexible mechanism than what the command line could give the user. As I already said, I think that would be a better longer-term default, and that makes it even more stand-out.

Show 18 quoted lines

> @@ -1360,6 +1366,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  		numbered = 1;
>  	if (numbered)
>  		rev.total = total + start_number - 1;
> +	if (cover_letter == -1)
> +		cover_letter = (total > 1);
>  	if (in_reply_to || thread || cover_letter)
>  		rev.ref_message_ids = xcalloc(1, sizeof(struct string_list));
>  	if (in_reply_to) {
> @@ -1371,6 +1379,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  	if (cover_letter) {
>  		if (thread)
>  			gen_message_id(&rev, "cover");
> +		if (!branch_name)
> +			branch_name = find_branch_name(&rev);
>  		make_cover_letter(&rev, use_stdout,
>  				  origin, nr, list, head, branch_name, quiet);
>  		total++;

This part is nicely done, thanks to the previous two steps. You might even want to remove the call to the find_branch_name() helper function from here, and instead call it from make_cover_letter() when branch_name is not yet there.

We would need tests to make sure future changes will not break this.