threads / patch / 33411

v3, 4 partssend-email: couple of improvements

Subject: [PATCH v3 0/4] send-email: couple of improvements

## tl;dr

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

replies: 20people: 3as markdown or json

Felipe Contreras· Apr 7, 2013, 07:10 UTC · lore
Hi,

Integrated the latest feedback, and improve format-patch along the way. There should be no disadvantages with applying the latest patch.

Felipe Contreras (4):
  send-email: make annotate configurable
  format-patch: improve head calculation for cover-letter
  format-patch: refactor branch name calculation
  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                      | 75 +++++++++++++++++---------------------
 git-send-email.perl                |  7 ++--
 5 files changed, 51 insertions(+), 48 deletions(-)
-- 
1.8.2
Felipe Contreras· Apr 7, 2013, 07:10 UTC · re: Felipe Contreras · lore

[PATCH v3 1/4] 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              | 7 ++++---
 3 files changed, 8 insertions(+), 5 deletions(-)
Show changes to 3 files +8 −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..e187b12 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
@@ -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 = (
@@ -304,7 +305,7 @@ my $rc = GetOptions("h" => \$help,
 		    "smtp-debug:i" => \$debug_net_smtp,
 		    "smtp-domain:s" => \$smtp_domain,
 		    "identity=s" => \$identity,
-		    "annotate" => \$annotate,
+		    "annotate!" => \$annotate,
 		    "compose" => \$compose,
 		    "quiet" => \$quiet,
 		    "cc-cmd=s" => \$cc_cmd,

-- 
1.8.2
Junio C Hamano· Apr 7, 2013, 07:42 UTC · re: Felipe Contreras · lore

Re: [PATCH v3 1/4] send-email: make annotate configurable

Felipe Contreras <felipe.contreras@gmail.com> writes:
Show 5 quoted lines
> Some people always do --annotate, lets not force them to always type
> that.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
Sounds sensible; any tests to protect this from later breakages?
Show 65 quoted lines
>  Documentation/config.txt         | 1 +
>  Documentation/git-send-email.txt | 5 +++--
>  git-send-email.perl              | 7 ++++---
>  3 files changed, 8 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..e187b12 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
> @@ -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 = (
> @@ -304,7 +305,7 @@ my $rc = GetOptions("h" => \$help,
>  		    "smtp-debug:i" => \$debug_net_smtp,
>  		    "smtp-domain:s" => \$smtp_domain,
>  		    "identity=s" => \$identity,
> -		    "annotate" => \$annotate,
> +		    "annotate!" => \$annotate,
>  		    "compose" => \$compose,
>  		    "quiet" => \$quiet,
>  		    "cc-cmd=s" => \$cc_cmd,
Felipe Contreras· Apr 7, 2013, 16:45 UTC · re: Junio C Hamano · lore

Re: [PATCH v3 1/4] send-email: make annotate configurable

On Sun, Apr 7, 2013 at 1:42 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 9 quoted lines
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> Some people always do --annotate, lets not force them to always type
>> that.
>>
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> ---
>
> Sounds sensible; any tests to protect this from later breakages?

Given that annotate doesn't even have tests, I don't think it's needed at this point.

-- 
Felipe Contreras
Junio C Hamano· Apr 7, 2013, 17:22 UTC · re: Felipe Contreras · lore

Re: [PATCH v3 1/4] send-email: make annotate configurable

Felipe Contreras <felipe.contreras@gmail.com> writes:
Show 13 quoted lines
> On Sun, Apr 7, 2013 at 1:42 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>
>>> Some people always do --annotate, lets not force them to always type
>>> that.
>>>
>>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>>> ---
>>
>> Sounds sensible; any tests to protect this from later breakages?
>
> Given that annotate doesn't even have tests, I don't think it's needed
> at this point.

That is a good way to spread earlier mistakes and add more instance of the same kind of mistakes into the future.

It is perfectly fine to say "I ran out my Git time for the change itself; help by others to add test for it is appreciated." It is not reasonable to say "See there is the same mistake already in the code, so I am free to make the same mistake, you cannot hold it against me, and this feature should never be protected by tests".

Felipe Contreras· Apr 7, 2013, 17:58 UTC · re: Junio C Hamano · lore

Re: [PATCH v3 1/4] send-email: make annotate configurable

On Sun, Apr 7, 2013 at 12:22 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 24 quoted lines
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> On Sun, Apr 7, 2013 at 1:42 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>>
>>>> Some people always do --annotate, lets not force them to always type
>>>> that.
>>>>
>>>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>>>> ---
>>>
>>> Sounds sensible; any tests to protect this from later breakages?
>>
>> Given that annotate doesn't even have tests, I don't think it's needed
>> at this point.
>
> That is a good way to spread earlier mistakes and add more instance
> of the same kind of mistakes into the future.
>
> It is perfectly fine to say "I ran out my Git time for the change
> itself; help by others to add test for it is appreciated."  It is
> not reasonable to say "See there is the same mistake already in the
> code, so I am free to make the same mistake, you cannot hold it
> against me, and this feature should never be protected by tests".

I did not say that, I said I think this is not needed at this point, if you think otherwise, you can go ahead and implement it. I think the patch is perfectly fine as it is, and I'm not going to to add tests for something that is potentially time consuming for me, and this patch is not introducing, that is; the annotate feature. If we survived without tests for annotate, we can survive without tests for sendemail.annotate.

-- 
Felipe Contreras
Felipe Contreras· Apr 7, 2013, 07:10 UTC · re: Felipe Contreras · lore

[PATCH v3 2/4] format-patch: improve head calculation for cover-letter

If we do it after the revision traversal we can be sure that this is indeed a commit that will be processed (i.e. not a merge) and it's the top most one (thus removing the NEEDSWORK comment).

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/log.c | 20 ++------------------
 1 file changed, 2 insertions(+), 18 deletions(-)
Show changes to builtin/log.c +2 −18
diff --git a/builtin/log.c b/builtin/log.c
index 8f0b2e8..6c78d7f 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1319,24 +1319,6 @@ 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);
 	}
@@ -1372,6 +1354,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		list = xrealloc(list, nr * sizeof(list[0]));
 		list[nr - 1] = commit;
 	}
+	if (nr > 0)
+		head = list[0];
 	total = nr;
 	if (!keep_subject && auto_number && total > 1)
 		numbered = 1;

-- 
1.8.2
Junio C Hamano· Apr 7, 2013, 07:50 UTC · re: Felipe Contreras · lore

Re: [PATCH v3 2/4] format-patch: improve head calculation for cover-letter

Felipe Contreras <felipe.contreras@gmail.com> writes:
> If we do it after the revision traversal we can be sure that this is
> indeed a commit that will be processed (i.e. not a merge) and it's the
> top most one (thus removing the NEEDSWORK comment).

There may not be a single top-most one anyway (which is what that "randomly pick" comment refers to and punts), so taking the tip after traversal is just as good as doing it before. So this is a good change, but it still is punting.

The "head" is used only to produce the fake "From xxxxx <datestamp>" line to mark the output as format-patch output to /etc/magic, so it does not make sense to do anything fancier, like erroring out with "You specified two or more tips and we cannot pick one".

So it is a good thing to punt.
I like this simplification.  Good job.
Show 25 quoted lines
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  builtin/log.c | 20 ++------------------
>  1 file changed, 2 insertions(+), 18 deletions(-)
>
> diff --git a/builtin/log.c b/builtin/log.c
> index 8f0b2e8..6c78d7f 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -1319,24 +1319,6 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  	rev.show_root_diff = 1;
>  
>  	if (cover_letter) {
>  		if (!branch_name)
>  			branch_name = find_branch_name(&rev);
>  	}
> @@ -1372,6 +1354,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  		list = xrealloc(list, nr * sizeof(list[0]));
>  		list[nr - 1] = commit;
>  	}
> +	if (nr > 0)
> +		head = list[0];
>  	total = nr;
>  	if (!keep_subject && auto_number && total > 1)
>  		numbered = 1;
Felipe Contreras· Apr 7, 2013, 08:36 UTC · re: Junio C Hamano · lore

Re: [PATCH v3 2/4] format-patch: improve head calculation for cover-letter

On Sun, Apr 7, 2013 at 1:50 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 10 quoted lines
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> If we do it after the revision traversal we can be sure that this is
>> indeed a commit that will be processed (i.e. not a merge) and it's the
>> top most one (thus removing the NEEDSWORK comment).
>
> There may not be a single top-most one anyway (which is what that
> "randomly pick" comment refers to and punts), so taking the tip
> after traversal is just as good as doing it before.  So this is a
> good change, but it still is punting.

Yeah, I thought of looking at 'git diff' and do the same thing, but then I realized 'git diff --stat' has the same issue, so I don't think it really matters.

-- 
Felipe Contreras
Felipe Contreras· Apr 7, 2013, 07:10 UTC · re: Felipe Contreras · lore

[PATCH v3 3/4] format-patch: refactor branch name calculation

By moving the part that relies on rev->pending earlier, where we are already checking the special case where there's only one ref.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/log.c | 33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)
Show changes to builtin/log.c +16 −17
diff --git a/builtin/log.c b/builtin/log.c
index 6c78d7f..e4fb3c8 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1049,15 +1049,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;
 	}
@@ -1288,28 +1279,36 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	}
 
 	if (rev.pending.nr == 1) {
+		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"))
+			check_head = 1;
+
+		if (check_head) {
+			unsigned char sha1[20];
+			const char *ref;
+			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 */
+		}
 	}
 
 	/*

-- 
1.8.2
Felipe Contreras· Apr 7, 2013, 07:10 UTC · re: Felipe Contreras · lore

[PATCH v3 4/4] 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.

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

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 e4fb3c8..fab7998 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);
 }
@@ -1071,7 +1083,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;
@@ -1317,11 +1328,6 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	 */
 	rev.show_root_diff = 1;
 
-	if (cover_letter) {
-		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. */
 		if (rev.pending.nr == 2) {
@@ -1360,6 +1366,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) {
@@ -1371,6 +1379,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	if (cover_letter) {
 		if (thread)
 			gen_message_id(&rev, "cover");
+		if (!branch_name)
+			branch_name = find_branch_name(&rev);
 		make_cover_letter(&rev, use_stdout,
 				  origin, nr, list, head, branch_name, quiet);
 		total++;

-- 
1.8.2
Junio C Hamano· Apr 7, 2013, 08:14 UTC · re: Felipe Contreras · lore

Re: [PATCH v3 4/4] format-patch: add format.cover-letter configuration

Felipe Contreras <felipe.contreras@gmail.com> writes:
Show 19 quoted lines
> Also, add a new option: 'auto', so if there's more than one patch, the
> cover letter is generated, otherwise it's not.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  Documentation/config.txt           |  6 ++++++
>  Documentation/git-format-patch.txt |  5 +++--
>  builtin/log.c                      | 22 ++++++++++++++++------
>  3 files changed, 25 insertions(+), 8 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::
I thought I saw somebody already pointed out that this was wrong.
Show 5 quoted lines
> +	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).

I find this somewhat a strange phrasing. It _sets_ the default for the cover-letter option, either to true or false or auto, so there is no "in addition" about it.

Perhaps
format.coverLetter::
	Setting this variable to `true` (or `false`) tells the
	`format-patch` command to pretend the `--cover-letter` (or
	`--no-cover-letter`) was given from the command line.  When
	set to `auto`, `format-patch` assumes `--cover-letter` if
	and only if it is working on more than one commit.
or something?

It also is somewhat strange why the command line argument does not accept `--cover-letter=auto` option. In general, a configuration variable should not be more flexible mechanism than what the command line could give the user. As I already said, I think that would be a better longer-term default, and that makes it even more stand-out.

Show 18 quoted lines
> @@ -1360,6 +1366,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) {
> @@ -1371,6 +1379,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  	if (cover_letter) {
>  		if (thread)
>  			gen_message_id(&rev, "cover");
> +		if (!branch_name)
> +			branch_name = find_branch_name(&rev);
>  		make_cover_letter(&rev, use_stdout,
>  				  origin, nr, list, head, branch_name, quiet);
>  		total++;

This part is nicely done, thanks to the previous two steps. You might even want to remove the call to the find_branch_name() helper function from here, and instead call it from make_cover_letter() when branch_name is not yet there.

We would need tests to make sure future changes will not break this.
Felipe Contreras· Apr 7, 2013, 08:48 UTC · re: Junio C Hamano · lore

Re: [PATCH v3 4/4] format-patch: add format.cover-letter configuration

On Sun, Apr 7, 2013 at 3:14 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 23 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.
>>
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> ---
>>  Documentation/config.txt           |  6 ++++++
>>  Documentation/git-format-patch.txt |  5 +++--
>>  builtin/log.c                      | 22 ++++++++++++++++------
>>  3 files changed, 25 insertions(+), 8 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::
>
> I thought I saw somebody already pointed out that this was wrong.
Yeah, I forgot.
Show 9 quoted lines
>> +     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).
>
> I find this somewhat a strange phrasing.  It _sets_ the default for
> the cover-letter option, either to true or false or auto, so there
> is no "in addition" about it.
There's no --cover-letter=auto.
Show 8 quoted lines
> Perhaps
>
> format.coverLetter::
>         Setting this variable to `true` (or `false`) tells the
>         `format-patch` command to pretend the `--cover-letter` (or
>         `--no-cover-letter`) was given from the command line.  When
>         set to `auto`, `format-patch` assumes `--cover-letter` if
>         and only if it is working on more than one commit.
I prefer the wording suggested by Ramkumar with a minor modification:

A boolean that controls whether to generate a cover-letter when format-patch is invoked, but in addition can be set to "auto", to generate a cover-letter only when generating more than one patch.

Show 7 quoted lines
> or something?
>
> It also is somewhat strange why the command line argument does not
> accept `--cover-letter=auto` option.  In general, a configuration
> variable should not be more flexible mechanism than what the command
> line could give the user.  As I already said, I think that would be
> a better longer-term default, and that makes it even more stand-out.

Users know how to count, if the default is no cover-letter, then doing --cover-letter=auto is basically --cover-letter, unless they don't know how to count and thought --cover-letter=auto would do something. The same is true is they configured coverletter=true.

I don't see any value in doing that.
> This part is nicely done, thanks to the previous two steps.  You
> might even want to remove the call to the find_branch_name() helper
> function from here, and instead call it from make_cover_letter()
> when branch_name is not yet there.
True. We can also move the 'head' variable inside make_cover_letter() as well.
> We would need tests to make sure future changes will not break this.
Sigh. All right.
-- 
Felipe Contreras
Junio C Hamano· Apr 7, 2013, 09:03 UTC · re: Felipe Contreras · lore

Re: [PATCH v3 4/4] format-patch: add format.cover-letter configuration

Felipe Contreras <felipe.contreras@gmail.com> writes:
> Users know how to count, if the default is no cover-letter, then doing
> --cover-letter=auto is basically --cover-letter, unless they don't
> know how to count and thought --cover-letter=auto would do something.
> The same is true is they configured coverletter=true.

The user who says "format-patch origin" does not necessarily have to count beforehand with "rev-list origin..", so "Users know how to count" does not sound like a relevant and convincing argument to me.

Felipe Contreras· Apr 7, 2013, 09:11 UTC · re: Junio C Hamano · lore

Re: [PATCH v3 4/4] format-patch: add format.cover-letter configuration

On Sun, Apr 7, 2013 at 4:03 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 10 quoted lines
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> Users know how to count, if the default is no cover-letter, then doing
>> --cover-letter=auto is basically --cover-letter, unless they don't
>> know how to count and thought --cover-letter=auto would do something.
>> The same is true is they configured coverletter=true.
>
> The user who says "format-patch origin" does not necessarily have to
> count beforehand with "rev-list origin..", so "Users know how to
> count" does not sound like a relevant and convincing argument to me.

So, I'm a user that does 'git format-patch --cover-letter=auto origin' so I don't have the format.coverletter=auto configuration. Why? Why am I not setting that configuration, and why am I running --cover-letter=auto?

The only reason I can think of is that I ran 'git format-patch origin' and it didn't do what I wanted (or I knew befeorehand).

What other reason could the user possibly have?
-- 
Felipe Contreras
Matthieu Moy· Apr 7, 2013, 14:50 UTC · re: Felipe Contreras · lore

Re: [PATCH v3 4/4] format-patch: add format.cover-letter configuration

Felipe Contreras <felipe.contreras@gmail.com> writes:
> So, I'm a user that does 'git format-patch --cover-letter=auto origin'
> so I don't have the format.coverletter=auto configuration. Why? Why am
> I not setting that configuration, and why am I running
> --cover-letter=auto?

The command may well be ran from a script or alias. It is good in general to give the user/script writer a simple way to get predictible behavior regardless of the configuration.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Felipe Contreras· Apr 7, 2013, 16:03 UTC · re: Matthieu Moy · lore

Re: [PATCH v3 4/4] format-patch: add format.cover-letter configuration

On Sun, Apr 7, 2013 at 9:50 AM, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:

Show 8 quoted lines
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> So, I'm a user that does 'git format-patch --cover-letter=auto origin'
>> so I don't have the format.coverletter=auto configuration. Why? Why am
>> I not setting that configuration, and why am I running
>> --cover-letter=auto?
>
> The command may well be ran from a script or alias.
Wouldn't this work for both cases?
% git -c format.coverletter=auto format-patch
> It is good in
> general to give the user/script writer a simple way to get predictible
> behavior regardless of the configuration.

Predictable means when the user does X, (s)he would know what would be the outcome. But if the user never does X, it doesn't matter.

-- 
Felipe Contreras
Matthieu Moy· Apr 7, 2013, 16:20 UTC · re: Felipe Contreras · lore

Re: [PATCH v3 4/4] format-patch: add format.cover-letter configuration

Felipe Contreras <felipe.contreras@gmail.com> writes:
Show 14 quoted lines
> On Sun, Apr 7, 2013 at 9:50 AM, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>
>>> So, I'm a user that does 'git format-patch --cover-letter=auto origin'
>>> so I don't have the format.coverletter=auto configuration. Why? Why am
>>> I not setting that configuration, and why am I running
>>> --cover-letter=auto?
>>
>> The command may well be ran from a script or alias.
>
> Wouldn't this work for both cases?
>
> % git -c format.coverletter=auto format-patch
Then, what's the point in having a --cover-letter option?

"git -c" is a good last-chance solution, but when we provide the same feature as a command-line option and as a configuration option, I can see no reason to add subtle difference between them.

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

Re: [PATCH v3 4/4] format-patch: add format.cover-letter configuration

On Sun, Apr 7, 2013 at 11:20 AM, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:

Show 18 quoted lines
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> On Sun, Apr 7, 2013 at 9:50 AM, Matthieu Moy
>> <Matthieu.Moy@grenoble-inp.fr> wrote:
>>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>>
>>>> So, I'm a user that does 'git format-patch --cover-letter=auto origin'
>>>> so I don't have the format.coverletter=auto configuration. Why? Why am
>>>> I not setting that configuration, and why am I running
>>>> --cover-letter=auto?
>>>
>>> The command may well be ran from a script or alias.
>>
>> Wouldn't this work for both cases?
>>
>> % git -c format.coverletter=auto format-patch
>
> Then, what's the point in having a --cover-letter option?

It's useful in the opposite cases: when the command *not* run from a script, or an alias.

-- 
Felipe Contreras
Junio C Hamano· Apr 7, 2013, 17:22 UTC · re: Matthieu Moy · lore

Re: [PATCH v3 4/4] format-patch: add format.cover-letter configuration

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
Show 9 quoted lines
>> Wouldn't this work for both cases?
>>
>> % git -c format.coverletter=auto format-patch
>
> Then, what's the point in having a --cover-letter option?
>
> "git -c" is a good last-chance solution, but when we provide the same
> feature as a command-line option and as a configuration option, I can
> see no reason to add subtle difference between them.

I do not see a good reason to resist consistency by suggesting a workaround. We may do so ourselves when responding to an end user bug report "I cannot countermand X from the command line" as a first response "In the meantime you can work it around like so", but that is as you said a last-resort workaround and does not justify why the command line interface is lacking an obvious override.

But I think --cover-letter=auto I made was a bad suggestion. An optional parameter to a command line option is generally bad, as it makes the parsing ambiguous [*1*], and turning what traditionally was a boolean without any parameter into one that takes an optional parameter is even worse.

We should instead add this as a new feature, --auto-cover-letter, or something, i.e. the synopsis will have these as the choices

    [--[no-]cover-letter|--auto-cover-letter]

and the last one would win (e.g. "--cover-letter --auto-cover-letter" would mean "auto").

[Footnote]

*1* Does "--cover-letter yes" mean "--cover-letter=yes" aka "please use cover letter" or is it "--cover-letter" (please use cover letter) followed by an unrelated command line argument "yes"?

Felipe Contreras· Apr 7, 2013, 18:02 UTC · re: Junio C Hamano · lore

Re: [PATCH v3 4/4] format-patch: add format.cover-letter configuration

On Sun, Apr 7, 2013 at 12:22 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 14 quoted lines
> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
>>> Wouldn't this work for both cases?
>>>
>>> % git -c format.coverletter=auto format-patch
>>
>> Then, what's the point in having a --cover-letter option?
>>
>> "git -c" is a good last-chance solution, but when we provide the same
>> feature as a command-line option and as a configuration option, I can
>> see no reason to add subtle difference between them.
>
> I do not see a good reason to resist consistency by suggesting a
> workaround.
There's no need for a workaround, because there's no use-case.
Show 5 quoted lines
> We may do so ourselves when responding to an end user
> bug report "I cannot countermand X from the command line" as a first
> response "In the meantime you can work it around like so", but that
> is as you said a last-resort workaround and does not justify why the
> command line interface is lacking an obvious override.
The command line interface is not missing anything.
Show 13 quoted lines
> But I think --cover-letter=auto I made was a bad suggestion.  An
> optional parameter to a command line option is generally bad, as it
> makes the parsing ambiguous [*1*], and turning what traditionally
> was a boolean without any parameter into one that takes an optional
> parameter is even worse.
>
> We should instead add this as a new feature, --auto-cover-letter, or
> something, i.e. the synopsis will have these as the choices
>
>     [--[no-]cover-letter|--auto-cover-letter]
>
> and the last one would win (e.g. "--cover-letter --auto-cover-letter"
> would mean "auto").

Well, go ahead and implement it, because I'm not going to waste my time implementing something nobody will *ever* use.

-- 
Felipe Contreras

← back to recent threads