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

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

[Felipe Contreras]

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

[PATCH v3 1/4] 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              | 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,
-- 
1.8.2

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

[Junio C Hamano]

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?

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

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

[Felipe Contreras]

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.

-- 
Felipe Contreras

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

[Junio C Hamano]

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".

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

[Felipe Contreras]

On Sun, Apr 7, 2013 at 12:22 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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

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

[Felipe Contreras]

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

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

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

[Junio C Hamano]

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.

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

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

[Felipe Contreras]

On Sun, Apr 7, 2013 at 1:50 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 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

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

[Felipe Contreras]

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

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

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

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::
+	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

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

> +	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.

> @@ -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.

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

[Felipe Contreras]

On Sun, Apr 7, 2013 at 3:14 AM, 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.
>>
>> 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.

>> +     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.

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

> 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

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

[Junio C Hamano]

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.

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

[Felipe Contreras]

On Sun, Apr 7, 2013 at 4:03 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 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

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

[Matthieu Moy]

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/

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

[Felipe Contreras]

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

> 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

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

[Matthieu Moy]

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?

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

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

[Felipe Contreras]

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

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

[Junio C Hamano]

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.  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"?

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

[Felipe Contreras]

On Sun, Apr 7, 2013 at 12:22 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

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

> 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