[PATCH 0/6] more automation for cover letter generation

[PATCH 0/6] more automation for cover letter generation

[Frank Terbeck]

I like cover letters, in fact I like them enough to always want
--cover-letter to format-patch. The problem with that are patch "series"
that are only one patch long, where a cover letter would feel silly.

For now, I solved that by using a shell function that wrapped around
format-patch and did the trick for me.

With this series, format-patch can do it and do it better than my
wrapper could.

The following setup would suit me pretty well:

[format]
    coverletter = true
    coveronepatch = false
    overwritecoverletter = false


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.

Finally, this series does not change anything about format-patch's
default behaviour.

Regards, Frank


Frank Terbeck (6):
  format-patch: add cover{letter,onepatch} options

    The above makes it possible to *always* create cover letters
    without requesting it via --cover-letter. You can also suppress
    cover letters for patch series, that are just one patch long,
    where you'd probably put anything you have to say in addition
    between '---' and the diffstat.

  Add documentation for format-patch's --cover-one-patch
  Document format.coverletter and format.coveronepatch

  format-patch: introduce overwritecoverletter option

    When you're always generating cover letters, it can be a good idea
    to not overwrite an existing cover letter, especially if you're
    just refreshing your patch series, or added more commits to it -
    were comments you may have made in an existing cover letter might
    be valuable for the new patch series as well. Therefore setting
    overwritecoverletter to false protects you from overwriting an
    existing cover letter.

    I had hoped that this change would be possible without changes in so
    many places, but I didn't see a straight forward one.

  Add documentation for --cover-overwrite
  Document format.overwritecoverletter

 Documentation/config.txt           |   15 +++++++++++++
 Documentation/git-format-patch.txt |   11 +++++++++
 builtin-log.c                      |   40 +++++++++++++++++++++++++++++++----
 log-tree.c                         |    9 +++++--
 log-tree.h                         |    4 +-
 5 files changed, 69 insertions(+), 10 deletions(-)

[PATCH 1/6] format-patch: add cover{letter,onepatch} options

[Frank Terbeck]

When using --cover-letter, a cover letter is created even if the patch
series is only one patch long. By setting format.coveronepatch to 'false', this
is prevented. To temporarily force creating cover letter even for
one-patch "series", the --cover-one-patch option may be used.

To always create cover letters, the format.coverletter option may be set
to 'true'.

A possible setup to create cover letters for every created patch series,
that is longer than one patch, would be:

[format]
    coverletter = true
    coveronepatch = false

git-format-patch's default behaviour is not altered by this patch.

Signed-off-by: Frank Terbeck <ft@bewatermyfriend.org>
---
 builtin-log.c |   16 +++++++++++++++-
 1 files changed, 15 insertions(+), 1 deletions(-)

diff --git a/builtin-log.c b/builtin-log.c
index 5eaec5d..82d8724 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -436,6 +436,9 @@ static char **extra_cc;
 static int extra_cc_nr;
 static int extra_cc_alloc;
 
+static int cover_letter = 0;
+static int cover_one_patch = 1;
+
 static void add_header(const char *value)
 {
 	int len = strlen(value);
@@ -512,6 +515,14 @@ static int git_format_config(const char *var, const char *value, void *cb)
 		do_signoff = git_config_bool(var, value);
 		return 0;
 	}
+	if (!strcmp(var, "format.coverletter")) {
+		cover_letter = git_config_bool(var, value);
+		return 0;
+	}
+	if (!strcmp(var, "format.coveronepatch")) {
+		cover_one_patch = git_config_bool(var, value);
+		return 0;
+	}
 
 	return git_log_config(var, value, cb);
 }
@@ -752,7 +763,6 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	int numbered_files = 0;		/* _just_ numbers */
 	int subject_prefix = 0;
 	int ignore_if_in_upstream = 0;
-	int cover_letter = 0;
 	int boundary_count = 0;
 	int no_binary_diff = 0;
 	struct commit *origin = NULL, *head = NULL;
@@ -868,6 +878,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 			fmt_patch_suffix = argv[i] + 9;
 		else if (!strcmp(argv[i], "--cover-letter"))
 			cover_letter = 1;
+		else if (!strcmp(argv[i], "--cover-one-patch"))
+			cover_one_patch = 1;
 		else if (!strcmp(argv[i], "--no-binary"))
 			no_binary_diff = 1;
 		else if (!prefixcmp(argv[i], "--add-header="))
@@ -1010,6 +1022,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		list[nr - 1] = commit;
 	}
 	total = nr;
+	if (!cover_one_patch && total == 1)
+		cover_letter = 0;
 	if (!keep_subject && auto_number && total > 1)
 		numbered = 1;
 	if (numbered)
-- 
1.6.2.2.446.gfbdc0

[PATCH 2/6] Add documentation for format-patch's --cover-one-patch

[Frank Terbeck]

Signed-off-by: Frank Terbeck <ft@bewatermyfriend.org>
---
 Documentation/git-format-patch.txt |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index eb2fbcf..65e4089 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -20,6 +20,7 @@ SYNOPSIS
 		   [--subject-prefix=Subject-Prefix]
 		   [--cc=<email>]
 		   [--cover-letter]
+		   [--cover-one-patch]
 		   [ <since> | <revision range> ]
 
 DESCRIPTION
@@ -167,6 +168,11 @@ if that is not set.
 	containing the shortlog and the overall diffstat.  You can
 	fill in a description in the file before sending it out.
 
+--cover-one-patch::
+	If a patch series is only one patch long, the format.coveronepatch
+	will prevent the creation of cover letters, if disabled.
+	This option can force creating a cover letter in those cases.
+
 --suffix=.<sfx>::
 	Instead of using `.patch` as the suffix for generated
 	filenames, use specified suffix.  A common alternative is
-- 
1.6.2.2.446.gfbdc0

[PATCH 3/6] Document format.coverletter and format.coveronepatch

[Frank Terbeck]

Signed-off-by: Frank Terbeck <ft@bewatermyfriend.org>
---
 Documentation/config.txt |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 5319df5..00b8f1f 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -705,6 +705,16 @@ fetch.unpackLimit::
 	especially on slow filesystems.  If not set, the value of
 	`transfer.unpackLimit` is used instead.
 
+format.coverletter::
+	A boolean variable, if enabled causes the creation of cover letters
+	for patch series without manually using the --cover-letter option
+	of linkgit:git-format-patch[1]. This variable defaults to "false".
+
+format.coveronepatch::
+	If a patch series consists of only one patch, this boolean variable
+	decides whether to create a cover letter or not (if that is
+	requested). The default is "true".
+
 format.numbered::
 	A boolean which can enable or disable sequence numbers in patch
 	subjects.  It defaults to "auto" which enables it only if there
-- 
1.6.2.2.446.gfbdc0

[PATCH 4/6] format-patch: introduce overwritecoverletter option

[Frank Terbeck]

This option, when set, causes a fatal error if format-patch would
overwrite an existing coverletter to avoid information loss if the
letter was already edited before.

This changes the return value of get_patch_filename() from void to int
and its prototype now contains a new integer parameter 'check'. If
'check' is false, get_patch_filename() behaves as it did before and it
will always return zero. If 'check' is true, get_patch_filename() will
use access(3) to check whether the filename it created exists, if so
get_patch_filename() returns 1; zero otherwise.

This access(3) test is only done if the overwritecoverletter option is
unset and reopen_stdout()'s new 'check' parameter is true. That is done
only by make_cover_letter().

The default for overwritecoverletter is true, which means, that the
default behaviour does not change. If overwritecoverletter was unset,
there is the new --cover-overwrite option to temporarily force
overwriting cover letters.

Signed-off-by: Frank Terbeck <ft@bewatermyfriend.org>
---
 builtin-log.c |   24 ++++++++++++++++++++----
 log-tree.c    |    9 ++++++---
 log-tree.h    |    4 ++--
 3 files changed, 28 insertions(+), 9 deletions(-)

diff --git a/builtin-log.c b/builtin-log.c
index 82d8724..816b4c9 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -438,6 +438,7 @@ static int extra_cc_alloc;
 
 static int cover_letter = 0;
 static int cover_one_patch = 1;
+static int cover_overwrite = 1;
 
 static void add_header(const char *value)
 {
@@ -523,6 +524,10 @@ static int git_format_config(const char *var, const char *value, void *cb)
 		cover_one_patch = git_config_bool(var, value);
 		return 0;
 	}
+	if (!strcmp(var, "format.overwritecoverletter")) {
+		cover_overwrite = git_config_bool(var, value);
+		return 0;
+	}
 
 	return git_log_config(var, value, cb);
 }
@@ -531,7 +536,7 @@ static FILE *realstdout = NULL;
 static const char *output_directory = NULL;
 static int outdir_offset;
 
-static int reopen_stdout(struct commit *commit, struct rev_info *rev)
+static int reopen_stdout(struct commit *commit, struct rev_info *rev, int check)
 {
 	struct strbuf filename = STRBUF_INIT;
 	int suffix_len = strlen(fmt_patch_suffix) + 1;
@@ -545,7 +550,16 @@ static int reopen_stdout(struct commit *commit, struct rev_info *rev)
 			strbuf_addch(&filename, '/');
 	}
 
-	get_patch_filename(commit, rev->nr, fmt_patch_suffix, &filename);
+	if (!cover_overwrite && check) {
+		int exists;
+		exists = get_patch_filename(commit, rev->nr, fmt_patch_suffix,
+					    check, &filename);
+		if (exists)
+			die("Not overwriting existing coverletter: %s",
+			    filename.buf);
+	} else
+		get_patch_filename(commit, rev->nr, fmt_patch_suffix,
+				   0, &filename);
 
 	if (!DIFF_OPT_TST(&rev->diffopt, QUIET))
 		fprintf(realstdout, "%s\n", filename.buf + outdir_offset);
@@ -656,7 +670,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 			sha1_to_hex(head->object.sha1), committer, committer);
 	}
 
-	if (!use_stdout && reopen_stdout(commit, rev))
+	if (!use_stdout && reopen_stdout(commit, rev, 1))
 		return;
 
 	if (commit) {
@@ -880,6 +894,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 			cover_letter = 1;
 		else if (!strcmp(argv[i], "--cover-one-patch"))
 			cover_one_patch = 1;
+		else if (!strcmp(argv[i], "--cover-overwrite"))
+			cover_overwrite = 1;
 		else if (!strcmp(argv[i], "--no-binary"))
 			no_binary_diff = 1;
 		else if (!prefixcmp(argv[i], "--add-header="))
@@ -1086,7 +1102,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		}
 
 		if (!use_stdout && reopen_stdout(numbered_files ? NULL : commit,
-						 &rev))
+						 &rev, 0))
 			die("Failed to create output files");
 		shown = log_tree_commit(&rev, commit);
 		free(commit->buffer);
diff --git a/log-tree.c b/log-tree.c
index 5bd29e6..7163abb 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -179,8 +179,8 @@ static int has_non_ascii(const char *s)
 	return 0;
 }
 
-void get_patch_filename(struct commit *commit, int nr, const char *suffix,
-			struct strbuf *buf)
+int get_patch_filename(struct commit *commit, int nr, const char *suffix,
+			int check, struct strbuf *buf)
 {
 	int suffix_len = strlen(suffix) + 1;
 	int start_len = buf->len;
@@ -194,6 +194,9 @@ void get_patch_filename(struct commit *commit, int nr, const char *suffix,
 			strbuf_setlen(buf, max_len);
 		strbuf_addstr(buf, suffix);
 	}
+	if (check && (access(buf->buf, F_OK) == 0))
+		return 1;
+	return 0;
 }
 
 void log_write_email_headers(struct rev_info *opt, struct commit *commit,
@@ -262,7 +265,7 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,
 		extra_headers = subject_buffer;
 
 		get_patch_filename(opt->numbered_files ? NULL : commit, opt->nr,
-				    opt->patch_suffix, &filename);
+				    opt->patch_suffix, 0, &filename);
 		snprintf(buffer, sizeof(buffer) - 1,
 			 "\n--%s%s\n"
 			 "Content-Type: text/x-patch;"
diff --git a/log-tree.h b/log-tree.h
index 20b5caf..8e91b7a 100644
--- a/log-tree.h
+++ b/log-tree.h
@@ -20,7 +20,7 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,
 void load_ref_decorations(void);
 
 #define FORMAT_PATCH_NAME_MAX 64
-void get_patch_filename(struct commit *commit, int nr, const char *suffix,
-			struct strbuf *buf);
+int get_patch_filename(struct commit *commit, int nr, const char *suffix,
+		       int check, struct strbuf *buf);
 
 #endif
-- 
1.6.2.2.446.gfbdc0

[PATCH 5/6] Add documentation for --cover-overwrite

[Frank Terbeck]

Signed-off-by: Frank Terbeck <ft@bewatermyfriend.org>
---
 Documentation/git-format-patch.txt |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index 65e4089..07a2ee3 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -21,6 +21,7 @@ SYNOPSIS
 		   [--cc=<email>]
 		   [--cover-letter]
 		   [--cover-one-patch]
+		   [--cover-overwrite]
 		   [ <since> | <revision range> ]
 
 DESCRIPTION
@@ -173,6 +174,10 @@ if that is not set.
 	will prevent the creation of cover letters, if disabled.
 	This option can force creating a cover letter in those cases.
 
+--cover-overwrite::
+	If format.overwritecoverletter is unset, this option will
+	temporarily force overwriting cover letters.
+
 --suffix=.<sfx>::
 	Instead of using `.patch` as the suffix for generated
 	filenames, use specified suffix.  A common alternative is
-- 
1.6.2.2.446.gfbdc0

[PATCH 6/6] Document format.overwritecoverletter

[Frank Terbeck]

Signed-off-by: Frank Terbeck <ft@bewatermyfriend.org>
---
 Documentation/config.txt |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 00b8f1f..616bfb5 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -715,6 +715,11 @@ format.coveronepatch::
 	decides whether to create a cover letter or not (if that is
 	requested). The default is "true".
 
+format.overwritecoverletter::
+	If this boolean variable is set to "false", it will cause
+	linkgit:git-format-patch[1] fail fatally if it would overwrite an
+	existing coverletter. The default is "true".
+
 format.numbered::
 	A boolean which can enable or disable sequence numbers in patch
 	subjects.  It defaults to "auto" which enables it only if there
-- 
1.6.2.2.446.gfbdc0

Re: [PATCH 0/6] more automation for cover letter generation

[Junio C Hamano]

Frank Terbeck <ft@bewatermyfriend.org> writes:

> The following setup would suit me pretty well:
>
> [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.

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

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

[Frank Terbeck]

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

Junio C Hamano <gitster@pobox.com> wrote:
> 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.

> 
> >     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:
[...]
> 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.

[...]
> > 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(-)

[PATCH v2 1/4] Add format.coverletter option

[Frank Terbeck]

When using --cover-letter, a cover letter is created even if the patch
series is only one patch long.

This patch cures that by making the behaviour configurable.
'format.coverletter' is an integer option, that tells git how long a
patch series needs to be for it to generate a cover letter.
If set to 0, git never creates a cover letter.

The default minimum patch series length is 2.

The --cover-letter option now accepts integer arguments (such as
--cover-letter=5), which have the same effect as setting the
'format.coverletter' option. 'never' instead of an integer has the
same effect as 0 has; it surpresses cover letter generation.
'always' has the same effect as setting the minimum length to 1.

Signed-off-by: Frank Terbeck <ft@bewatermyfriend.org>
---
 builtin-log.c |   23 +++++++++++++++++++++--
 1 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/builtin-log.c b/builtin-log.c
index 5eaec5d..157c8cf 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -436,6 +436,9 @@ static char **extra_cc;
 static int extra_cc_nr;
 static int extra_cc_alloc;
 
+static int cover_letter = 0;
+static int cover_letter_len = 2;
+
 static void add_header(const char *value)
 {
 	int len = strlen(value);
@@ -480,6 +483,10 @@ static int git_format_config(const char *var, const char *value, void *cb)
 	if (!strcmp(var, "diff.color") || !strcmp(var, "color.diff")) {
 		return 0;
 	}
+	if (!strcmp(var, "format.coverletter")) {
+		cover_letter_len = git_config_int(var, value);
+		return 0;
+	}
 	if (!strcmp(var, "format.numbered")) {
 		if (value && !strcasecmp(value, "auto")) {
 			auto_number = 1;
@@ -752,7 +759,6 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	int numbered_files = 0;		/* _just_ numbers */
 	int subject_prefix = 0;
 	int ignore_if_in_upstream = 0;
-	int cover_letter = 0;
 	int boundary_count = 0;
 	int no_binary_diff = 0;
 	struct commit *origin = NULL, *head = NULL;
@@ -868,7 +874,15 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 			fmt_patch_suffix = argv[i] + 9;
 		else if (!strcmp(argv[i], "--cover-letter"))
 			cover_letter = 1;
-		else if (!strcmp(argv[i], "--no-binary"))
+		else if (!prefixcmp(argv[i], "--cover-letter=always")) {
+			cover_letter = 1;
+			cover_letter_len = 1;
+		} else if (!prefixcmp(argv[i], "--cover-letter=never"))
+			cover_letter = 0;
+		else if (!prefixcmp(argv[i], "--cover-letter=")) {
+			cover_letter = 1;
+			cover_letter_len = strtol(argv[i] + 15, NULL, 10);
+		} else if (!strcmp(argv[i], "--no-binary"))
 			no_binary_diff = 1;
 		else if (!prefixcmp(argv[i], "--add-header="))
 			add_header(argv[i] + 13);
@@ -877,6 +891,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	}
 	argc = j;
 
+	if (cover_letter_len <= 0)
+		cover_letter = 0;
+
 	if (do_signoff) {
 		const char *committer;
 		const char *endpos;
@@ -1010,6 +1027,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		list[nr - 1] = commit;
 	}
 	total = nr;
+	if (total < cover_letter_len)
+		cover_letter = 0;
 	if (!keep_subject && auto_number && total > 1)
 		numbered = 1;
 	if (numbered)
-- 
1.6.2.2.446.gfbdc0

[PATCH v2 2/4] Add format.coverauto boolean

[Frank Terbeck]

If set to true, format-patch behaves like it had been started
using the --cover-letter option.

An exception is if it is called using the --stdout option,
which disables format.coverauto, because users of --stdout
(like git-rebase.sh) usually are not interested in
cover letters at all.

If you do want it anyway, you can use:
  % git format-patch --stdout --cover-letter

Signed-off-by: Frank Terbeck <ft@bewatermyfriend.org>
---
 builtin-log.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/builtin-log.c b/builtin-log.c
index 157c8cf..52c0d47 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -483,6 +483,10 @@ static int git_format_config(const char *var, const char *value, void *cb)
 	if (!strcmp(var, "diff.color") || !strcmp(var, "color.diff")) {
 		return 0;
 	}
+	if (!strcmp(var, "format.coverauto")) {
+		cover_letter = git_config_bool(var, value);;
+		return 0;
+	}
 	if (!strcmp(var, "format.coverletter")) {
 		cover_letter_len = git_config_int(var, value);
 		return 0;
@@ -789,9 +793,10 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	 * possibly a valid SHA1.
 	 */
 	for (i = 1, j = 1; i < argc; i++) {
-		if (!strcmp(argv[i], "--stdout"))
+		if (!strcmp(argv[i], "--stdout")) {
 			use_stdout = 1;
-		else if (!strcmp(argv[i], "-n") ||
+			cover_letter = 0;
+		} else if (!strcmp(argv[i], "-n") ||
 				!strcmp(argv[i], "--numbered"))
 			numbered = 1;
 		else if (!strcmp(argv[i], "-N") ||
-- 
1.6.2.2.446.gfbdc0

Re: [PATCH v2 2/4] Add format.coverauto boolean

[Stephen Boyd]

On Mon, May 4, 2009 at 2:59 AM, Frank Terbeck <ft@bewatermyfriend.org> wrote:
> An exception is if it is called using the --stdout option,
> which disables format.coverauto, because users of --stdout
> (like git-rebase.sh) usually are not interested in
> cover letters at all.
>

Would it make more sense to just have git-rebase.sh use
--cover-letter=never? I thought configuration variables were defaults
which have to be overridden.

Also, why does this variable even exist? I think Jeff's suggestion is
best, where you can set format.coverletter to always, never, or some
number.

> +       if (!strcmp(var, "format.coverauto")) {
> +               cover_letter = git_config_bool(var, value);;
> +               return 0;
> +       }

Double semi-colon?

Finally, this option is very useful, so keep up the good work.

Re: [PATCH v2 2/4] Add format.coverauto boolean

[Frank Terbeck]

Stephen Boyd <bebarino@gmail.com>:
> On Mon, May 4, 2009 at 2:59 AM, Frank Terbeck <ft@bewatermyfriend.org> wrote:
> > An exception is if it is called using the --stdout option,
> > which disables format.coverauto, because users of --stdout
> > (like git-rebase.sh) usually are not interested in
> > cover letters at all.
> >
> 
> Would it make more sense to just have git-rebase.sh use
> --cover-letter=never? I thought configuration variables were defaults
> which have to be overridden.

Could be done. And in an earlier version I did that. Since that took
changes in more places, I reverted to this less intrusive approach.

Also, this will keep the scripts of people who use --stdout working,
no matter what the settings of an individual user might be.

I don't think people who use --stdout will want the cover letter
(which is always the same) in the output. Since you can still force
its output, I think this would be reasonable compromise.

> Also, why does this variable even exist? I think Jeff's suggestion is
> best, where you can set format.coverletter to always, never, or some
> number.

Well, I did this because I wanted both Junio's and Jeff's suggestions
to be incorporated.

Junio correctly stated, that nobody will usually want cover letters
for one-patch "series". Which I think is right; and the wrapper I used
for this before did take that into account as well. So coverletter had
to default to 2.

If that where to enable the generation of cover letters for all
format-patch calls with every patch series that is at least two
patches long, it would change format-patch's default behaviour;
potentially breaking people's scripts (similarly to the way
coverauto=true could break git-rebase.sh without either adding
--cover-letter=never or the exception in the --stdout codepath).

That's why I think this should be handled in two separate options.
And as I mentioned, coverauto could be left out if we'd advise users
who want that to create an git alias that does 'format-patch
--cover-letter'.

> > +	if (!strcmp(var, "format.coverauto")) {
> > +		cover_letter = git_config_bool(var, value);;
> > +		return 0;
> > +	}
> 
> Double semi-colon?

Oops, yeah. I'll resend this one (if coverauto turns out to be the way
to go).

Regards, Frank

-- 
In protocol design, perfection has been reached not when there is
nothing left to add, but when there is nothing left to take away.
                                                  -- RFC 1925

Re: [PATCH v2 2/4] Add format.coverauto boolean

[Junio C Hamano]

Frank Terbeck <ft@bewatermyfriend.org> writes:

> If set to true, format-patch behaves like it had been started
> using the --cover-letter option.

I thought "If this is set, you can run format-patch without giving an
explicit --cover-letter=foo from the command line" was already done with
the earlier format.coverletter configuration variable.  Why do you need a
separate variable?  It does not make any sense to me, unless I am missing
something.

Re: [PATCH v2 2/4] Add format.coverauto boolean

[Frank Terbeck]

Junio C Hamano <gitster@pobox.com>:
> Frank Terbeck <ft@bewatermyfriend.org> writes:
> 
> > If set to true, format-patch behaves like it had been started
> > using the --cover-letter option.
> 
> I thought "If this is set, you can run format-patch without giving an
> explicit --cover-letter=foo from the command line" was already done with
> the earlier format.coverletter configuration variable.  Why do you need a
> separate variable?  It does not make any sense to me, unless I am missing
> something.

Well, the two can certainly by merged. That could potentially break
people's existing scripts - either by new default behaviour or by the
setting of format.coverletter of an individual user. That could still
happen when using coverauto, so maybe my reasoning was flawed - given
that Stephen raised the same question.

So, I should create one option 'coverletter'. If it's set to zero,
never create cover letters; if one, always create cover letters; if
any other positive integer, create cover letters automatically if a
patch series is at least that long. And do that without requiring the
user to supply --cover-letter; only provide that option to explicitly
overwrite the configured behaviour. Right?

If so, do you want coverletter to default to zero (which wouldn't
change the default behaviour) or do you want it to default to two?

Regards, Frank

-- 
In protocol design, perfection has been reached not when there is
nothing left to add, but when there is nothing left to take away.
                                                  -- RFC 1925

Re: [PATCH v2 2/4] Add format.coverauto boolean

[Junio C Hamano]

Frank Terbeck <ft@bewatermyfriend.org> writes:

> ... That could potentially break
> people's existing scripts - either by new default behaviour or by the
> setting of format.coverletter of an individual user. That could still
> happen when using coverauto, so maybe my reasoning was flawed - given
> that Stephen raised the same question.

That part I agree with 100%.  A separate configuration variable does not
help here; it will not act as an "I know what I am doing" flag.

"rebase" is not a problem because both format-patch and rebase come from
the same git and people do not mix and match.  The only thing we need to
do is to make sure that rebase is updated to explicitly decline the cover
letter in the same or earlier version of git that adds this new option.

However, there are other people's scripts to worry about, and no matter
what you do, you cannot avoid breaking them if you introduce a
configuration variable that changes the behaviour of the command in a
backward incompatible way.  This could be a disruptive change that needs
to happen at a major revision boundary, if we were to add it.

> If so, do you want coverletter to default to zero (which wouldn't
> change the default behaviour) or do you want it to default to two?

The default between 0 and 2 does not matter much.  If people find this new
feature useful (if not, then this new feature is not worth adding), then
they will set it to non-zero themselves, and then get their scripts
broken.  In a sense, defaulting it to zero is just delaying an inevitable
breakage.

If we were to do this, we should rather default it to two from day one and
make sure we break people's scripts that (rightly) rely on format-patch
not producing covers that the caller never asked, to make sure they are
adjusted to decline cover explicitly.  And such a release should come with
a note in huge red letters that says "this new feature allows you not to
say --cover on the command line, and it is far more important than keeping
your scripts that run format-patch and process its output working.  You
must adjust your scripts to the new reality.  Sorry for the inconvenience,
and have a nice day."

I do not think I would stand behind such a release note, though.

For one thing, I do not see a huge need for this configuration variable.
Why is "--cover" too cumbersome to type, when you are already willing to
type "format-patch"?  You can alias the whole thing away to make it even
shorter.  For another, we do not simply break people's scripts knowingly.

For this feature to go forward, somebody has to find a way not to break
people's scripts even when the user uses it.  One possibility is to find a
nicer verb X and introduce "git X" command that does what "format-patch"
does but with a better default (and syntax --- people get confused by the
oldest form "git format-patch $commit", which does not behave like "git
log $commit" simply because the syntax predates the modern "revision
range" notation the "log" family supports, such as A..B).

Re: [PATCH v2 2/4] Add format.coverauto boolean

[Frank Terbeck]

Junio C Hamano <gitster@pobox.com>:
[...]
> For one thing, I do not see a huge need for this configuration variable.
> Why is "--cover" too cumbersome to type, when you are already willing to
> type "format-patch"?  You can alias the whole thing away to make it even
> shorter.  For another, we do not simply break people's scripts knowingly.
> 
> For this feature to go forward, somebody has to find a way not to break
> people's scripts even when the user uses it.  One possibility is to find a
> nicer verb X and introduce "git X" command that does what "format-patch"
> does but with a better default (and syntax --- people get confused by the
> oldest form "git format-patch $commit", which does not behave like "git
> log $commit" simply because the syntax predates the modern "revision
> range" notation the "log" family supports, such as A..B).

Yes, this whole compatibility issue is exactly the reason why I said
earlier, that we could only take format.coverletter (the way it's
currently implemented in the latest series) and forget about
format.coverauto (and its implications) altogether:

<1241431142-8444-1-git-send-email-ft@bewatermyfriend.org>:
} 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.

I obviously was missing quotes around the alias's value but then,
'git fp' would do exactly what I'm after with this series. You could
just set format.coverletter to 2 and use 'git fp'. You always get a
cover letter for patch series longer than one patch.

The other approach, to create yet another subcommand just for this
would be too much IMHO. There are enough of them already.

Regards, Frank

-- 
In protocol design, perfection has been reached not when there is
nothing left to add, but when there is nothing left to take away.
                                                  -- RFC 1925

Re: [PATCH v2 2/4] Add format.coverauto boolean

[Frank Terbeck]

Frank Terbeck <ft@bewatermyfriend.org>:
[...]
> }  % git config --global alias.fp format-patch --cover-letter

That may still break backwards compatibility.
To be safe, we could make --cover-letter behave as it always did, and
only make it look at format.coverletter if --cover-letter=auto was
specified.

Regards, Frank

[PATCH v2 3/4] Add tests for coverauto, coverletter and --cover-letter

[Frank Terbeck]

Very much based on Jeff King's suggestion from
<20090421033213.GA14881@coredump.intra.peff.net>

This change also makes sure that one major user of --stdout
(git-rebase.sh) works with format.coverauto set to true.

Signed-off-by: Frank Terbeck <ft@bewatermyfriend.org>
---
 t/t3400-rebase.sh       |    1 +
 t/t4014-format-patch.sh |   58 +++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 57 insertions(+), 2 deletions(-)

diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index 6e391a3..48d8360 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -15,6 +15,7 @@ export GIT_AUTHOR_EMAIL
 test_expect_success \
     'prepare repository with topic branches' \
     'git config core.logAllRefUpdates true &&
+     git config format.coverauto true &&
      echo First > A &&
      git update-index --add A &&
      git commit -m "Add A." &&
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 11061dd..3f431ff 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -151,6 +151,60 @@ test_expect_success 'multiple files' '
 	ls patches/0001-Side-changes-1.patch patches/0002-Side-changes-2.patch patches/0003-Side-changes-3-with-n-backslash-n-in-it.patch
 '
 
+test_expect_success '--cover-letter generates cover letter' '
+
+	rm -rf patches &&
+	git config format.coverletter 1
+	git format-patch --cover-letter -o patches/ master &&
+	test -f patches/0000-cover-letter.patch
+'
+
+test_expect_success 'format.coverletter=number > 1 generates cover letter' '
+
+	rm -rf patches &&
+	git config format.coverletter 3 &&
+	git format-patch --cover-letter -o patches/ master &&
+	test -f patches/0000-cover-letter.patch
+'
+
+test_expect_success 'format.coverletter respects minimum patchset size' '
+
+	rm -rf patches &&
+	git config format.coverletter 4 &&
+	git format-patch --cover-letter -o patches/ master &&
+	! test -f patches/0000-cover-letter.patch
+'
+
+test_expect_success '--cover-letter=never disables cover letters' '
+
+	rm -rf patches &&
+	git config format.coverletter 2 &&
+	git format-patch --cover-letter=never -o patches/ master &&
+	! test -f patches/0000-cover-letter.patch
+'
+
+test_expect_success '--cover-letter=always forces cover letters' '
+
+	rm -rf patches &&
+	git format-patch --cover-letter=always -o patches/ master &&
+	test -f patches/0000-cover-letter.patch
+'
+
+test_expect_success 'format.coverauto works (1)' '
+
+	rm -rf patches &&
+	git config format.coverauto true &&
+	git format-patch -1 -o patches/ master &&
+	! test -f patches/0000-cover-letter.patch
+'
+
+test_expect_success 'format.coverauto works (2)' '
+
+	rm -rf patches &&
+	git format-patch -2 -o patches/ master &&
+	test -f patches/0000-cover-letter.patch
+'
+
 check_threading () {
 	expect="$1" &&
 	shift &&
@@ -405,9 +459,9 @@ test_expect_success 'cover-letter inherits diff options' '
 
 	git mv file foo &&
 	git commit -m foo &&
-	git format-patch --cover-letter -1 &&
+	git format-patch --cover-letter=always -1 &&
 	! grep "file => foo .* 0 *$" 0000-cover-letter.patch &&
-	git format-patch --cover-letter -1 -M &&
+	git format-patch --cover-letter=always -1 -M &&
 	grep "file => foo .* 0 *$" 0000-cover-letter.patch
 
 '
-- 
1.6.2.2.446.gfbdc0

[PATCH v2 4/4] Documentation for format.coverletter and --cover-letter

[Frank Terbeck]

Signed-off-by: Frank Terbeck <ft@bewatermyfriend.org>
---
 Documentation/config.txt           |    9 +++++++++
 Documentation/git-format-patch.txt |   17 ++++++++++++++---
 2 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 5dcad94..2842cb1 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -723,6 +723,15 @@ format.attach::
 	value as the boundary.  See the --attach option in
 	linkgit:git-format-patch[1].
 
+format.coverauto::
+	A boolean variable, if set linkgit:git-format-patch[1] will behave
+	as if it was called with its --cover-letter option, unless it was
+	run with its --stdout option.
+
+format.coverletter::
+	Configures how long a patch series needs to be for cover letters
+	to be created. The default is 2.
+
 format.numbered::
 	A boolean which can enable or disable sequence numbers in patch
 	subjects.  It defaults to "auto" which enables it only if there
diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index 6f1fc80..64f90a1 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -19,7 +19,7 @@ SYNOPSIS
 		   [--ignore-if-in-upstream]
 		   [--subject-prefix=Subject-Prefix]
 		   [--cc=<email>]
-		   [--cover-letter]
+		   [--cover-letter[=(always|never|<length>)]]
 		   [<common diff options>]
 		   [ <since> | <revision range> ]
 
@@ -107,7 +107,11 @@ include::diff-options.txt[]
 
 --stdout::
 	Print all commits to the standard output in mbox format,
-	instead of creating a file for each one.
+	instead of creating a file for each one. This also disables
+	the format.coverauto option if it was set. If you really want
+	cover letters to be included in --stdout output you have to
+	specify the --cover-letter option after the --stdout option
+	on the command line.
 
 --attach[=<boundary>]::
 	Create multipart/mixed attachment, the first part of
@@ -163,10 +167,17 @@ if that is not set.
 	to any configured headers, and may be used multiple times.
 	For example, --add-header="Organization: git-foo"
 
---cover-letter::
+--cover-letter[=(always|never|<length>)]::
 	In addition to the patches, generate a cover letter file
 	containing the shortlog and the overall diffstat.  You can
 	fill in a description in the file before sending it out.
+	Per default, cover letters are created if a patch series is least two
+	patches long (the format.coverletter allows you to change that default
+	length). If a parameter is given it overwrites the default minimum
+	series length. If the parameter is 0 or 'never', a cover letter will
+	not be generated even if the format.coverletter option would trigger
+	normally it. If the parameter is 1 or 'always' a cover letter will
+	be generated even if the patch series is only one patch long.
 
 --suffix=.<sfx>::
 	Instead of using `.patch` as the suffix for generated
-- 
1.6.2.2.446.gfbdc0

Re: [PATCH 0/6] more automation for cover letter generation

[Jeff King]

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

> I like cover letters, in fact I like them enough to always want
> --cover-letter to format-patch. The problem with that are patch "series"
> that are only one patch long, where a cover letter would feel silly.
> 
> For now, I solved that by using a shell function that wrapped around
> format-patch and did the trick for me.
> 
> With this series, format-patch can do it and do it better than my
> wrapper could.

That seems like a reasonable goal.

> The following setup would suit me pretty well:
> 
> [format]
>     coverletter = true
>     coveronepatch = false
>     overwritecoverletter = false

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

?

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

---
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 11061dd..9e13ee9 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -151,6 +151,27 @@ test_expect_success 'multiple files' '
 	ls patches/0001-Side-changes-1.patch patches/0002-Side-changes-2.patch patches/0003-Side-changes-3-with-n-backslash-n-in-it.patch
 '
 
+test_expect_success 'format.coverletter=true generates cover letter' '
+	rm -rf patches &&
+	git config core.coverletter true
+	git format-patch -o patches/ master &&
+	test -f patches/0000-cover-letter.patch
+'
+
+test_expect_success 'format.coverletter=number generates cover letter' '
+	rm -rf patches &&
+	git config core.coverletter 3 &&
+	git format-patch -o patches/ master &&
+	test -f patches/0000-cover-letter.patch
+'
+
+test_expect_success 'format.coverletter respects minimum patchset size' '
+	rm -rf patches &&
+	git config core.coverletter 4 &&
+	git format-patch -o patches/ master &&
+	! test -f patches/0000-cover-letter.patch
+'
+
 check_threading () {
 	expect="$1" &&
 	shift &&