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

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Apr 7, 2013, 08:48 UTC
Message-ID: <CAMP44s0rsWCZE=MyW3zuBoVM5ZtEGQQb+deQJB5-i6tBzFdTfw@mail.gmail.com>
In-Reply-To: <7v4nfi92q1.fsf@alter.siamese.dyndns.org>

On Sun, Apr 7, 2013 at 3:14 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 23 quoted lines

> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> 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.

Yeah, I forgot.

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

There's no --cover-letter=auto.

Show 8 quoted lines

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

I prefer the wording suggested by Ramkumar with a minor modification:

A boolean that controls whether to generate a cover-letter when format-patch is invoked, but in addition can be set to "auto", to generate a cover-letter only when generating more than one patch.

Show 7 quoted lines

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

Users know how to count, if the default is no cover-letter, then doing --cover-letter=auto is basically --cover-letter, unless they don't know how to count and thought --cover-letter=auto would do something. The same is true is they configured coverletter=true.

I don't see any value in doing that.

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

True. We can also move the 'head' variable inside make_cover_letter() as well.

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

Sigh. All right.

-- 
Felipe Contreras