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

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

[Felipe Contreras]

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

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

[Felipe Contreras]

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(-)

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

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

[Felipe Contreras]

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(-)

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

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

[Ramkumar Ramachandra]

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.

Thanks.

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

[Felipe Contreras]

On Sat, Apr 6, 2013 at 6:43 AM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
> 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

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

[Matthieu Moy]

Felipe Contreras <felipe.contreras@gmail.com> writes:

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

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

[Matthieu Moy]

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/

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

[Matthieu Moy]

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/

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

[Junio C Hamano]

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?

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

[Junio C Hamano]

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.

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

[Felipe Contreras]

On Sat, Apr 6, 2013 at 9:32 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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):

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

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

[Felipe Contreras]

On Sat, Apr 6, 2013 at 9:32 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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