threads / patch / 33404

v2, 2 partssend-email: couple of improvements

Subject: [PATCH v2 0/2] send-email: couple of improvements

## tl;dr

12 messages between Apr 6, 2013 and Apr 7, 2013. Diffs are folded; open one to read it.

replies: 11people: 4as markdown or json

Felipe Contreras· Apr 6, 2013, 09:03 UTC · lore
Hi,

First patch was already sent, I just added the --no-annotate option. Second one is new, it adds a configuration for --cover-letter, so it can automatically determine when to generated: 1 patch, no cover, otherwise there is.

Felipe Contreras (2):
  send-email: make annotate configurable
  format-patch: add format.cover-letter configuration
 Documentation/config.txt           |  7 +++++
 Documentation/git-format-patch.txt |  5 ++--
 Documentation/git-send-email.txt   |  5 ++--
 builtin/log.c                      | 55 +++++++++++++++++++++++---------------
 git-send-email.perl                | 12 ++++++---
 5 files changed, 55 insertions(+), 29 deletions(-)
-- 
1.8.2
Felipe Contreras· Apr 6, 2013, 09:03 UTC · re: Felipe Contreras · lore

[PATCH v2 1/2] send-email: make annotate configurable

Some people always do --annotate, lets not force them to always type that.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Documentation/config.txt         |  1 +
 Documentation/git-send-email.txt |  5 +++--
 git-send-email.perl              | 12 +++++++++---
 3 files changed, 13 insertions(+), 5 deletions(-)
Show changes to 3 files +13 −5

Documentation/config.txt, Documentation/git-send-email.txt, git-send-email.perl

diff --git a/Documentation/config.txt b/Documentation/config.txt
index bbba728..c8e2178 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1994,6 +1994,7 @@ sendemail.<identity>.*::
 
 sendemail.aliasesfile::
 sendemail.aliasfiletype::
+sendemail.annotate::
 sendemail.bcc::
 sendemail.cc::
 sendemail.cccmd::
diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index 44a1f7c..2facc18 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -45,8 +45,9 @@ Composing
 ~~~~~~~~~
 
 --annotate::
-	Review and edit each patch you're about to send. See the
-	CONFIGURATION section for 'sendemail.multiedit'.
+	Review and edit each patch you're about to send. Default is the value
+	of 'sendemail.annotate'. See the CONFIGURATION section for
+	'sendemail.multiedit'.
 
 --bcc=<address>::
 	Specify a "Bcc:" value for each email. Default is the value of
diff --git a/git-send-email.perl b/git-send-email.perl
index be809e5..e7fe9fb 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -54,7 +54,7 @@ git send-email [options] <file | directory | rev-list options >
     --[no-]bcc              <str>  * Email Bcc:
     --subject               <str>  * Email "Subject:"
     --in-reply-to           <str>  * Email "In-Reply-To:"
-    --annotate                     * Review each patch that will be sent in an editor.
+    --[no-]annotate                * Review each patch that will be sent in an editor.
     --compose                      * Open an editor for introduction.
     --compose-encoding      <str>  * Encoding to assume for introduction.
     --8bit-encoding         <str>  * Encoding to assume 8bit mails if undeclared
@@ -143,7 +143,7 @@ my $auth;
 # Variables we fill in automatically, or via prompting:
 my (@to,$no_to,@initial_to,@cc,$no_cc,@initial_cc,@bcclist,$no_bcc,@xh,
 	$initial_reply_to,$initial_subject,@files,
-	$author,$sender,$smtp_authpass,$annotate,$compose,$time);
+	$author,$sender,$smtp_authpass,$annotate,$no_annotate,$compose,$time);
 
 my $envelope_sender;
 
@@ -212,7 +212,8 @@ my %config_bool_settings = (
     "signedoffbycc" => [\$signed_off_by_cc, undef],
     "signedoffcc" => [\$signed_off_by_cc, undef],      # Deprecated
     "validate" => [\$validate, 1],
-    "multiedit" => [\$multiedit, undef]
+    "multiedit" => [\$multiedit, undef],
+    "annotate" => [\$annotate, undef]
 );
 
 my %config_settings = (
@@ -305,6 +306,7 @@ my $rc = GetOptions("h" => \$help,
 		    "smtp-domain:s" => \$smtp_domain,
 		    "identity=s" => \$identity,
 		    "annotate" => \$annotate,
+		    "no-annotate" => \$no_annotate,
 		    "compose" => \$compose,
 		    "quiet" => \$quiet,
 		    "cc-cmd=s" => \$cc_cmd,
@@ -389,6 +391,10 @@ foreach my $setting (values %config_bool_settings) {
 	${$setting->[0]} = $setting->[1] unless (defined (${$setting->[0]}));
 }
 
+if ($no_annotate) {
+	$annotate = 0;
+}
+
 # 'default' encryption is none -- this only prevents a warning
 $smtp_encryption = '' unless (defined $smtp_encryption);
 
-- 
1.8.2
Junio C Hamano· Apr 7, 2013, 03:32 UTC · re: Felipe Contreras · lore

Re: [PATCH v2 1/2] send-email: make annotate configurable

Felipe Contreras <felipe.contreras@gmail.com> writes:
Show 5 quoted lines
> @@ -305,6 +306,7 @@ my $rc = GetOptions("h" => \$help,
>  		    "smtp-domain:s" => \$smtp_domain,
>  		    "identity=s" => \$identity,
>  		    "annotate" => \$annotate,
> +		    "no-annotate" => \$no_annotate,

Wouldn't it be much better to let GetOptions know that --no-annotate is allowed by saying

	"annotate!" => \$annotate
which also let us lose an extra $no_annotate variable?
Felipe Contreras· Apr 7, 2013, 06:54 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 1/2] send-email: make annotate configurable

On Sat, Apr 6, 2013 at 9:32 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 14 quoted lines
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> @@ -305,6 +306,7 @@ my $rc = GetOptions("h" => \$help,
>>                   "smtp-domain:s" => \$smtp_domain,
>>                   "identity=s" => \$identity,
>>                   "annotate" => \$annotate,
>> +                 "no-annotate" => \$no_annotate,
>
> Wouldn't it be much better to let GetOptions know that --no-annotate
> is allowed by saying
>
>         "annotate!" => \$annotate
>
> which also let us lose an extra $no_annotate variable?
Right. Will resend.
-- 
Felipe Contreras
Felipe Contreras· Apr 6, 2013, 09:03 UTC · re: Felipe Contreras · lore

[PATCH v2 2/2] format-patch: add format.cover-letter configuration

Also, add a new option: 'auto', so if there's more than one patch, the cover letter is generated, otherwise it's not.

This has the slight disadvantage that a piece of code will always be run even if the user doesn't want a cover letter, and thus waste a few cycles. But the convenience is well worth it.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Documentation/config.txt           |  6 +++++
 Documentation/git-format-patch.txt |  5 ++--
 builtin/log.c                      | 55 +++++++++++++++++++++++---------------
 3 files changed, 42 insertions(+), 24 deletions(-)
Show changes to 3 files +42 −23

Documentation/config.txt, Documentation/git-format-patch.txt, builtin/log.c

diff --git a/Documentation/config.txt b/Documentation/config.txt
index c8e2178..c10195c 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1092,6 +1092,12 @@ format.signoff::
     the rights to submit this work under the same open source license.
     Please see the 'SubmittingPatches' document for further discussion.
 
+format.cover-letter::
+	Allows to configure the --cover-letter option of format-patch by
+	default. In addition, you can set it to 'auto' to automatically
+	determine based on the number of patches (generate if there's more than
+	one).
+
 filter.<driver>.clean::
 	The command which is used to convert the content of a worktree
 	file to a blob upon checkin.  See linkgit:gitattributes[5] for
diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index 3a62f50..e1f5730 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -20,7 +20,7 @@ SYNOPSIS
 		   [--ignore-if-in-upstream]
 		   [--subject-prefix=Subject-Prefix] [(--reroll-count|-v) <n>]
 		   [--to=<email>] [--cc=<email>]
-		   [--cover-letter] [--quiet] [--notes[=<ref>]]
+		   [--[no-]cover-letter] [--quiet] [--notes[=<ref>]]
 		   [<common diff options>]
 		   [ <since> | <revision range> ]
 
@@ -195,7 +195,7 @@ will want to ensure that threading is disabled for `git send-email`.
 	`Cc:`, and custom) headers added so far from config or command
 	line.
 
---cover-letter::
+--[no-]cover-letter::
 	In addition to the patches, generate a cover letter file
 	containing the shortlog and the overall diffstat.  You can
 	fill in a description in the file before sending it out.
@@ -260,6 +260,7 @@ attachments, and sign off patches with configuration variables.
 	cc = <email>
 	attach [ = mime-boundary-string ]
 	signoff = true
+	cover-letter = auto
 ------------
 
 
diff --git a/builtin/log.c b/builtin/log.c
index 8f0b2e8..ed89c10 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -618,6 +618,7 @@ static void add_header(const char *value)
 #define THREAD_DEEP 2
 static int thread;
 static int do_signoff;
+static int cover_letter;
 static const char *signature = git_version_string;
 
 static int git_format_config(const char *var, const char *value, void *cb)
@@ -680,6 +681,17 @@ static int git_format_config(const char *var, const char *value, void *cb)
 	}
 	if (!strcmp(var, "format.signature"))
 		return git_config_string(&signature, var, value);
+	if (!strcmp(var, "format.cover-letter")) {
+		if (cover_letter != -1)
+			/* user overrode it */
+			return 0;
+		if (value && !strcasecmp(value, "auto")) {
+			cover_letter = -1;
+			return 0;
+		}
+		cover_letter = git_config_bool(var, value);
+		return 0;
+	}
 
 	return git_log_config(var, value, cb);
 }
@@ -1080,7 +1092,6 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	int start_number = -1;
 	int just_numbers = 0;
 	int ignore_if_in_upstream = 0;
-	int cover_letter = 0;
 	int boundary_count = 0;
 	int no_binary_diff = 0;
 	struct commit *origin = NULL, *head = NULL;
@@ -1318,28 +1329,26 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	 */
 	rev.show_root_diff = 1;
 
-	if (cover_letter) {
-		/*
-		 * NEEDSWORK:randomly pick one positive commit to show
-		 * diffstat; this is often the tip and the command
-		 * happens to do the right thing in most cases, but a
-		 * complex command like "--cover-letter a b c ^bottom"
-		 * picks "c" and shows diffstat between bottom..c
-		 * which may not match what the series represents at
-		 * all and totally broken.
-		 */
-		int i;
-		for (i = 0; i < rev.pending.nr; i++) {
-			struct object *o = rev.pending.objects[i].item;
-			if (!(o->flags & UNINTERESTING))
-				head = (struct commit *)o;
-		}
-		/* There is nothing to show; it is not an error, though. */
-		if (!head)
-			return 0;
-		if (!branch_name)
-			branch_name = find_branch_name(&rev);
+	/*
+	 * NEEDSWORK:randomly pick one positive commit to show
+	 * diffstat; this is often the tip and the command
+	 * happens to do the right thing in most cases, but a
+	 * complex command like "--cover-letter a b c ^bottom"
+	 * picks "c" and shows diffstat between bottom..c
+	 * which may not match what the series represents at
+	 * all and totally broken.
+	 */
+	for (i = 0; i < rev.pending.nr; i++) {
+		struct object *o = rev.pending.objects[i].item;
+		if (!(o->flags & UNINTERESTING))
+			head = (struct commit *)o;
 	}
+	/* There is nothing to show; it is not an error, though. */
+	if (!head)
+		return 0;
+
+	if (!branch_name)
+		branch_name = find_branch_name(&rev);
 
 	if (ignore_if_in_upstream) {
 		/* Don't say anything if head and upstream are the same. */
@@ -1377,6 +1386,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		numbered = 1;
 	if (numbered)
 		rev.total = total + start_number - 1;
+	if (cover_letter == -1)
+		cover_letter = (total > 1);
 	if (in_reply_to || thread || cover_letter)
 		rev.ref_message_ids = xcalloc(1, sizeof(struct string_list));
 	if (in_reply_to) {
-- 
1.8.2
Ramkumar Ramachandra· Apr 6, 2013, 12:43 UTC · re: Felipe Contreras · lore

Re: [PATCH v2 2/2] format-patch: add format.cover-letter configuration

Felipe Contreras wrote:
> Also, add a new option: 'auto', so if there's more than one patch, the
> cover letter is generated, otherwise it's not.

Awesome! I wanted to fix this myself, but got sidetracked with the whole submodules thing.

Show 6 quoted lines
> +format.cover-letter::
> +       Allows to configure the --cover-letter option of format-patch by
> +       default. In addition, you can set it to 'auto' to automatically
> +       determine based on the number of patches (generate if there's more than
> +       one).
> +

Perhaps you can clarify this: Controls whether to generate a cover-letter when format-patch is invoked. Can be true, false, or auto. "auto" generates a cover-letter only when generating more than one patch.

Thanks.
Felipe Contreras· Apr 6, 2013, 16:02 UTC · re: Ramkumar Ramachandra · lore

Re: [PATCH v2 2/2] format-patch: add format.cover-letter configuration

On Sat, Apr 6, 2013 at 6:43 AM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
Show 18 quoted lines
> Felipe Contreras wrote:
>> Also, add a new option: 'auto', so if there's more than one patch, the
>> cover letter is generated, otherwise it's not.
>
> Awesome!  I wanted to fix this myself, but got sidetracked with the
> whole submodules thing.
>
>> +format.cover-letter::
>> +       Allows to configure the --cover-letter option of format-patch by
>> +       default. In addition, you can set it to 'auto' to automatically
>> +       determine based on the number of patches (generate if there's more than
>> +       one).
>> +
>
> Perhaps you can clarify this: Controls whether to generate a
> cover-letter when format-patch is invoked.  Can be true, false, or
> auto.  "auto" generates a cover-letter only when generating more than
> one patch.

That's good, but I believe if we say it's a boolean, true and false are implied, and then we have an extra "auto".

-- 
Felipe Contreras
Matthieu Moy· Apr 6, 2013, 17:43 UTC · re: Felipe Contreras · lore

Re: [PATCH v2 2/2] format-patch: add format.cover-letter configuration

Felipe Contreras <felipe.contreras@gmail.com> writes:
Show 7 quoted lines
>> Perhaps you can clarify this: Controls whether to generate a
>> cover-letter when format-patch is invoked.  Can be true, false, or
>> auto.  "auto" generates a cover-letter only when generating more than
>> one patch.
>
> That's good, but I believe if we say it's a boolean, true and false
> are implied, and then we have an extra "auto".

At the moment, we don't say it's Boolean. I'm not sure it's a good idea to say Boolean here, since it would also imply that there's no value other than true and false.

Also, you should mention the default value. Something like "Set to true to always generate a cover letter. Defaults to false." or so.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Matthieu Moy· Apr 6, 2013, 17:43 UTC · re: Felipe Contreras · lore

Re: [PATCH v2 2/2] format-patch: add format.cover-letter configuration

Felipe Contreras <felipe.contreras@gmail.com> writes:
> +format.cover-letter::
We normally use camelCase, so format.coverLetter, not cover-letter.
-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Junio C Hamano· Apr 7, 2013, 03:32 UTC · re: Felipe Contreras · lore

Re: [PATCH v2 2/2] format-patch: add format.cover-letter configuration

Felipe Contreras <felipe.contreras@gmail.com> writes:
> Also, add a new option: 'auto', so if there's more than one patch, the
> cover letter is generated, otherwise it's not.
Very sensible goal.
> This has the slight disadvantage that a piece of code will always be run
> even if the user doesn't want a cover letter, and thus waste a few
> cycles.
I am not sure what overhead you are referring to.

We need to count how many we are emitting with or without the cover letter, and count is stored in "total". Even when the user said "auto" (which I personally think should become the default in the longer term, but that is a separate issue), we shouldn't have to spend any extra cost if you moved the code that does anything heavy for cover letter generation after we know what that "total" is, no?

I think the reason you did not move the "find the branch for use in the cover letter" code down was because it uses the rev->pending interface, which you cannot read out of after you count the commits, but that logic to use rev->pending predates the introduction of a more modern rev->cmdline mechanism, which is already used by the find_branch_name() function in the same codepath, and it is not clobbered by prepare_revision_walk().

So perhaps by moving that code down after we know what value "total" has, and rewriting "what was the positive commit the user gave us" logic using rev->cmdline, you do not have to do unnecessary work at all.

Felipe Contreras· Apr 7, 2013, 05:46 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 2/2] format-patch: add format.cover-letter configuration

On Sat, Apr 6, 2013 at 9:32 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 32 quoted lines
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> Also, add a new option: 'auto', so if there's more than one patch, the
>> cover letter is generated, otherwise it's not.
>
> Very sensible goal.
>
>> This has the slight disadvantage that a piece of code will always be run
>> even if the user doesn't want a cover letter, and thus waste a few
>> cycles.
>
> I am not sure what overhead you are referring to.
>
> We need to count how many we are emitting with or without the cover
> letter, and count is stored in "total".  Even when the user said
> "auto" (which I personally think should become the default in the
> longer term, but that is a separate issue), we shouldn't have to
> spend any extra cost if you moved the code that does anything heavy
> for cover letter generation after we know what that "total" is, no?
>
> I think the reason you did not move the "find the branch for use in
> the cover letter" code down was because it uses the rev->pending
> interface, which you cannot read out of after you count the commits,
> but that logic to use rev->pending predates the introduction of a
> more modern rev->cmdline mechanism, which is already used by the
> find_branch_name() function in the same codepath, and it is not
> clobbered by prepare_revision_walk().
>
> So perhaps by moving that code down after we know what value "total"
> has, and rewriting "what was the positive commit the user gave us"
> logic using rev->cmdline, you do not have to do unnecessary work at
> all.

I did try to do that, but somehow missed a few possibilities that I see now. Perhaps something like this (after this, it's possible to move the find_branch_name() code):

Show changes to builtin/log.c +31 −35
diff --git a/builtin/log.c b/builtin/log.c
index ed89c10..32add91 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1061,15 +1061,6 @@ static char *find_branch_name(struct rev_info *rev)
 	if (0 <= positive) {
 		ref = rev->cmdline.rev[positive].name;
 		tip_sha1 = rev->cmdline.rev[positive].item->sha1;
-	} else if (!rev->cmdline.nr && rev->pending.nr == 1 &&
-		   !strcmp(rev->pending.objects[0].name, "HEAD")) {
-		/*
-		 * No actual ref from command line, but "HEAD" from
-		 * rev->def was added in setup_revisions()
-		 * e.g. format-patch --cover-letter -12
-		 */
-		ref = "HEAD";
-		tip_sha1 = rev->pending.objects[0].item->sha1;
 	} else {
 		return NULL;
 	}
@@ -1299,28 +1290,41 @@ int cmd_format_patch(int argc, const char
**argv, const char *prefix)
 	}

 	if (rev.pending.nr == 1) {
+		unsigned char sha1[20];
+		const char *ref;
+		int check_head = 0;
+
 		if (rev.max_count < 0 && !rev.show_root_diff) {
 			/*
 			 * This is traditional behaviour of "git format-patch
 			 * origin" that prepares what the origin side still
 			 * does not have.
 			 */
-			unsigned char sha1[20];
-			const char *ref;
-
 			rev.pending.objects[0].item->flags |= UNINTERESTING;
 			add_head_to_pending(&rev);
-			ref = resolve_ref_unsafe("HEAD", sha1, 1, NULL);
-			if (ref && !prefixcmp(ref, "refs/heads/"))
-				branch_name = xstrdup(ref + strlen("refs/heads/"));
-			else
-				branch_name = xstrdup(""); /* no branch */
+			check_head = 1;
 		}
 		/*
 		 * Otherwise, it is "format-patch -22 HEAD", and/or
 		 * "format-patch --root HEAD".  The user wants
 		 * get_revision() to do the usual traversal.
 		 */
+
+		if (!strcmp(rev.pending.objects[0].name, "HEAD"))
+			/*
+			 * No actual ref from command line, but "HEAD" from
+			 * rev->def was added in setup_revisions()
+			 * e.g. format-patch --cover-letter -12
+			 */
+			check_head = 1;
+
+		if (check_head) {
+			ref = resolve_ref_unsafe("HEAD", sha1, 1, NULL);
+			if (ref && !prefixcmp(ref, "refs/heads/"))
+				branch_name = xstrdup(ref + strlen("refs/heads/"));
+			else
+				branch_name = xstrdup(""); /* no branch */
+		}
 	}

 	/*
@@ -1329,24 +1333,6 @@ int cmd_format_patch(int argc, const char
**argv, const char *prefix)
 	 */
 	rev.show_root_diff = 1;

-	/*
-	 * NEEDSWORK:randomly pick one positive commit to show
-	 * diffstat; this is often the tip and the command
-	 * happens to do the right thing in most cases, but a
-	 * complex command like "--cover-letter a b c ^bottom"
-	 * picks "c" and shows diffstat between bottom..c
-	 * which may not match what the series represents at
-	 * all and totally broken.
-	 */
-	for (i = 0; i < rev.pending.nr; i++) {
-		struct object *o = rev.pending.objects[i].item;
-		if (!(o->flags & UNINTERESTING))
-			head = (struct commit *)o;
-	}
-	/* There is nothing to show; it is not an error, though. */
-	if (!head)
-		return 0;
-
 	if (!branch_name)
 		branch_name = find_branch_name(&rev);

@@ -1381,6 +1367,16 @@ int cmd_format_patch(int argc, const char
**argv, const char *prefix)
 		list = xrealloc(list, nr * sizeof(list[0]));
 		list[nr - 1] = commit;
 	}
+	/*
+	 * NEEDSWORK:randomly pick one positive commit to show
+	 * diffstat; this is often the tip and the command
+	 * happens to do the right thing in most cases, but a
+	 * complex command like "--cover-letter a b c ^bottom"
+	 * picks "c" and shows diffstat between bottom..c
+	 * which may not match what the series represents at
+	 * all and totally broken.
+	 */
+	head = list[0];
 	total = nr;
 	if (!keep_subject && auto_number && total > 1)
 		numbered = 1;
-- 
Felipe Contreras
Matthieu Moy· Apr 6, 2013, 17:47 UTC · re: Felipe Contreras · lore

Re: [PATCH v2 0/2] send-email: couple of improvements

Felipe Contreras <felipe.contreras@gmail.com> writes:
> First patch was already sent, I just added the --no-annotate option. Second one
> is new, it adds a configuration for --cover-letter, so it can automatically
> determine when to generated: 1 patch, no cover, otherwise there is.

Very good. I unconditionnally run --annotate, but do that with an alias. And indeed, I use --cover-letter almost if and only if I have several patches to send. These patches do exactly what I need!

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

← back to recent threads