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

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

[Daniel Barkalow]

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                                      |  215 ++++++++++++++------
 t/t4013-diff-various.sh                            |    1 +
 ...tch_--stdout_--cover-letter_-n_initial..master^ |  101 +++++++++
 4 files changed, 265 insertions(+), 58 deletions(-)
 create mode 100644 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..7ec01a0 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
@@ -139,6 +140,11 @@ include::diff-options.txt[]
 	Instead of using `.patch` as the suffix for generated
 	filenames, use specified suffix.  A common alternative is
 	`--suffix=.txt`.
+
+--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.
 +
 Note that you would need to include the leading dot `.` if you
 want a filename like `0001-description-of-my-change.patch`, and
diff --git a/builtin-log.c b/builtin-log.c
index 1f74d66..c6e3f91 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";
@@ -453,74 +454,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 (!filename)
+		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);
 	}
 
@@ -591,6 +595,74 @@ static void gen_message_id(struct rev_info *info, char *base)
 	info->message_id = buf.buf;
 }
 
+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;
@@ -626,6 +698,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;
@@ -725,6 +799,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];
 	}
@@ -776,6 +852,20 @@ 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 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;
+			else
+				head = (struct commit *)o;
+		}
+		/* 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);
@@ -802,9 +892,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];
@@ -819,10 +918,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.27.gf6864

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

[Peter Oberndorfer]

On Mittwoch 06 Februar 2008, Daniel Barkalow wrote:
> 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>
> ---
> +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++] = '/';
>  	}
>  
*snip*
> +	if (!filename)
> +		len += sprintf(filename + len, "%d", nr);
maybe this should be !oneline instead?
> +	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);
>  	}

> +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";
I don't know git policy but maybe use
const char body[] = "*** SUBJECT HERE ***\n\n\n*** BLURB HERE ***\n";
since you don't change the pointer.
(or remove the variable)
> +	const char *msg;
> +	const char *extra_headers = rev->extra_headers;
> +	struct strbuf sb;
> +	const char *encoding = "utf-8";
same here
> +
> +	if (rev->commit_format != CMIT_FMT_EMAIL)
> +		die("Cover letter needs email format");

It might be useful to split the reopen_stdout/get_oneline_for_filename
into a separate patch.

When i tried to do this --cover-letter function i went the way to create a 
empty fake commit and let log_tree_commit do all the formating stuff for me.
But i don't know if which way is preferred...

Does you patch set up a reply to chain,
so patches are in reply to cover letter?
I remember battling a bit to set it up reasonably.

Greetings Peter
Who tried to create this --cover-letter function
but gave up silently when his patch mails never reached the ML :-(

I don't know if my patches are of any use (do not apply cleanly anymore, 
reading cover letter message from a file does not honor encoding in any way)
But i can send them to you if you want...

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

[Daniel Barkalow]

On Wed, 6 Feb 2008, Peter Oberndorfer wrote:

> On Mittwoch 06 Februar 2008, Daniel Barkalow wrote:
> > 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>
> > ---
> > +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++] = '/';
> >  	}
> >  
> *snip*
> > +	if (!filename)
> > +		len += sprintf(filename + len, "%d", nr);
> maybe this should be !oneline instead?

Yup.

> > +	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);
> >  	}
> 
> > +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";
> I don't know git policy but maybe use
> const char body[] = "*** SUBJECT HERE ***\n\n\n*** BLURB HERE ***\n";
> since you don't change the pointer.
> (or remove the variable)

It's there to make it easy to get the message from somewhere else later, 
with something of the form:

	if (have_cover_letter_text)
		body = cover_letter_text;

Likewise encoding.

> > +	const char *msg;
> > +	const char *extra_headers = rev->extra_headers;
> > +	struct strbuf sb;
> > +	const char *encoding = "utf-8";
> same here
> > +
> > +	if (rev->commit_format != CMIT_FMT_EMAIL)
> > +		die("Cover letter needs email format");
> 
> It might be useful to split the reopen_stdout/get_oneline_for_filename
> into a separate patch.

Perhaps; I mostly just got lazy, since I had these changes together and 
had already finished with my shuffling things around patch.

> When i tried to do this --cover-letter function i went the way to create a 
> empty fake commit and let log_tree_commit do all the formating stuff for me.
> But i don't know if which way is preferred...

This way looks cleaner and more obvious to me, anyway.

> Does you patch set up a reply to chain,
> so patches are in reply to cover letter?
> I remember battling a bit to set it up reasonably.

I believe that, if you use --thread, all of the later messages are replies 
to the first; if you use --cover-letter, the first is the cover letter.

> Greetings Peter
> Who tried to create this --cover-letter function
> but gave up silently when his patch mails never reached the ML :-(
> 
> I don't know if my patches are of any use (do not apply cleanly anymore, 
> reading cover letter message from a file does not honor encoding in any way)
> But i can send them to you if you want...

I think one other person's implementation was sufficient. :)

	-Daniel
*This .sig left intentionally blank*

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

[Junio C Hamano]

Daniel Barkalow <barkalow@iabervon.org> writes:

> diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
> index 651efe6..7ec01a0 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
> @@ -139,6 +140,11 @@ include::diff-options.txt[]
>  	Instead of using `.patch` as the suffix for generated
>  	filenames, use specified suffix.  A common alternative is
>  	`--suffix=.txt`.
> +
> +--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.
>  +
>  Note that you would need to include the leading dot `.` if you
>  want a filename like `0001-description-of-my-change.patch`, and

Huh?  Leading dot?  The insertion is one paragraph too low or high.

> diff --git a/builtin-log.c b/builtin-log.c
> index 1f74d66..c6e3f91 100644
> --- a/builtin-log.c
> +++ b/builtin-log.c
> @@ -453,74 +454,77 @@ static int git_format_config(...
> ...
>  static FILE *realstdout = NULL;
>  static const char *output_directory = NULL;
>  
> +static int reopen_stdout(const char *oneline, int nr, int total)
>  {
>  	char filename[PATH_MAX];
>  	int len = 0;
>  	int suffix_len = strlen(fmt_patch_suffix) + 1;
>  
>  	if (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");
>  		if (filename[len - 1] != '/')
>  			filename[len++] = '/';
>  	}
>  
> +	if (!filename)
> +		len += sprintf(filename + len, "%d", nr);

How can this trigger?  Do you mean "oneline"?

I can understand that you do not want to pass numbered_files
variable separately to the function, but then there should be a
comment at the beginning of this function that says "oneline is
NULL under numbered_files mode, the caller is responsible for
ensuring that".

> +	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);
>  	}
>  
> @@ -591,6 +595,74 @@ static void gen_message_id(struct rev_info *info, char *base)
> +	/*
> +	 * We can only do diffstat with a unique reference point, and
> +	 * log is a bit tricky, so just skip it.
> +	 */
> +	if (!origin)
> +		return;

Maybe we would want to leave a note in the output to tell your
users that we punted here.

> +	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);

Makes me wonder if we already have enough infrastructure to do
this without spawning two new processes.  Not complaining, but
just wondering.

> @@ -776,6 +852,20 @@ 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 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;
> +			else
> +				head = (struct commit *)o;
> +		}

What if you have more than one origin or head?  Perhaps the
punting logic should be modified to make sure we only have one
negative and one positive and nothing else?

> +		/* We can't generate a cover letter without any patches */
> +		if (!head)
> +			return 0;
> +	}

That's true, but with or without cover letter we won't have
anything to format if we do not have any positive commit.

> @@ -802,9 +892,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;

Nice attention to the detail, moving add_signoff after the cover
letter.

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

[Daniel Barkalow]

On Wed, 6 Feb 2008, Junio C Hamano wrote:

> Daniel Barkalow <barkalow@iabervon.org> writes:
> 
> > diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
> > index 651efe6..7ec01a0 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
> > @@ -139,6 +140,11 @@ include::diff-options.txt[]
> >  	Instead of using `.patch` as the suffix for generated
> >  	filenames, use specified suffix.  A common alternative is
> >  	`--suffix=.txt`.
> > +
> > +--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.
> >  +
> >  Note that you would need to include the leading dot `.` if you
> >  want a filename like `0001-description-of-my-change.patch`, and
> 
> Huh?  Leading dot?  The insertion is one paragraph too low or high.

Uh, right. :)

> > diff --git a/builtin-log.c b/builtin-log.c
> > index 1f74d66..c6e3f91 100644
> > --- a/builtin-log.c
> > +++ b/builtin-log.c
> > @@ -453,74 +454,77 @@ static int git_format_config(...
> > ...
> >  static FILE *realstdout = NULL;
> >  static const char *output_directory = NULL;
> >  
> > +static int reopen_stdout(const char *oneline, int nr, int total)
> >  {
> >  	char filename[PATH_MAX];
> >  	int len = 0;
> >  	int suffix_len = strlen(fmt_patch_suffix) + 1;
> >  
> >  	if (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");
> >  		if (filename[len - 1] != '/')
> >  			filename[len++] = '/';
> >  	}
> >  
> > +	if (!filename)
> > +		len += sprintf(filename + len, "%d", nr);
> 
> How can this trigger?  Do you mean "oneline"?

Yes.

> I can understand that you do not want to pass numbered_files
> variable separately to the function, but then there should be a
> comment at the beginning of this function that says "oneline is
> NULL under numbered_files mode, the caller is responsible for
> ensuring that".

I think the code is clearer than the option. It doesn't append the first 
line of the commit if you don't call it with the first line of the commit. 
How the user causes this to happen is a matter for cmd_format_patch to 
determine.

> > +	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);
> >  	}
> >  
> > @@ -591,6 +595,74 @@ static void gen_message_id(struct rev_info *info, char *base)
> > +	/*
> > +	 * We can only do diffstat with a unique reference point, and
> > +	 * log is a bit tricky, so just skip it.
> > +	 */
> > +	if (!origin)
> > +		return;
> 
> Maybe we would want to leave a note in the output to tell your
> users that we punted here.

I think that if you're in this situation, you pretty much aren't expecting 
anything here, and it would be a bit annoying to stick a note into the 
email that the user then has to remove.

> > +	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);
> 
> Makes me wonder if we already have enough infrastructure to do
> this without spawning two new processes.  Not complaining, but
> just wondering.

I don't know. They're both things that have a tendancy to interfere with 
other things, I think. You'd probably know better than me. If you think 
it's likely to work, I can try...

> > @@ -776,6 +852,20 @@ 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 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;
> > +			else
> > +				head = (struct commit *)o;
> > +		}
> 
> What if you have more than one origin or head?  Perhaps the
> punting logic should be modified to make sure we only have one
> negative and one positive and nothing else?

I think the command line parsing logic prevents us from getting more than 
one head in cmd_format_patch. We can have any number of negatives, with 0 
being the interesting case ("git format-patch -3"). I may have failed to 
consider more than one negative (which would be okay, but should again 
skip the diffstat and, for now, the shortlog).

> > +		/* We can't generate a cover letter without any patches */
> > +		if (!head)
> > +			return 0;
> > +	}
> 
> That's true, but with or without cover letter we won't have
> anything to format if we do not have any positive commit.

If we don't have a cover letter and don't have any positive commit, we can 
perfectly happily output what was asked of us (i.e., nothing). If we're 
asked for a cover letter, we fail, because we can't generate it.

> > @@ -802,9 +892,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;
> 
> Nice attention to the detail, moving add_signoff after the cover
> letter.

I think I actually added the cover letter before the sign-off, and then 
moved in_reply_to above the cover letter, and diff just decided to show it 
this way. But yes, the cover letter shouldn't be signed off.

	-Daniel
*This .sig left intentionally blank*

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

[Johannes Schindelin]

Hi,

I don't know about you, but with lines...

On Wed, 6 Feb 2008, Peter Oberndorfer wrote:

> On Mittwoch 06 Februar 2008, Daniel Barkalow wrote:
>

like this:

> > -		strlcpy(filename, output_directory, sizeof(filename) -
suffix_len);

... (when the mailer wraps the original line), comments like this...

> > +		len += sprintf(filename + len, "%d", nr);
> maybe this should be !oneline instead?
> > +	else {

... are extremely hard to spot.  You could help people who are too dumb to 
do that easily (such as yours truly), by putting an empty line between 
every text you quote and your comments.

Thanks,
Dscho

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

[Daniel Barkalow]

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^

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

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

[Junio C Hamano]

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.

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

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

[Johannes Schindelin]

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?

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

> @@ -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?

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

> +	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?

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

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

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

[Daniel Barkalow]

On Sun, 17 Feb 2008, Junio C Hamano wrote:

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

> > @@ -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*

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

[Daniel Barkalow]

On Mon, 18 Feb 2008, Johannes Schindelin wrote:

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

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

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

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

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

> > +		/* 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.

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

> > +		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*

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

[Junio C Hamano]

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?

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

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

[Daniel Barkalow]

On Mon, 18 Feb 2008, Junio C Hamano wrote:

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