threads / patch / 19988

patch, 2 partsformat-patch and send-email

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

## tl;dr

8 messages between Jun 30, 2009 and Jul 2, 2009. Diffs are folded; open one to read it.

replies: 7people: 3as markdown or json

Joe Perches· Jun 30, 2009, 23:40 UTC · lore
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(-)
Joe Perches· Jun 30, 2009, 23:40 UTC · re: Joe Perches · lore

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

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(-)
Show changes to git-send-email.perl +2 −1
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
Markus Heidelberg· Jul 1, 2009, 06:18 UTC · re: Joe Perches · lore

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

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.

Show 19 quoted lines
> 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>
Joe Perches· Jul 1, 2009, 06:27 UTC · re: Markus Heidelberg · lore

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

On Wed, 2009-07-01 at 08:18 +0200, Markus Heidelberg wrote:
Show 7 quoted lines
> 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.

Show 18 quoted lines
> > 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
Markus Heidelberg· Jul 1, 2009, 16:59 UTC · re: Joe Perches · lore

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

Joe Perches, 01.07.2009:
Show 10 quoted lines
> 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
Joe Perches· Jul 1, 2009, 17:25 UTC · re: Markus Heidelberg · lore

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

On Wed, 2009-07-01 at 18:59 +0200, Markus Heidelberg wrote:
Show 14 quoted lines
> 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
Joe Perches· Jun 30, 2009, 23:40 UTC · re: Joe Perches · lore

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

--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(-)
Show changes to 3 files +56 −4

Documentation/git-format-patch.txt, builtin-log.c, contrib/completion/git-completion.bash

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
Stephen Boyd· Jul 2, 2009, 06:45 UTC · re: Joe Perches · lore

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

Joe Perches wrote:
Show 22 quoted lines
> @@ -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?

Show 13 quoted lines
> +	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>]]).

← back to recent threads