threads / patch / 12152

patch, 4 partsAdd a --cover-letter option to format-patch

Subject: [PATCH 4/4] Add a --cover-letter option to format-patch

## tl;dr

7 messages between Feb 17, 2008 and Feb 18, 2008. Diffs are folded; open one to read it.

replies: 6people: 3as markdown or json

Daniel Barkalow· Feb 17, 2008, 18:35 UTC · lore

If --cover-letter is provided, generate a cover letter message before the patches, numbered 0.

Original patch thanks to Johannes Schindelin
Signed-off-by: Daniel Barkalow <barkalow@iabervon.org>
---
 Documentation/git-format-patch.txt                 |    6 +
 builtin-log.c                                      |  220 ++++++++++++++-----
 t/t4013-diff-various.sh                            |    1 +
 ...tch_--stdout_--cover-letter_-n_initial..master^ |  101 +++++++++
 4 files changed, 270 insertions(+), 58 deletions(-)
 create mode 100644 t/t4013/diff.format-patch_--stdout_--cover-letter_-n_initial..master^
Show changes to 4 files +265 −58

Documentation/git-format-patch.txt, builtin-log.c, t/t4013-diff-various.sh, t/t4013/diff.format-patch_--stdout_--cover-letter_-n_initial..master^

diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index 651efe6..53e6d2e 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -17,6 +17,7 @@ SYNOPSIS
                    [--in-reply-to=Message-Id] [--suffix=.<sfx>]
                    [--ignore-if-in-upstream]
                    [--subject-prefix=Subject-Prefix]
+                   [--cover-letter]
 		   [ <since> | <revision range> ]
 
 DESCRIPTION
@@ -135,6 +136,11 @@ include::diff-options.txt[]
 	allows for useful naming of a patch series, and can be
 	combined with the --numbered option.
 
+--cover-letter::
+	Generate a cover letter template.  You still have to fill in
+	a description, but the shortlog and the diffstat will be
+	generated for you.
+
 --suffix=.<sfx>::
 	Instead of using `.patch` as the suffix for generated
 	filenames, use specified suffix.  A common alternative is
diff --git a/builtin-log.c b/builtin-log.c
index 867cc13..bfefb67 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -14,6 +14,7 @@
 #include "reflog-walk.h"
 #include "patch-ids.h"
 #include "refs.h"
+#include "run-command.h"
 
 static int default_show_root = 1;
 static const char *fmt_patch_subject_prefix = "PATCH";
@@ -452,74 +453,77 @@ static int git_format_config(const char *var, const char *value)
 }
 
 
+static const char *get_oneline_for_filename(struct commit *commit,
+		int keep_subject)
+{
+	static char filename[PATH_MAX];
+	char *sol;
+	int len = 0;
+
+	sol = strstr(commit->buffer, "\n\n");
+	if (!sol)
+		filename[0] = '\0';
+	else {
+		int j, space = 0;
+
+		sol += 2;
+		/* strip [PATCH] or [PATCH blabla] */
+		if (!keep_subject && !prefixcmp(sol, "[PATCH")) {
+			char *eos = strchr(sol + 6, ']');
+			if (eos) {
+				while (isspace(*eos))
+					eos++;
+				sol = eos;
+			}
+		}
+
+		for (j = 0; len < sizeof(filename) &&
+				sol[j] && sol[j] != '\n'; j++) {
+			if (istitlechar(sol[j])) {
+				if (space) {
+					filename[len++] = '-';
+					space = 0;
+				}
+				filename[len++] = sol[j];
+				if (sol[j] == '.')
+					while (sol[j + 1] == '.')
+						j++;
+			} else
+				space = 1;
+		}
+		while (filename[len - 1] == '.'
+		       || filename[len - 1] == '-')
+			len--;
+		filename[len] = '\0';
+	}
+	return filename;
+}
+
 static FILE *realstdout = NULL;
 static const char *output_directory = NULL;
 
-static int reopen_stdout(struct commit *commit, int nr, int keep_subject,
-			 int numbered_files)
+static int reopen_stdout(const char *oneline, int nr, int total)
 {
 	char filename[PATH_MAX];
-	char *sol;
 	int len = 0;
 	int suffix_len = strlen(fmt_patch_suffix) + 1;
 
 	if (output_directory) {
-		if (strlen(output_directory) >=
+		len = snprintf(filename, sizeof(filename), "%s",
+				output_directory);
+		if (len >=
 		    sizeof(filename) - FORMAT_PATCH_NAME_MAX - suffix_len)
 			return error("name of output directory is too long");
-		strlcpy(filename, output_directory, sizeof(filename) - suffix_len);
-		len = strlen(filename);
 		if (filename[len - 1] != '/')
 			filename[len++] = '/';
 	}
 
-	if (numbered_files) {
-		sprintf(filename + len, "%d", nr);
-		len = strlen(filename);
-
-	} else {
-		sprintf(filename + len, "%04d", nr);
-		len = strlen(filename);
-
-		sol = strstr(commit->buffer, "\n\n");
-		if (sol) {
-			int j, space = 1;
-
-			sol += 2;
-			/* strip [PATCH] or [PATCH blabla] */
-			if (!keep_subject && !prefixcmp(sol, "[PATCH")) {
-				char *eos = strchr(sol + 6, ']');
-				if (eos) {
-					while (isspace(*eos))
-						eos++;
-					sol = eos;
-				}
-			}
-
-			for (j = 0;
-			     j < FORMAT_PATCH_NAME_MAX - suffix_len - 5 &&
-				     len < sizeof(filename) - suffix_len &&
-				     sol[j] && sol[j] != '\n';
-			     j++) {
-				if (istitlechar(sol[j])) {
-					if (space) {
-						filename[len++] = '-';
-						space = 0;
-					}
-					filename[len++] = sol[j];
-					if (sol[j] == '.')
-						while (sol[j + 1] == '.')
-							j++;
-				} else
-					space = 1;
-			}
-			while (filename[len - 1] == '.'
-			       || filename[len - 1] == '-')
-				len--;
-			filename[len] = 0;
-		}
-		if (len + suffix_len >= sizeof(filename))
-			return error("Patch pathname too long");
+	if (!oneline)
+		len += sprintf(filename + len, "%d", nr);
+	else {
+		len += sprintf(filename + len, "%04d-", nr);
+		len += snprintf(filename + len, sizeof(filename) - len - 1
+				- suffix_len, "%s", oneline);
 		strcpy(filename + len, fmt_patch_suffix);
 	}
 
@@ -590,6 +594,74 @@ static void gen_message_id(struct rev_info *info, char *base)
 	info->message_id = strbuf_detach(&buf, NULL);
 }
 
+static void make_cover_letter(struct rev_info *rev,
+		int use_stdout, int numbered, int numbered_files,
+			      struct commit *origin, struct commit *head)
+{
+	const char *committer;
+	const char *origin_sha1, *head_sha1;
+	const char *argv[7];
+	const char *subject_start = NULL;
+	const char *body = "*** SUBJECT HERE ***\n\n\n*** BLURB HERE ***\n";
+	const char *msg;
+	const char *extra_headers = rev->extra_headers;
+	struct strbuf sb;
+	const char *encoding = "utf-8";
+
+	if (rev->commit_format != CMIT_FMT_EMAIL)
+		die("Cover letter needs email format");
+
+	if (!use_stdout && reopen_stdout(numbered_files ?
+				NULL : "cover-letter", 0, rev->total))
+		return;
+
+	origin_sha1 = sha1_to_hex(origin ? origin->object.sha1 : null_sha1);
+	head_sha1 = sha1_to_hex(head->object.sha1);
+
+	write_email_headers(rev, head_sha1, &subject_start, &extra_headers);
+
+	committer = git_committer_info(0);
+
+	msg = body;
+	strbuf_init(&sb, 0);
+	add_user_info(NULL, CMIT_FMT_EMAIL, &sb, committer, DATE_RFC2822,
+		      encoding);
+	pp_title_line(CMIT_FMT_EMAIL, &msg, &sb, subject_start, extra_headers,
+		      encoding, 0);
+	pp_remainder(CMIT_FMT_EMAIL, &msg, &sb, 0);
+	printf("%s\n", sb.buf);
+
+	strbuf_release(&sb);
+
+	/*
+	 * We can only do diffstat with a unique reference point, and
+	 * log is a bit tricky, so just skip it.
+	 */
+	if (!origin)
+		return;
+
+	argv[0] = "shortlog";
+	argv[1] = head_sha1;
+	argv[2] = "--not";
+	argv[3] = origin_sha1;
+	argv[4] = NULL;
+	fflush(stdout);
+	run_command_v_opt(argv, RUN_GIT_CMD);
+
+	argv[0] = "diff";
+	argv[1] = "--stat";
+	argv[2] = "--summary";
+	argv[3] = head_sha1;
+	argv[4] = "--not";
+	argv[5] = origin_sha1;
+	argv[6] = NULL;
+	fflush(stdout);
+	run_command_v_opt(argv, RUN_GIT_CMD);
+
+	fflush(stdout);
+	printf("\n");
+}
+
 static const char *clean_message_id(const char *msg_id)
 {
 	char ch;
@@ -625,6 +697,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	int subject_prefix = 0;
 	int ignore_if_in_upstream = 0;
 	int thread = 0;
+	int cover_letter = 0;
+	struct commit *origin = NULL, *head = NULL;
 	const char *in_reply_to = NULL;
 	struct patch_ids ids;
 	char *add_signoff = NULL;
@@ -724,6 +798,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 			rev.subject_prefix = argv[i] + 17;
 		} else if (!prefixcmp(argv[i], "--suffix="))
 			fmt_patch_suffix = argv[i] + 9;
+		else if (!strcmp(argv[i], "--cover-letter"))
+			cover_letter = 1;
 		else
 			argv[j++] = argv[i];
 	}
@@ -775,6 +851,25 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		 * get_revision() to do the usual traversal.
 		 */
 	}
+	if (cover_letter) {
+		/* remember the range */
+		int negative_count = 0;
+		int i;
+		for (i = 0; i < rev.pending.nr; i++) {
+			struct object *o = rev.pending.objects[i].item;
+			if (o->flags & UNINTERESTING) {
+				origin = (struct commit *)o;
+				negative_count++;
+			} else
+				head = (struct commit *)o;
+		}
+		/* Multiple origins don't work for diffstat. */
+		if (negative_count > 1)
+			origin = NULL;
+		/* We can't generate a cover letter without any patches */
+		if (!head)
+			return 0;
+	}
 
 	if (ignore_if_in_upstream)
 		get_patch_ids(&rev, &ids, prefix);
@@ -801,9 +896,18 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		numbered = 1;
 	if (numbered)
 		rev.total = total + start_number - 1;
-	rev.add_signoff = add_signoff;
 	if (in_reply_to)
 		rev.ref_message_id = clean_message_id(in_reply_to);
+	if (cover_letter) {
+		if (thread) {
+			gen_message_id(&rev, "cover");
+		}
+		make_cover_letter(&rev, use_stdout, numbered, numbered_files,
+				  origin, head);
+		total++;
+		start_number--;
+	}
+	rev.add_signoff = add_signoff;
 	while (0 <= --nr) {
 		int shown;
 		commit = list[nr];
@@ -818,10 +922,10 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 			}
 			gen_message_id(&rev, sha1_to_hex(commit->object.sha1));
 		}
-		if (!use_stdout)
-			if (reopen_stdout(commit, rev.nr, keep_subject,
-					  numbered_files))
-				die("Failed to create output files");
+		if (!use_stdout && reopen_stdout(numbered_files ? NULL :
+				get_oneline_for_filename(commit, keep_subject),
+				rev.nr, rev.total))
+			die("Failed to create output files");
 		shown = log_tree_commit(&rev, commit);
 		free(commit->buffer);
 		commit->buffer = NULL;
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index 9eec754..6b4d1c5 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -245,6 +245,7 @@ format-patch --inline --stdout initial..master
 format-patch --inline --stdout --subject-prefix=TESTCASE initial..master
 config format.subjectprefix DIFFERENT_PREFIX
 format-patch --inline --stdout initial..master^^
+format-patch --stdout --cover-letter -n initial..master^
 
 diff --abbrev initial..side
 diff -r initial..side
diff --git a/t/t4013/diff.format-patch_--stdout_--cover-letter_-n_initial..master^ b/t/t4013/diff.format-patch_--stdout_--cover-letter_-n_initial..master^
new file mode 100644
index 0000000..311e207
--- /dev/null
+++ b/t/t4013/diff.format-patch_--stdout_--cover-letter_-n_initial..master^
@@ -0,0 +1,101 @@
+$ git format-patch --stdout --cover-letter -n initial..master^
+From 9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0 Mon Sep 17 00:00:00 2001
+From: C O Mitter <committer@example.com>
+Date: Mon, 26 Jun 2006 00:05:00 +0000
+Subject: [DIFFERENT_PREFIX 0/2] *** SUBJECT HERE ***
+
+
+*** BLURB HERE ***
+
+A U Thor (2):
+      Second
+      Third
+
+ dir/sub |    4 ++++
+ file0   |    3 +++
+ file1   |    3 +++
+ file2   |    3 ---
+ 4 files changed, 10 insertions(+), 3 deletions(-)
+ create mode 100644 file1
+ delete mode 100644 file2
+
+From 1bde4ae5f36c8d9abe3a0fce0c6aab3c4a12fe44 Mon Sep 17 00:00:00 2001
+From: A U Thor <author@example.com>
+Date: Mon, 26 Jun 2006 00:01:00 +0000
+Subject: [DIFFERENT_PREFIX 1/2] Second
+
+This is the second commit.
+---
+ dir/sub |    2 ++
+ file0   |    3 +++
+ file2   |    3 ---
+ 3 files changed, 5 insertions(+), 3 deletions(-)
+ delete mode 100644 file2
+
+diff --git a/dir/sub b/dir/sub
+index 35d242b..8422d40 100644
+--- a/dir/sub
++++ b/dir/sub
+@@ -1,2 +1,4 @@
+ A
+ B
++C
++D
+diff --git a/file0 b/file0
+index 01e79c3..b414108 100644
+--- a/file0
++++ b/file0
+@@ -1,3 +1,6 @@
+ 1
+ 2
+ 3
++4
++5
++6
+diff --git a/file2 b/file2
+deleted file mode 100644
+index 01e79c3..0000000
+--- a/file2
++++ /dev/null
+@@ -1,3 +0,0 @@
+-1
+-2
+-3
+-- 
+g-i-t--v-e-r-s-i-o-n
+
+
+From 9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0 Mon Sep 17 00:00:00 2001
+From: A U Thor <author@example.com>
+Date: Mon, 26 Jun 2006 00:02:00 +0000
+Subject: [DIFFERENT_PREFIX 2/2] Third
+
+---
+ dir/sub |    2 ++
+ file1   |    3 +++
+ 2 files changed, 5 insertions(+), 0 deletions(-)
+ create mode 100644 file1
+
+diff --git a/dir/sub b/dir/sub
+index 8422d40..cead32e 100644
+--- a/dir/sub
++++ b/dir/sub
+@@ -2,3 +2,5 @@ A
+ B
+ C
+ D
++E
++F
+diff --git a/file1 b/file1
+new file mode 100644
+index 0000000..b1e6722
+--- /dev/null
++++ b/file1
+@@ -0,0 +1,3 @@
++A
++B
++C
+-- 
+g-i-t--v-e-r-s-i-o-n
+
+$
-- 
1.5.4.1.1350.g2b9ee
Junio C Hamano· Feb 18, 2008, 03:45 UTC · re: Daniel Barkalow · lore

Re: [PATCH 4/4] Add a --cover-letter option to format-patch

Daniel Barkalow <barkalow@iabervon.org> writes:
Show 7 quoted lines
> +	argv[0] = "shortlog";
> +	argv[1] = head_sha1;
> +	argv[2] = "--not";
> +	argv[3] = origin_sha1;
> +	argv[4] = NULL;
> +	fflush(stdout);
> +	run_command_v_opt(argv, RUN_GIT_CMD);

Please make it a habit to always append "--" disambiguator for such command lines generated internally. In this case you are not usinging short and common name like "master" or "HEAD" but 40-letter-long hex string, so it is much less likely to get hit by the "ambiguous rev vs file" rule, but even then it is still possible that a work tree happens to have a file with the same name.

Show 14 quoted lines
> @@ -0,0 +1,101 @@
> +$ git format-patch --stdout --cover-letter -n initial..master^
> +From 9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0 Mon Sep 17 00:00:00 2001
> +From: C O Mitter <committer@example.com>
> +Date: Mon, 26 Jun 2006 00:05:00 +0000
> +Subject: [DIFFERENT_PREFIX 0/2] *** SUBJECT HERE ***
> +
> +
> +*** BLURB HERE ***
> +
> +A U Thor (2):
> +      Second
> +      Third
> +

This probably falls within the "personal taste" category, but I'd rather not to see two extra blank lines between Subject: and BLURB HERE.

Daniel Barkalow· Feb 18, 2008, 17:15 UTC · re: Junio C Hamano · lore

Re: [PATCH 4/4] Add a --cover-letter option to format-patch

On Sun, 17 Feb 2008, Junio C Hamano wrote:
Show 17 quoted lines
> Daniel Barkalow <barkalow@iabervon.org> writes:
> 
> > +	argv[0] = "shortlog";
> > +	argv[1] = head_sha1;
> > +	argv[2] = "--not";
> > +	argv[3] = origin_sha1;
> > +	argv[4] = NULL;
> > +	fflush(stdout);
> > +	run_command_v_opt(argv, RUN_GIT_CMD);
> 
> Please make it a habit to always append "--" disambiguator for
> such command lines generated internally.  In this case you are
> not usinging short and common name like "master" or "HEAD" but
> 40-letter-long hex string, so it is much less likely to get hit
> by the "ambiguous rev vs file" rule, but even then it is still
> possible that a work tree happens to have a file with the same
> name.
Sure.
Show 18 quoted lines
> > @@ -0,0 +1,101 @@
> > +$ git format-patch --stdout --cover-letter -n initial..master^
> > +From 9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0 Mon Sep 17 00:00:00 2001
> > +From: C O Mitter <committer@example.com>
> > +Date: Mon, 26 Jun 2006 00:05:00 +0000
> > +Subject: [DIFFERENT_PREFIX 0/2] *** SUBJECT HERE ***
> > +
> > +
> > +*** BLURB HERE ***
> > +
> > +A U Thor (2):
> > +      Second
> > +      Third
> > +
> 
> This probably falls within the "personal taste" category, but
> I'd rather not to see two extra blank lines between Subject: and
> BLURB HERE.

The file is ugly with the double blank line, but I like it for what I think is the normal case, where you import the series into your mail reader, edit the cover letter there (where it appears as a message with some space devoted to the blurb by virtue of starting with now one blank line), and then send. In that context, I think it's nicer.

	-Daniel
*This .sig left intentionally blank*
Johannes Schindelin· Feb 18, 2008, 13:19 UTC · re: Daniel Barkalow · lore

Re: [PATCH 4/4] Add a --cover-letter option to format-patch

Hi,
On Sun, 17 Feb 2008, Daniel Barkalow wrote:
Show 10 quoted lines
> diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
> index 651efe6..53e6d2e 100644
> --- a/Documentation/git-format-patch.txt
> +++ b/Documentation/git-format-patch.txt
> @@ -17,6 +17,7 @@ SYNOPSIS
>                     [--in-reply-to=Message-Id] [--suffix=.<sfx>]
>                     [--ignore-if-in-upstream]
>                     [--subject-prefix=Subject-Prefix]
> +                   [--cover-letter]
>  		   [ <since> | <revision range> ]

Since you are already there, and it is really a small change, why not fix the whitespace?

Show 61 quoted lines
> diff --git a/builtin-log.c b/builtin-log.c
> index 867cc13..bfefb67 100644
> --- a/builtin-log.c
> +++ b/builtin-log.c
> @@ -14,6 +14,7 @@
>  #include "reflog-walk.h"
>  #include "patch-ids.h"
>  #include "refs.h"
> +#include "run-command.h"
>  
>  static int default_show_root = 1;
>  static const char *fmt_patch_subject_prefix = "PATCH";
> @@ -452,74 +453,77 @@ static int git_format_config(const char *var, const char *value)
>  }
>  
>  
> +static const char *get_oneline_for_filename(struct commit *commit,
> +		int keep_subject)
> +{
> +	static char filename[PATH_MAX];
> +	char *sol;
> +	int len = 0;
> +
> +	sol = strstr(commit->buffer, "\n\n");
> +	if (!sol)
> +		filename[0] = '\0';
> +	else {
> +		int j, space = 0;
> +
> +		sol += 2;
> +		/* strip [PATCH] or [PATCH blabla] */
> +		if (!keep_subject && !prefixcmp(sol, "[PATCH")) {
> +			char *eos = strchr(sol + 6, ']');
> +			if (eos) {
> +				while (isspace(*eos))
> +					eos++;
> +				sol = eos;
> +			}
> +		}
> +
> +		for (j = 0; len < sizeof(filename) &&
> +				sol[j] && sol[j] != '\n'; j++) {
> +			if (istitlechar(sol[j])) {
> +				if (space) {
> +					filename[len++] = '-';
> +					space = 0;
> +				}
> +				filename[len++] = sol[j];
> +				if (sol[j] == '.')
> +					while (sol[j + 1] == '.')
> +						j++;
> +			} else
> +				space = 1;
> +		}
> +		while (filename[len - 1] == '.'
> +		       || filename[len - 1] == '-')
> +			len--;
> +		filename[len] = '\0';
> +	}
> +	return filename;
> +}

Where has the FORMAT_PATCH_NAME_MAX handling gone? See c06d2daa(Limit filename for format-patch)...

BTW it is not only because format-patch would fail. The maximum length was chosen such that it does not clutter the window when ls'ing the files.

Show 54 quoted lines
> @@ -590,6 +594,74 @@ static void gen_message_id(struct rev_info *info, char *base)
>  	info->message_id = strbuf_detach(&buf, NULL);
>  }
>  
> +static void make_cover_letter(struct rev_info *rev,
> +		int use_stdout, int numbered, int numbered_files,
> +			      struct commit *origin, struct commit *head)
> +{
> +	const char *committer;
> +	const char *origin_sha1, *head_sha1;
> +	const char *argv[7];
> +	const char *subject_start = NULL;
> +	const char *body = "*** SUBJECT HERE ***\n\n\n*** BLURB HERE ***\n";
> +	const char *msg;
> +	const char *extra_headers = rev->extra_headers;
> +	struct strbuf sb;
> +	const char *encoding = "utf-8";
> +
> +	if (rev->commit_format != CMIT_FMT_EMAIL)
> +		die("Cover letter needs email format");
> +
> +	if (!use_stdout && reopen_stdout(numbered_files ?
> +				NULL : "cover-letter", 0, rev->total))
> +		return;
> +
> +	origin_sha1 = sha1_to_hex(origin ? origin->object.sha1 : null_sha1);
> +	head_sha1 = sha1_to_hex(head->object.sha1);
> +
> +	write_email_headers(rev, head_sha1, &subject_start, &extra_headers);
> +
> +	committer = git_committer_info(0);
> +
> +	msg = body;
> +	strbuf_init(&sb, 0);
> +	add_user_info(NULL, CMIT_FMT_EMAIL, &sb, committer, DATE_RFC2822,
> +		      encoding);
> +	pp_title_line(CMIT_FMT_EMAIL, &msg, &sb, subject_start, extra_headers,
> +		      encoding, 0);
> +	pp_remainder(CMIT_FMT_EMAIL, &msg, &sb, 0);
> +	printf("%s\n", sb.buf);
> +
> +	strbuf_release(&sb);
> +
> +	/*
> +	 * We can only do diffstat with a unique reference point, and
> +	 * log is a bit tricky, so just skip it.
> +	 */
> +	if (!origin)
> +		return;
> +
> +	argv[0] = "shortlog";
> +	argv[1] = head_sha1;
> +	argv[2] = "--not";
> +	argv[3] = origin_sha1;
Should this not output a complete shortlog, if the user said
	git format-patch --root --cover-letter HEAD
Hmm?
Show 10 quoted lines
> +	argv[4] = NULL;
> +	fflush(stdout);
> +	run_command_v_opt(argv, RUN_GIT_CMD);
> +
> +	argv[0] = "diff";
> +	argv[1] = "--stat";
> +	argv[2] = "--summary";
> +	argv[3] = head_sha1;
> +	argv[4] = "--not";
> +	argv[5] = origin_sha1;
Likewise.
Show 30 quoted lines
> +	argv[6] = NULL;
> +	fflush(stdout);
> +	run_command_v_opt(argv, RUN_GIT_CMD);
> +
> +	fflush(stdout);
> +	printf("\n");
> +}
> +
>  static const char *clean_message_id(const char *msg_id)
>  {
>  	char ch;
> @@ -775,6 +851,25 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  		 * get_revision() to do the usual traversal.
>  		 */
>  	}
> +	if (cover_letter) {
> +		/* remember the range */
> +		int negative_count = 0;
> +		int i;
> +		for (i = 0; i < rev.pending.nr; i++) {
> +			struct object *o = rev.pending.objects[i].item;
> +			if (o->flags & UNINTERESTING) {
> +				origin = (struct commit *)o;
> +				negative_count++;
> +			} else
> +				head = (struct commit *)o;
> +		}
> +		/* Multiple origins don't work for diffstat. */
> +		if (negative_count > 1)
> +			origin = NULL;
Oh, and here it gets interesting.  If you say
	git format-patch --cover-letter ^commit1 ^commit2 HEAD

then origin will be set to NULL, and in the diffstat, instead of showing _no_ diffstat, you show the same as if no negative ref was given (which _should_ show a diff against the empty tree, but shows nothing currently).

So I suggest changing "origin" to a commit_list, and have cover-letter _not_ output a diffstat if more than one commits were given.

Or as many diffstats.  Whatever.
> +		/* We can't generate a cover letter without any patches */
> +		if (!head)
> +			return 0;
Should this not
			return error("You want a cover letter for what,"
				"exactly?");
Hmm?
Show 11 quoted lines
> @@ -801,9 +896,18 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  		numbered = 1;
>  	if (numbered)
>  		rev.total = total + start_number - 1;
> -	rev.add_signoff = add_signoff;
>  	if (in_reply_to)
>  		rev.ref_message_id = clean_message_id(in_reply_to);
> +	if (cover_letter) {
> +		if (thread) {
> +			gen_message_id(&rev, "cover");
> +		}
These curly brackets are sure distracting.
Show 6 quoted lines
> +		make_cover_letter(&rev, use_stdout, numbered, numbered_files,
> +				  origin, head);
> +		total++;
> +		start_number--;
> +	}
> +	rev.add_signoff = add_signoff;
Why not sign off the cover letter, too?

BTW I had to chew over the thread handling quite a few minutes... maybe you want to add a test for --cover-letter --thread, just to make sure it does not get broken (and of course, to prove that it works with your patches).

Thanks, Dscho

Daniel Barkalow· Feb 18, 2008, 18:54 UTC · re: Johannes Schindelin · lore

Re: [PATCH 4/4] Add a --cover-letter option to format-patch

On Mon, 18 Feb 2008, Johannes Schindelin wrote:
Show 17 quoted lines
> Hi,
> 
> On Sun, 17 Feb 2008, Daniel Barkalow wrote:
> 
> > diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
> > index 651efe6..53e6d2e 100644
> > --- a/Documentation/git-format-patch.txt
> > +++ b/Documentation/git-format-patch.txt
> > @@ -17,6 +17,7 @@ SYNOPSIS
> >                     [--in-reply-to=Message-Id] [--suffix=.<sfx>]
> >                     [--ignore-if-in-upstream]
> >                     [--subject-prefix=Subject-Prefix]
> > +                   [--cover-letter]
> >  		   [ <since> | <revision range> ]
> 
> Since you are already there, and it is really a small change, why not fix 
> the whitespace?
What *is* the correct whitespace for alignment in documentation?
Show 64 quoted lines
> > diff --git a/builtin-log.c b/builtin-log.c
> > index 867cc13..bfefb67 100644
> > --- a/builtin-log.c
> > +++ b/builtin-log.c
> > @@ -14,6 +14,7 @@
> >  #include "reflog-walk.h"
> >  #include "patch-ids.h"
> >  #include "refs.h"
> > +#include "run-command.h"
> >  
> >  static int default_show_root = 1;
> >  static const char *fmt_patch_subject_prefix = "PATCH";
> > @@ -452,74 +453,77 @@ static int git_format_config(const char *var, const char *value)
> >  }
> >  
> >  
> > +static const char *get_oneline_for_filename(struct commit *commit,
> > +		int keep_subject)
> > +{
> > +	static char filename[PATH_MAX];
> > +	char *sol;
> > +	int len = 0;
> > +
> > +	sol = strstr(commit->buffer, "\n\n");
> > +	if (!sol)
> > +		filename[0] = '\0';
> > +	else {
> > +		int j, space = 0;
> > +
> > +		sol += 2;
> > +		/* strip [PATCH] or [PATCH blabla] */
> > +		if (!keep_subject && !prefixcmp(sol, "[PATCH")) {
> > +			char *eos = strchr(sol + 6, ']');
> > +			if (eos) {
> > +				while (isspace(*eos))
> > +					eos++;
> > +				sol = eos;
> > +			}
> > +		}
> > +
> > +		for (j = 0; len < sizeof(filename) &&
> > +				sol[j] && sol[j] != '\n'; j++) {
> > +			if (istitlechar(sol[j])) {
> > +				if (space) {
> > +					filename[len++] = '-';
> > +					space = 0;
> > +				}
> > +				filename[len++] = sol[j];
> > +				if (sol[j] == '.')
> > +					while (sol[j + 1] == '.')
> > +						j++;
> > +			} else
> > +				space = 1;
> > +		}
> > +		while (filename[len - 1] == '.'
> > +		       || filename[len - 1] == '-')
> > +			len--;
> > +		filename[len] = '\0';
> > +	}
> > +	return filename;
> > +}
> 
> Where has the FORMAT_PATCH_NAME_MAX handling gone?  See c06d2daa(Limit 
> filename for format-patch)...

Wow, beats me. I'd have thought I'd have gotten some conflicts while rebasing.

Anyway, fixed, and I'll add a test for this case.
Show 60 quoted lines
> > @@ -590,6 +594,74 @@ static void gen_message_id(struct rev_info *info, char *base)
> >  	info->message_id = strbuf_detach(&buf, NULL);
> >  }
> >  
> > +static void make_cover_letter(struct rev_info *rev,
> > +		int use_stdout, int numbered, int numbered_files,
> > +			      struct commit *origin, struct commit *head)
> > +{
> > +	const char *committer;
> > +	const char *origin_sha1, *head_sha1;
> > +	const char *argv[7];
> > +	const char *subject_start = NULL;
> > +	const char *body = "*** SUBJECT HERE ***\n\n\n*** BLURB HERE ***\n";
> > +	const char *msg;
> > +	const char *extra_headers = rev->extra_headers;
> > +	struct strbuf sb;
> > +	const char *encoding = "utf-8";
> > +
> > +	if (rev->commit_format != CMIT_FMT_EMAIL)
> > +		die("Cover letter needs email format");
> > +
> > +	if (!use_stdout && reopen_stdout(numbered_files ?
> > +				NULL : "cover-letter", 0, rev->total))
> > +		return;
> > +
> > +	origin_sha1 = sha1_to_hex(origin ? origin->object.sha1 : null_sha1);
> > +	head_sha1 = sha1_to_hex(head->object.sha1);
> > +
> > +	write_email_headers(rev, head_sha1, &subject_start, &extra_headers);
> > +
> > +	committer = git_committer_info(0);
> > +
> > +	msg = body;
> > +	strbuf_init(&sb, 0);
> > +	add_user_info(NULL, CMIT_FMT_EMAIL, &sb, committer, DATE_RFC2822,
> > +		      encoding);
> > +	pp_title_line(CMIT_FMT_EMAIL, &msg, &sb, subject_start, extra_headers,
> > +		      encoding, 0);
> > +	pp_remainder(CMIT_FMT_EMAIL, &msg, &sb, 0);
> > +	printf("%s\n", sb.buf);
> > +
> > +	strbuf_release(&sb);
> > +
> > +	/*
> > +	 * We can only do diffstat with a unique reference point, and
> > +	 * log is a bit tricky, so just skip it.
> > +	 */
> > +	if (!origin)
> > +		return;
> > +
> > +	argv[0] = "shortlog";
> > +	argv[1] = head_sha1;
> > +	argv[2] = "--not";
> > +	argv[3] = origin_sha1;
> 
> Should this not output a complete shortlog, if the user said
> 
> 	git format-patch --root --cover-letter HEAD
> 
> Hmm?

It should, but that's in the "tricky" category. For future work, I want to make it possible to have the log mechanism able to output the same batch of commits twice with different settings, so that the cover letter's short log will be an exact match for the contents of the series no matter what, but that's a lot more work and not needed for the common case, which I'd like to get in first.

Show 12 quoted lines
> > +	argv[4] = NULL;
> > +	fflush(stdout);
> > +	run_command_v_opt(argv, RUN_GIT_CMD);
> > +
> > +	argv[0] = "diff";
> > +	argv[1] = "--stat";
> > +	argv[2] = "--summary";
> > +	argv[3] = head_sha1;
> > +	argv[4] = "--not";
> > +	argv[5] = origin_sha1;
> 
> Likewise.

Here, we really want to be able to ask the log-output stuff if we have exactly one non-included parent of included commits, which is what we should diff against. In any case, again depends on being able to ask for the right info from the library in order to do more than just the easy case without reimplementing a lot or having a mess of special cases.

Show 43 quoted lines
> > +	argv[6] = NULL;
> > +	fflush(stdout);
> > +	run_command_v_opt(argv, RUN_GIT_CMD);
> > +
> > +	fflush(stdout);
> > +	printf("\n");
> > +}
> > +
> >  static const char *clean_message_id(const char *msg_id)
> >  {
> >  	char ch;
> > @@ -775,6 +851,25 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> >  		 * get_revision() to do the usual traversal.
> >  		 */
> >  	}
> > +	if (cover_letter) {
> > +		/* remember the range */
> > +		int negative_count = 0;
> > +		int i;
> > +		for (i = 0; i < rev.pending.nr; i++) {
> > +			struct object *o = rev.pending.objects[i].item;
> > +			if (o->flags & UNINTERESTING) {
> > +				origin = (struct commit *)o;
> > +				negative_count++;
> > +			} else
> > +				head = (struct commit *)o;
> > +		}
> > +		/* Multiple origins don't work for diffstat. */
> > +		if (negative_count > 1)
> > +			origin = NULL;
> 
> Oh, and here it gets interesting.  If you say
> 
> 	git format-patch --cover-letter ^commit1 ^commit2 HEAD
> 
> then origin will be set to NULL, and in the diffstat, instead of showing 
> _no_ diffstat, you show the same as if no negative ref was given (which 
> _should_ show a diff against the empty tree, but shows nothing currently).
>
> So I suggest changing "origin" to a commit_list, and have cover-letter 
> _not_ output a diffstat if more than one commits were given.
> 
> Or as many diffstats.  Whatever.

I think it's better for now to leave the cover letter bare in all of the complicated cases, and add it when the core code can provide more info.

Show 10 quoted lines
> > +		/* We can't generate a cover letter without any patches */
> > +		if (!head)
> > +			return 0;
> 
> Should this not
> 
> 			return error("You want a cover letter for what,"
> 				"exactly?");
> 
> Hmm?

I'm not sure. I decided to use the rule that, if there's nothing to format, you get nothing with no error, which is true before this series. But I think I'd rather get an error message if there's nothing to do. In any case, I don't think the result for no commits should depend on the cover letter option, in any case; the fact that these situations differ on whether the result is technically what you asked for or not isn't that significant.

Show 13 quoted lines
> > @@ -801,9 +896,18 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> >  		numbered = 1;
> >  	if (numbered)
> >  		rev.total = total + start_number - 1;
> > -	rev.add_signoff = add_signoff;
> >  	if (in_reply_to)
> >  		rev.ref_message_id = clean_message_id(in_reply_to);
> > +	if (cover_letter) {
> > +		if (thread) {
> > +			gen_message_id(&rev, "cover");
> > +		}
> 
> These curly brackets are sure distracting.
Yup, fixed.
Show 8 quoted lines
> > +		make_cover_letter(&rev, use_stdout, numbered, numbered_files,
> > +				  origin, head);
> > +		total++;
> > +		start_number--;
> > +	}
> > +	rev.add_signoff = add_signoff;
> 
> Why not sign off the cover letter, too?

I think cover letters aren't normally signed off, since they don't contain any content that goes into the history.

> BTW I had to chew over the thread handling quite a few minutes... maybe 
> you want to add a test for --cover-letter --thread, just to make sure it 
> does not get broken (and of course, to prove that it works with your 
> patches).

Yeah, I probably want a comment explaining it, too, since I just chewed over it myself for a few minutes. Don't forget --in-reply-to=, which also works right.

Actually, I guess it's a bit odd that --thread causes all of your messages to get Message-Id headers, of which at most one is referenced, while not having --thread means that none of them have IDs, even if they reference some other message by way of --in-reply-to=. I suppose that's because we might want to support having each message be a reply to the previous, which is not a style any of us like (I think), but some people somewhere like it, IIRC.

	-Daniel
*This .sig left intentionally blank*
Junio C Hamano· Feb 18, 2008, 21:09 UTC · re: Daniel Barkalow · lore

Re: [PATCH 4/4] Add a --cover-letter option to format-patch

Daniel Barkalow <barkalow@iabervon.org> writes:
Show 15 quoted lines
>> > diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
>> > index 651efe6..53e6d2e 100644
>> > --- a/Documentation/git-format-patch.txt
>> > +++ b/Documentation/git-format-patch.txt
>> > @@ -17,6 +17,7 @@ SYNOPSIS
>> >                     [--in-reply-to=Message-Id] [--suffix=.<sfx>]
>> >                     [--ignore-if-in-upstream]
>> >                     [--subject-prefix=Subject-Prefix]
>> > +                   [--cover-letter]
>> >  		   [ <since> | <revision range> ]
>> 
>> Since you are already there, and it is really a small change, why not fix 
>> the whitespace?
>
> What *is* the correct whitespace for alignment in documentation?

The last time I checked, AsciiDoc output did not change before and after running "expand" and then "unexpand" in all *.txt files in Documentation/ directory, so if it is indenting to multiple of 8, HT is fine.

Don't we have Documentation/.gitattributes that says indentation to 8*N-th column must be done with N HTs these days?

>> Why not sign off the cover letter, too?
>
> I think cover letters aren't normally signed off, since they don't contain 
> any content that goes into the history.
Right.  They do not contain any content that needs DCO.
Daniel Barkalow· Feb 18, 2008, 21:17 UTC · re: Junio C Hamano · lore

Re: [PATCH 4/4] Add a --cover-letter option to format-patch

On Mon, 18 Feb 2008, Junio C Hamano wrote:
Show 25 quoted lines
> Daniel Barkalow <barkalow@iabervon.org> writes:
> 
> >> > diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
> >> > index 651efe6..53e6d2e 100644
> >> > --- a/Documentation/git-format-patch.txt
> >> > +++ b/Documentation/git-format-patch.txt
> >> > @@ -17,6 +17,7 @@ SYNOPSIS
> >> >                     [--in-reply-to=Message-Id] [--suffix=.<sfx>]
> >> >                     [--ignore-if-in-upstream]
> >> >                     [--subject-prefix=Subject-Prefix]
> >> > +                   [--cover-letter]
> >> >  		   [ <since> | <revision range> ]
> >> 
> >> Since you are already there, and it is really a small change, why not fix 
> >> the whitespace?
> >
> > What *is* the correct whitespace for alignment in documentation?
> 
> The last time I checked, AsciiDoc output did not change before
> and after running "expand" and then "unexpand" in all *.txt
> files in Documentation/ directory, so if it is indenting to
> multiple of 8, HT is fine.
> 
> Don't we have Documentation/.gitattributes that says indentation
> to 8*N-th column must be done with N HTs these days?
Ah, yes, that's right; I'll fix that block.
	-Daniel
*This .sig left intentionally blank*

← back to recent threads