[PATCH v2 0/4] more automation for cover letter generation

From: Frank Terbeck <ft@bewatermyfriend.org>
Date: May 4, 2009, 09:58 UTC
Message-ID: <1241431142-8444-1-git-send-email-ft@bewatermyfriend.org>
In-Reply-To: <7v8wlxx18c.fsf@gitster.siamese.dyndns.org>

Sorry for taking this long, I was covered in work.

Junio C Hamano <gitster@pobox.com> wrote:

Show 21 quoted lines

> Frank Terbeck <ft@bewatermyfriend.org> writes:
> > [format]
> >     coverletter = true
> >     coveronepatch = false
> 
> Nobody wants a cover letter to a single patch, so a better way
> would probably be:
> 
> 	'yes' means default behaviour, that is add cover letter for
> 	multiple-patch series, non for a single patch;
> 
> 	'no' means no cover; and
> 
> 	'always' means a probably insane "cover even a single patch".
> 
> In any case, because this new feature is way too late to be in the
> upcoming 1.6.3 release anyway, I think it is a saner approach to add a
> command line option "--cover=yes" to "cover if multiple", "--cover=always"
> to "cover even a single patch", and "--cover=no" to countermand a
> configured "format.cover" the user may have in the configuration from the
> command line.

You're right. And so is Jeff. I'll send a followup, that will address comments of the both of you. See below.

Show 15 quoted lines

> 
> >     overwritecoverletter = false
> 
> I do not think it is particularly a good idea, and it is a good idea to
> have it in the configuration.
> 
>  - Why not protect the earlier patch output?  People often tweak messages
>    (both above and below the three-dash lines) in them.
> 
>  - Isn't this pretty much per invocation?
> 
> I can understand (I may not be enthused about it) a new --clobber={yes,no}
> command line option to allow/forbid clobbering the existing files, and you
> may want to add --clobber=patches to allow clobbering only the patches but
> not cover (which I do not think makes much sense, though).

This was a followup idea, which I thought was a reasonable. In the meantime, I feel this is not needed. I'd agree that a more generic --clobber option would make sense but frankly, protecting overwrites at this stage is not something worth putting much effort into, IMHO.

Jeff King <peff@peff.net> wrote:

> On Sat, Apr 18, 2009 at 06:16:15PM +0200, Frank Terbeck wrote:

[...]

Show 7 quoted lines

> Something about "coveronepatch" seems a bit hack-ish to me. Perhaps it
> should be "generate cover letter if there are more than N patches". You
> could even just overload "format.coverletter" as:
> 
>   true - always generate cover letter
>   false - never generate cover letter
>   <number> - generate if there are at least <number> patches

Yeah, a lot better. As Junio said, noone wants cover letters for single patches. Therefore, I made format.coverletter default to 2. Setting it to one makes --cover-letter produce for cover letter for single patches, too. Setting it to 0 disables cover letters, even with --cover-letter. Any other value sets the minimum size of patch series that will trigger --cover-letter to create a cover letter.

That can be altered from the command-line via:
    --cover-letter=always
    --cover-letter=never
    --cover-letter=<length>

In addition to that I did add format.coverauto. Setting it to true (it defaults to false) makes format-patch behave like the user gave the --cover-letter option - with the following exception: if --stdout is specified, cover letters are disabled. To force cover letters in that case too, --cover-letter needs to be specified *after* --stdout.

Now that I think of it again, two weeks after writing this mail originally, I guess it would be possible to drop format.coverauto altogether and tell users to use:

  % git config --global alias.fp format-patch --cover-letter

I don't know which solution would be preferred. If it's the user-should- use-an-alias approach, I'll create a series that gets rid of format.coverauto changes.

[...]

Show 8 quoted lines

> > The series is based on master and doesn't seem to break anything
> > within the test suite. It could maybe use own tests, but I must admit
> > that I didn't look too closely at how git's test suite works and where
> > to put in tests for this.
> 
> The tests below were not actually run (and you can see they are based on
> what I proposed above, not your existing patches), but they should give
> hopefully give you a headstart.

Yes, it did. Thank you.

Frank Terbeck (4):
The new options and changes:
  Add format.coverletter option
  Add format.coverauto boolean

Tests, thanks to Jeff:
  Add tests for coverauto, coverletter and --cover-letter

And finally documentation.
  Documentation for format.coverletter and --cover-letter

 Documentation/config.txt           |    9 +++++
 Documentation/git-format-patch.txt |   17 ++++++++--
 builtin-log.c                      |   32 +++++++++++++++++--
 t/t3400-rebase.sh                  |    1 +
 t/t4014-format-patch.sh            |   58 ++++++++++++++++++++++++++++++++++-
 5 files changed, 108 insertions(+), 9 deletions(-)