diff mbox series

[v2,4/4] format-patch: learn --infer-cover-letter option

Message ID e682bd347a09d0e1293ec6bd495c38dac5006a19.1566258525.git.liu.denton@gmail.com (mailing list archive)
State New, archived
Headers show
Series format-patch: learn --infer-cover-subject option | expand

Commit Message

Denton Liu Aug. 19, 2019, 11:52 p.m. UTC
We used to populate the subject of the cover letter generated by
git-format-patch with "*** SUBJECT HERE ***". However, if a user submits
multiple patchsets, they may want to keep a consistent subject between
rerolls.

If git-format-patch is run with `--infer-cover-letter` or
`format.inferCoverSubject`, infer the subject for the cover letter from
the top line(s) of a branch description, similar to how a subject is
read from a commit message.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 Documentation/config/format.txt    |  4 +++
 Documentation/git-format-patch.txt | 10 ++++++
 builtin/log.c                      | 56 +++++++++++++++++++-----------
 t/t4014-format-patch.sh            | 33 ++++++++++++++++++
 4 files changed, 82 insertions(+), 21 deletions(-)

Comments

Eric Sunshine Aug. 20, 2019, 3:46 a.m. UTC | #1
On Mon, Aug 19, 2019 at 7:53 PM Denton Liu <liu.denton@gmail.com> wrote:
> We used to populate the subject of the cover letter generated by
> git-format-patch with "*** SUBJECT HERE ***". However, if a user submits
> multiple patchsets, they may want to keep a consistent subject between
> rerolls.
>
> If git-format-patch is run with `--infer-cover-letter` or

s/letter/subject/

> `format.inferCoverSubject`, infer the subject for the cover letter from
> the top line(s) of a branch description, similar to how a subject is
> read from a commit message.

A possible rewrite of the entire commit message in imperative mood:

    Teach 'format-patch' to use the first line of the branch description
    as the Subject: of the generated cover letter, rather than
    "*** SUBJECT HERE ***", if --infer-cover-subject is specified (or the
    corresponding `format.inferCoverSubject` option is enabled). This
    complements existing inclusion of the branch description in the
    cover letter body.

A casual reader of this patch might wonder why this new useful
behavior isn't default, so it might make sense for the commit message
to further explain that making it default would potentially break
existing tooling.

> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
> diff --git a/Documentation/config/format.txt b/Documentation/config/format.txt
> @@ -36,6 +36,10 @@ format.subjectPrefix::
> +format.inferCoverSubject::
> +       A boolean value which lets you enable the
> +       `--infer-cover-subject` option of format-patch by default.

As mentioned in my review of 3/4, it is common to mention the default
value at the end of the paragraph. So, perhaps:

    A boolean that controls whether or not to take the first line of
    the branch description as the subject for the cover letter. See the
    `--infer-cover-subject` option in linkgit:git-format-patch[1].
    Default is false.

> diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
> @@ -171,6 +172,14 @@ will want to ensure that threading is disabled for `git send-email`.
> +--[no-]infer-cover-subject::
> +       Instead of using the default "*** SUBJECT HERE ***" subject for
> +       the cover letter, infer the subject from the branch's
> +       description.
> ++
> +Similar to a commit message, the subject is inferred as the beginning of
> +the description up to and excluding the first blank line.

I think this can all be collapsed to the simpler:

    Use the beginning of the branch description (up to the first
    blank line) as the cover letter subject instead of the default
    "*** SUBJECT HERE ***".

or something.

> diff --git a/builtin/log.c b/builtin/log.c
> @@ -1577,6 +1589,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> +               OPT_BOOL(0, "infer-cover-subject", &infer_cover_subject,
> +                           N_("infer a cover letter subject from the branch description")),

Shorter: "infer cover letter subject from branch description"
diff mbox series

Patch

diff --git a/Documentation/config/format.txt b/Documentation/config/format.txt
index 60e4e92885..9cd6a64e3e 100644
--- a/Documentation/config/format.txt
+++ b/Documentation/config/format.txt
@@ -36,6 +36,10 @@  format.subjectPrefix::
 	The default for format-patch is to output files with the '[PATCH]'
 	subject prefix. Use this variable to change that prefix.
 
+format.inferCoverSubject::
+	A boolean value which lets you enable the
+	`--infer-cover-subject` option of format-patch by default.
+
 format.signature::
 	The default for format-patch is to output a signature containing
 	the Git version number. Use this variable to change that default.
diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index 95bc4d53ca..00ccfb51e7 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -19,6 +19,7 @@  SYNOPSIS
 		   [--start-number <n>] [--numbered-files]
 		   [--in-reply-to=<Message-Id>] [--suffix=.<sfx>]
 		   [--ignore-if-in-upstream]
+		   [--[no-]infer-cover-subject]
 		   [--rfc] [--subject-prefix=<Subject-Prefix>]
 		   [(--reroll-count|-v) <n>]
 		   [--to=<email>] [--cc=<email>]
@@ -171,6 +172,14 @@  will want to ensure that threading is disabled for `git send-email`.
 	patches being generated, and any patch that matches is
 	ignored.
 
+--[no-]infer-cover-subject::
+	Instead of using the default "*** SUBJECT HERE ***" subject for
+	the cover letter, infer the subject from the branch's
+	description.
++
+Similar to a commit message, the subject is inferred as the beginning of
+the description up to and excluding the first blank line.
+
 --subject-prefix=<Subject-Prefix>::
 	Instead of the standard '[PATCH]' prefix in the subject
 	line, instead use '[<Subject-Prefix>]'. This
@@ -347,6 +356,7 @@  with configuration variables.
 	signOff = true
 	outputDirectory = <directory>
 	coverLetter = auto
+	inferCoverSubject = true
 ------------
 
 
diff --git a/builtin/log.c b/builtin/log.c
index 44b10b3415..b15754e282 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -774,6 +774,7 @@  static const char *signature = git_version_string;
 static const char *signature_file;
 static int config_cover_letter;
 static const char *config_output_directory;
+static int infer_cover_subject;
 
 enum {
 	COVER_UNSET,
@@ -887,6 +888,10 @@  static int git_format_config(const char *var, const char *value, void *cb)
 		}
 		return 0;
 	}
+	if (!strcmp(var, "format.infercoversubject")) {
+		infer_cover_subject = git_config_bool(var, value);
+		return 0;
+	}
 
 	return git_log_config(var, value, cb);
 }
@@ -993,20 +998,6 @@  static void print_signature(FILE *file)
 	putc('\n', file);
 }
 
-static void add_branch_description(struct strbuf *buf, const char *branch_name)
-{
-	struct strbuf desc = STRBUF_INIT;
-	if (!branch_name || !*branch_name)
-		return;
-	read_branch_desc(&desc, branch_name);
-	if (desc.len) {
-		strbuf_addch(buf, '\n');
-		strbuf_addbuf(buf, &desc);
-		strbuf_addch(buf, '\n');
-	}
-	strbuf_release(&desc);
-}
-
 static char *find_branch_name(struct rev_info *rev)
 {
 	int i, positive = -1;
@@ -1057,13 +1048,17 @@  static void make_cover_letter(struct rev_info *rev, int use_stdout,
 			      struct commit *origin,
 			      int nr, struct commit **list,
 			      const char *branch_name,
+			      int infer_subject,
 			      int quiet)
 {
 	const char *committer;
-	const char *body = "*** SUBJECT HERE ***\n\n*** BLURB HERE ***\n";
-	const char *msg;
+	const char *subject = "*** SUBJECT HERE ***";
+	const char *body = "*** BLURB HERE ***";
+	const char *description = NULL;
 	struct shortlog log;
 	struct strbuf sb = STRBUF_INIT;
+	struct strbuf description_sb = STRBUF_INIT;
+	struct strbuf subject_sb = STRBUF_INIT;
 	int i;
 	const char *encoding = "UTF-8";
 	int need_8bit_cte = 0;
@@ -1091,17 +1086,34 @@  static void make_cover_letter(struct rev_info *rev, int use_stdout,
 	if (!branch_name)
 		branch_name = find_branch_name(rev);
 
-	msg = body;
+	if (branch_name && *branch_name)
+		read_branch_desc(&description_sb, branch_name);
+
+	if (description_sb.len) {
+		if (infer_subject) {
+			description = format_subject(&subject_sb, description_sb.buf, " ");
+			subject = subject_sb.buf;
+		} else {
+			description = description_sb.buf;
+		}
+	}
+
 	pp.fmt = CMIT_FMT_EMAIL;
 	pp.date_mode.type = DATE_RFC2822;
 	pp.rev = rev;
 	pp.print_email_subject = 1;
 	pp_user_info(&pp, NULL, &sb, committer, encoding);
-	pp_title_line(&pp, &msg, &sb, encoding, need_8bit_cte);
-	pp_remainder(&pp, &msg, &sb, 0);
-	add_branch_description(&sb, branch_name);
+	pp_title_line(&pp, &subject, &sb, encoding, need_8bit_cte);
+	pp_remainder(&pp, &body, &sb, 0);
+	if (description) {
+		strbuf_addch(&sb, '\n');
+		strbuf_addstr(&sb, description);
+		strbuf_addch(&sb, '\n');
+	}
 	fprintf(rev->diffopt.file, "%s\n", sb.buf);
 
+	strbuf_release(&description_sb);
+	strbuf_release(&subject_sb);
 	strbuf_release(&sb);
 
 	shortlog_init(&log);
@@ -1577,6 +1589,8 @@  int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		{ OPTION_CALLBACK, 0, "rfc", &rev, NULL,
 			    N_("Use [RFC PATCH] instead of [PATCH]"),
 			    PARSE_OPT_NOARG | PARSE_OPT_NONEG, rfc_callback },
+		OPT_BOOL(0, "infer-cover-subject", &infer_cover_subject,
+			    N_("infer a cover letter subject from the branch description")),
 		{ OPTION_CALLBACK, 0, "subject-prefix", &rev, N_("prefix"),
 			    N_("Use [<prefix>] instead of [PATCH]"),
 			    PARSE_OPT_NONEG, subject_prefix_callback },
@@ -1916,7 +1930,7 @@  int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		if (thread)
 			gen_message_id(&rev, "cover");
 		make_cover_letter(&rev, use_stdout,
-				  origin, nr, list, branch_name, quiet);
+				  origin, nr, list, branch_name, infer_cover_subject, quiet);
 		print_bases(&bases, rev.diffopt.file);
 		print_signature(rev.diffopt.file);
 		total++;
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 0114608b1f..ad920bb063 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1558,6 +1558,39 @@  test_expect_success 'format patch ignores color.ui' '
 	test_cmp expect actual
 '
 
+test_expect_success 'cover letter with config subject' '
+	test_config branch.rebuild-1.description "config subject
+
+body" &&
+	test_config format.inferCoverSubject true &&
+	git checkout rebuild-1 &&
+	git format-patch --stdout --cover-letter master >actual &&
+	grep "^Subject: \[PATCH 0/2\] config subject$" actual &&
+	grep "^body" actual
+'
+
+test_expect_success 'cover letter with command-line subject' '
+	test_config branch.rebuild-1.description "command-line subject
+
+body" &&
+	git checkout rebuild-1 &&
+	git format-patch --stdout --cover-letter --infer-cover-subject master >actual &&
+	grep "^Subject: \[PATCH 0/2\] command-line subject$" actual &&
+	grep "^body" actual
+'
+
+test_expect_success 'cover letter with command-line --no-infer-cover-subject overrides config' '
+	test_config branch.rebuild-1.description "config subject
+
+body" &&
+	test_config format.inferCoverSubject true &&
+	git checkout rebuild-1 &&
+	git format-patch --stdout --cover-letter --no-infer-cover-subject master >actual &&
+	grep "^Subject: \[PATCH 0/2\] \*\*\* SUBJECT HERE \*\*\*$" actual &&
+	grep "^config subject" actual &&
+	grep "^body" actual
+'
+
 test_expect_success 'cover letter using branch description (1)' '
 	git checkout rebuild-1 &&
 	test_config branch.rebuild-1.description hello &&