[PATCH 0/2] format-patch and send-email

[PATCH 0/2] format-patch and send-email

[Joe Perches]

A couple of patches that have previously been sent to the list

Joe Perches (2):
  git-send-email.perl: Don't add header "In-Reply-To:" when --no-chain-reply-to set
  format-patch: Add --cover-letter-wrap

 Documentation/git-format-patch.txt     |   13 +++++++++
 builtin-log.c                          |   46 +++++++++++++++++++++++++++++---
 contrib/completion/git-completion.bash |    1 +
 git-send-email.perl                    |    3 +-
 4 files changed, 58 insertions(+), 5 deletions(-)

[PATCH 1/2] git-send-email.perl: Don't add header "In-Reply-To:" when --no-chain-reply-to set

[Joe Perches]

using
  git format-patch --thread=shallow -o <foo>
and
  git send-email --no-thread --no-chain-reply-to <foo>

duplicates the headers

  In-Reply-To:
  References:

Signed-off-by: Joe Perches <joe@perches.com>
---
 git-send-email.perl |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 8ce6f1f..1b9b27e 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1151,7 +1151,8 @@ foreach my $t (@files) {
 
 	# set up for the next message
 	if ($thread && $message_was_sent &&
-		($chain_reply_to || !defined $reply_to || length($reply_to) == 0)) {
+	    ($chain_reply_to && 
+	     (!defined $reply_to || length($reply_to) == 0))) {
 		$reply_to = $message_id;
 		if (length $references > 0) {
 			$references .= "\n $message_id";
-- 
1.6.3.1.10.g659a0.dirty

Re: [PATCH 1/2] git-send-email.perl: Don't add header "In-Reply-To:" when --no-chain-reply-to set

[Markus Heidelberg]

Joe Perches, 01.07.2009:
> using
>   git format-patch --thread=shallow -o <foo>
> and
>   git send-email --no-thread --no-chain-reply-to <foo>

I guess you meant --thread here.

> duplicates the headers
> 
>   In-Reply-To:
>   References:

I noticed the duplicated headers when fixing two bugs some weeks ago. I
guess to get rid of the duplicated headers, you have to parse the mail
that format-patch produces, since you don't have a clue, what
format-patch did. I'm not sure if it's worth it, if the duplicated
headers don't harm the email standard. It only complicates the tool.

> Signed-off-by: Joe Perches <joe@perches.com>
> ---
>  git-send-email.perl |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 8ce6f1f..1b9b27e 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -1151,7 +1151,8 @@ foreach my $t (@files) {
>  
>  	# set up for the next message
>  	if ($thread && $message_was_sent &&
> -		($chain_reply_to || !defined $reply_to || length($reply_to) == 0)) {
> +	    ($chain_reply_to && 
> +	     (!defined $reply_to || length($reply_to) == 0))) {
>  		$reply_to = $message_id;
>  		if (length $references > 0) {
>  			$references .= "\n $message_id";

This part of git-send-email seems to be prone for errors, blame it and
you will see in the latest commits.

You should run at least the test from the test suite for the particular
command you change. Test 49 (threading but no chain-reply-to) from t9001
now fails.

That means, this will fail now (covered by test 49):

  git format-patch -o <foo>
  git send-email --thread --no-chain-reply-to <foo>

and also this (not covered by any test, maybe we should add one):

  git format-patch -o <foo>
  git send-email --thread --chain-reply-to <foo>

Re: [PATCH 1/2] git-send-email.perl: Don't add header "In-Reply-To:" when --no-chain-reply-to set

[Joe Perches]

On Wed, 2009-07-01 at 08:18 +0200, Markus Heidelberg wrote:
> Joe Perches, 01.07.2009:
> > using
> >   git format-patch --thread=shallow -o <foo>
> > and
> >   git send-email --no-thread --no-chain-reply-to <foo>
> 
> I guess you meant --thread here.

Actually, I did mean --no-thread.
If format-patch does the threading, send-email shouldn't.

> > duplicates the headers
> > 
> >   In-Reply-To:
> >   References:
> 
> You should run at least the test from the test suite for the particular
> command you change. Test 49 (threading but no chain-reply-to) from t9001
> now fails.
> 
> That means, this will fail now (covered by test 49):
> 
>   git format-patch -o <foo>
>   git send-email --thread --no-chain-reply-to <foo>
> 
> and also this (not covered by any test, maybe we should add one):
> 
>   git format-patch -o <foo>
>   git send-email --thread --chain-reply-to <foo>

I didn't know the tests existed, thanks.
I'll investigate a bit more.

cheers, Joe

Re: [PATCH 1/2] git-send-email.perl: Don't add header "In-Reply-To:" when --no-chain-reply-to set

[Markus Heidelberg]

Joe Perches, 01.07.2009:
> On Wed, 2009-07-01 at 08:18 +0200, Markus Heidelberg wrote:
> > Joe Perches, 01.07.2009:
> > > using
> > >   git format-patch --thread=shallow -o <foo>
> > > and
> > >   git send-email --no-thread --no-chain-reply-to <foo>
> > 
> > I guess you meant --thread here.
> 
> Actually, I did mean --no-thread.

But --no-thread doesn't duplicate, --thread does.

According to the last line of your patch, you are using an old version
1.6.3.1. After this version two threading bugs were fixed, so you may
encounter a wrong behaviour with your old version.

> If format-patch does the threading, send-email shouldn't.

If the user wants format-patch to do the threading, he shouldn't want
send-email to do it as well.

Markus

Re: [PATCH 1/2] git-send-email.perl: Don't add header "In-Reply-To:" when --no-chain-reply-to set

[Joe Perches]

On Wed, 2009-07-01 at 18:59 +0200, Markus Heidelberg wrote:
> Joe Perches, 01.07.2009:
> > On Wed, 2009-07-01 at 08:18 +0200, Markus Heidelberg wrote:
> > > Joe Perches, 01.07.2009:
> > > > using
> > > >   git format-patch --thread=shallow -o <foo>
> > > > and
> > > >   git send-email --no-thread --no-chain-reply-to <foo>
> > > I guess you meant --thread here.
> > Actually, I did mean --no-thread.
> But --no-thread doesn't duplicate, --thread does.
> 
> According to the last line of your patch, you are using an old version
> 1.6.3.1. After this version two threading bugs were fixed, so you may
> encounter a wrong behaviour with your old version.

Great, I'm glad the bug was fixed already and apologies
for the noise, I don't generally track the git list.

Reading the archives, I see you're responsible, thanks.

cheers, Joe

[PATCH 2/2] format-patch: Add --cover-letter-wrap

[Joe Perches]

--cover-letter does not give control over the column wrap
position.  This adds --cover-letter-wrap with 3 arguments
position as well as indent and additional_indent.

Signed-off-by: Joe Perches <joe@perches.com>
---
 Documentation/git-format-patch.txt     |   13 +++++++++
 builtin-log.c                          |   46 +++++++++++++++++++++++++++++---
 contrib/completion/git-completion.bash |    1 +
 3 files changed, 56 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index 6f1fc80..f6b34ff 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -20,6 +20,8 @@ SYNOPSIS
 		   [--subject-prefix=Subject-Prefix]
 		   [--cc=<email>]
 		   [--cover-letter]
+		   [--cover-letter-wrap=width[,indent1[,indent2]]]
+		   [--no-cover-letter-wrap]
 		   [<common diff options>]
 		   [ <since> | <revision range> ]
 
@@ -168,6 +170,17 @@ if that is not set.
 	containing the shortlog and the overall diffstat.  You can
 	fill in a description in the file before sending it out.
 
+--cover-letter-wrap=<width>[,<indent1>[,<indent2>]]]::
+	Linewrap the cover-letter shortlog output by wrapping each line at
+	`width`.  The first line of each entry is indented by `indent1`
+	spaces, and the second and subsequent lines are indented by
+	`indent2` spaces.
+	`width`, `indent1`, and `indent2` default to 72, 2 and 4 respectively.
+
+--no-cover-letter-wrap::
+	Do not linewrap the cover-letter shortlog output.
+	indent is fixed at 6.
+
 --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 44f9a27..ec89823 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -461,6 +461,20 @@ static void add_header(const char *value)
 static int thread = 0;
 static int do_signoff = 0;
 
+struct cover_letter_style {
+	int cover_letter_wrap;
+	int cover_letter_wrappos;
+	int cover_letter_indent1;
+	int cover_letter_indent2;
+};
+
+static struct cover_letter_style cls = {
+	.cover_letter_wrap = 1,
+	.cover_letter_wrappos = 72,
+	.cover_letter_indent1 = 2,
+	.cover_letter_indent2 = 4,
+};
+
 static int git_format_config(const char *var, const char *value, void *cb)
 {
 	if (!strcmp(var, "format.headers")) {
@@ -669,10 +683,10 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 	strbuf_release(&sb);
 
 	shortlog_init(&log);
-	log.wrap_lines = 1;
-	log.wrap = 72;
-	log.in1 = 2;
-	log.in2 = 4;
+	log.wrap_lines = cls.cover_letter_wrap;
+	log.wrap = cls.cover_letter_wrappos;
+	log.in1 = cls.cover_letter_indent1;
+	log.in2 = cls.cover_letter_indent2;
 	for (i = 0; i < nr; i++)
 		shortlog_add_commit(&log, list[i]);
 
@@ -792,6 +806,27 @@ static int output_directory_callback(const struct option *opt, const char *arg,
 	return 0;
 }
 
+static int cls_callback(const struct option *opt, const char *arg, int unset)
+{
+	if (unset)
+		cls.cover_letter_wrap = 0;
+	else {
+		int i1, i2, i3;
+		if (!arg)
+			return 1;
+		int arg_count = sscanf(arg, "%d,%d,%d", &i1, &i2, &i3);
+		if (arg_count <= 0)
+			return 1;
+		if (arg_count >= 1)
+			cls.cover_letter_wrappos = i1;
+		if (arg_count >= 2)
+			cls.cover_letter_indent1 = i2;
+		if (arg_count >= 3)
+			cls.cover_letter_indent2 = i3;
+		}
+	return 0;
+}
+
 static int thread_callback(const struct option *opt, const char *arg, int unset)
 {
 	int *thread = (int *)opt->value;
@@ -875,6 +910,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 			    "print patches to standard out"),
 		OPT_BOOLEAN(0, "cover-letter", &cover_letter,
 			    "generate a cover letter"),
+		{ OPTION_CALLBACK, 0, "cover-letter-wrap", &cls, NULL,
+			    "control the cover letter format",
+			    PARSE_OPT_OPTARG, cls_callback },
 		OPT_BOOLEAN(0, "numbered-files", &numbered_files,
 			    "use simple number sequence for output file names"),
 		OPT_STRING(0, "suffix", &fmt_patch_suffix, "sfx",
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index b60cb68..aede61c 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -985,6 +985,7 @@ _git_format_patch ()
 			--full-index --binary
 			--not --all
 			--cover-letter
+			--no-cover-letter-wrap --cover-letter-wrap=
 			--no-prefix --src-prefix= --dst-prefix=
 			--inline --suffix= --ignore-if-in-upstream
 			--subject-prefix=
-- 
1.6.3.1.10.g659a0.dirty

Re: [PATCH 2/2] format-patch: Add --cover-letter-wrap

[Stephen Boyd]

Joe Perches wrote:
> @@ -792,6 +806,27 @@ static int output_directory_callback(const struct option *opt, const char *arg,
>  	return 0;
>  }
>  
> +static int cls_callback(const struct option *opt, const char *arg, int unset)
> +{
> +	if (unset)
> +		cls.cover_letter_wrap = 0;
> +	else {
> +		int i1, i2, i3;
> +		if (!arg)
> +			return 1;
> +		int arg_count = sscanf(arg, "%d,%d,%d", &i1, &i2, &i3);
> +		if (arg_count <= 0)
> +			return 1;
> +		if (arg_count >= 1)
> +			cls.cover_letter_wrappos = i1;
> +		if (arg_count >= 2)
> +			cls.cover_letter_indent1 = i2;
> +		if (arg_count >= 3)
> +			cls.cover_letter_indent2 = i3;
> +		}

This bracket is one indent off.

I'm not sure, but can this be simplified to just setting the struct
members directly through sscanf? You won't need to have these if's in
that case. I think something like --cover-letter-wrap="" would be
equivalent to just using the defaults and not an error. Does that sound
right?

> +	return 0;
> +}
> +
>  static int thread_callback(const struct option *opt, const char *arg, int unset)
>  {
>  	int *thread = (int *)opt->value;
> @@ -875,6 +910,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  			    "print patches to standard out"),
>  		OPT_BOOLEAN(0, "cover-letter", &cover_letter,
>  			    "generate a cover letter"),
> +		{ OPTION_CALLBACK, 0, "cover-letter-wrap", &cls, NULL,
> +			    "control the cover letter format",
> +			    PARSE_OPT_OPTARG, cls_callback },

Why is this PARSE_OPT_OPTARG? I only see the choice of having arguments
or prefixed with a --no. Also, please use PARSE_OPT_LITERAL_ARGHELP and
give it the help string you use in the docs
(<width>[,<indent1>[,<indent2>]]).