[PATCH v2 1/2] format-patch: Add a config option format.from to set the default for --from

[PATCH v2 1/2] format-patch: Add a config option format.from to set the default for --from

[Josh Triplett]

This helps users who would prefer format-patch to default to --from, and
makes it easier to change the default in the future.

Signed-off-by: Josh Triplett <josh@joshtriplett.org>
---
 Documentation/config.txt               | 10 ++++++-
 builtin/log.c                          | 46 +++++++++++++++++++++------
 contrib/completion/git-completion.bash |  1 +-
 t/t4014-format-patch.sh                | 40 +++++++++++++++++++++++-
 4 files changed, 88 insertions(+), 9 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 8b1aee4..bd34774 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1253,6 +1253,16 @@ format.attach::
 	value as the boundary.  See the --attach option in
 	linkgit:git-format-patch[1].
 
+format.from::
+	Provides the default value for the `--from` option to format-patch.
+	Accepts a boolean value, or a name and email address.  If false,
+	format-patch defaults to `--no-from`, using commit authors directly in
+	the "From:" field of patch mails.  If true, format-patch defaults to
+	`--from`, using your committer identity in the "From:" field of patch
+	mails and including a "From:" field in the body of the patch mail if
+	different.  If set to a non-boolean value, format-patch uses that
+	value instead of your committer identity.  Defaults to false.
+
 format.numbered::
 	A boolean which can enable or disable sequence numbers in patch
 	subjects.  It defaults to "auto" which enables it only if there
diff --git a/builtin/log.c b/builtin/log.c
index fd1652f..dbd2da7 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -719,6 +719,7 @@ static void add_header(const char *value)
 static int thread;
 static int do_signoff;
 static int base_auto;
+static char *from;
 static const char *signature = git_version_string;
 static const char *signature_file;
 static int config_cover_letter;
@@ -731,6 +732,28 @@ enum {
 	COVER_AUTO
 };
 
+enum from {
+	FROM_AUTHOR,
+	FROM_USER,
+	FROM_VALUE,
+};
+
+static void set_from(enum from type, const char *value)
+{
+	free(from);
+	switch (type) {
+	case FROM_AUTHOR:
+		from = NULL;
+		break;
+	case FROM_USER:
+		from = xstrdup(git_committer_info(IDENT_NO_DATE));
+		break;
+	case FROM_VALUE:
+		from = xstrdup(value);
+		break;
+	}
+}
+
 static int git_format_config(const char *var, const char *value, void *cb)
 {
 	if (!strcmp(var, "format.headers")) {
@@ -807,6 +830,16 @@ static int git_format_config(const char *var, const char *value, void *cb)
 		base_auto = git_config_bool(var, value);
 		return 0;
 	}
+	if (!strcmp(var, "format.from")) {
+		int b = git_config_maybe_bool(var, value);
+		if (b < 0)
+			set_from(FROM_VALUE, value);
+		else if (b)
+			set_from(FROM_USER, NULL);
+		else
+			set_from(FROM_AUTHOR, NULL);
+		return 0;
+	}
 
 	return git_log_config(var, value, cb);
 }
@@ -1199,16 +1232,12 @@ static int cc_callback(const struct option *opt, const char *arg, int unset)
 
 static int from_callback(const struct option *opt, const char *arg, int unset)
 {
-	char **from = opt->value;
-
-	free(*from);
-
 	if (unset)
-		*from = NULL;
+		set_from(FROM_AUTHOR, NULL);
 	else if (arg)
-		*from = xstrdup(arg);
+		set_from(FROM_VALUE, arg);
 	else
-		*from = xstrdup(git_committer_info(IDENT_NO_DATE));
+		set_from(FROM_USER, NULL);
 	return 0;
 }
 
@@ -1384,7 +1413,6 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	int quiet = 0;
 	int reroll_count = -1;
 	char *branch_name = NULL;
-	char *from = NULL;
 	char *base_commit = NULL;
 	struct base_tree_info bases;
 
@@ -1433,7 +1461,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 			    0, to_callback },
 		{ OPTION_CALLBACK, 0, "cc", NULL, N_("email"), N_("add Cc: header"),
 			    0, cc_callback },
-		{ OPTION_CALLBACK, 0, "from", &from, N_("ident"),
+		{ OPTION_CALLBACK, 0, "from", NULL, N_("ident"),
 			    N_("set From address to <ident> (or committer ident if absent)"),
 			    PARSE_OPT_OPTARG, from_callback },
 		OPT_STRING(0, "in-reply-to", &in_reply_to, N_("message-id"),
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 10f6d52..4393033 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2181,6 +2181,7 @@ _git_config ()
 		format.attach
 		format.cc
 		format.coverLetter
+		format.from
 		format.headers
 		format.numbered
 		format.pretty
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 1206c48..b0579dd 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -229,6 +229,46 @@ check_patch () {
 	grep -e "^Subject:" "$1"
 }
 
+test_expect_success 'format.from=false' '
+
+	git -c format.from=false format-patch --stdout master..side |
+	sed -e "/^\$/q" >patch &&
+	check_patch patch &&
+	! grep "^From: C O Mitter <committer@example.com>\$" patch
+'
+
+test_expect_success 'format.from=true' '
+
+	git -c format.from=true format-patch --stdout master..side |
+	sed -e "/^\$/q" >patch &&
+	check_patch patch &&
+	grep "^From: C O Mitter <committer@example.com>\$" patch
+'
+
+test_expect_success 'format.from with address' '
+
+	git -c format.from="F R Om <from@example.com>" format-patch --stdout master..side |
+	sed -e "/^\$/q" >patch &&
+	check_patch patch &&
+	grep "^From: F R Om <from@example.com>\$" patch
+'
+
+test_expect_success '--no-from overrides format.from' '
+
+	git -c format.from="F R Om <from@example.com>" format-patch --no-from --stdout master..side |
+	sed -e "/^\$/q" >patch &&
+	check_patch patch &&
+	! grep "^From: F R Om <from@example.com>\$" patch
+'
+
+test_expect_success '--from overrides format.from' '
+
+	git -c format.from="F R Om <from@example.com>" format-patch --from --stdout master..side |
+	sed -e "/^\$/q" >patch &&
+	check_patch patch &&
+	! grep "^From: F R Om <from@example.com>\$" patch
+'
+
 test_expect_success '--no-to overrides config.to' '
 
 	git config --replace-all format.to \
-- 
git-series 0.8.7

[PATCH v2 2/2] format-patch: Default to --from

[Josh Triplett]

This avoids spoofing mails when formatting commits not written by the
user.

Add tests for the new default, and fix tests whose expected output
depended on the old default.

Signed-off-by: Josh Triplett <josh@joshtriplett.org>
---
 Documentation/config.txt |  2 +-
 builtin/log.c            |  1 +
 t/t4014-format-patch.sh  | 28 +++++++++++++++++++++++++---
 3 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index bd34774..2310877 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1261,7 +1261,7 @@ format.from::
 	`--from`, using your committer identity in the "From:" field of patch
 	mails and including a "From:" field in the body of the patch mail if
 	different.  If set to a non-boolean value, format-patch uses that
-	value instead of your committer identity.  Defaults to false.
+	value instead of your committer identity.  Defaults to true.
 
 format.numbered::
 	A boolean which can enable or disable sequence numbers in patch
diff --git a/builtin/log.c b/builtin/log.c
index dbd2da7..bcca974 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1486,6 +1486,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 
+	set_from(FROM_USER, NULL);
 	extra_hdr.strdup_strings = 1;
 	extra_to.strdup_strings = 1;
 	extra_cc.strdup_strings = 1;
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index b0579dd..fa35cbe 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -652,7 +652,7 @@ EOF
 
 test_expect_success 'format-patch -p suppresses stat' '
 
-	git format-patch -p -2 &&
+	git format-patch --no-from -p -2 &&
 	sed -e "1,/^\$/d" -e "/^+5/q" < 0001-This-is-an-excessively-long-subject-line-for-a-messa.patch > output &&
 	test_cmp expect output
 
@@ -973,7 +973,7 @@ check_author() {
 	echo content >>file &&
 	git add file &&
 	GIT_AUTHOR_NAME=$1 git commit -m author-check &&
-	git format-patch --stdout -1 >patch &&
+	git format-patch --no-from --stdout -1 >patch &&
 	sed -n "/^From: /p; /^ /p; /^$/q" <patch >actual &&
 	test_cmp expect actual
 }
@@ -1089,6 +1089,18 @@ test_expect_success '--from=ident replaces author' '
 	test_cmp expect patch.head
 '
 
+test_expect_success 'Default uses committer ident' '
+	git format-patch -1 --stdout >patch &&
+	cat >expect <<-\EOF &&
+	From: C O Mitter <committer@example.com>
+
+	From: A U Thor <author@example.com>
+
+	EOF
+	sed -ne "/^From:/p; /^$/p; /^---$/q" <patch >patch.head &&
+	test_cmp expect patch.head
+'
+
 test_expect_success '--from uses committer ident' '
 	git format-patch -1 --stdout --from >patch &&
 	cat >expect <<-\EOF &&
@@ -1101,6 +1113,16 @@ test_expect_success '--from uses committer ident' '
 	test_cmp expect patch.head
 '
 
+test_expect_success '--no-from suppresses default --from' '
+	git format-patch -1 --stdout --no-from >patch &&
+	cat >expect <<-\EOF &&
+	From: A U Thor <author@example.com>
+
+	EOF
+	sed -ne "/^From:/p; /^$/p; /^---$/q" <patch >patch.head &&
+	test_cmp expect patch.head
+'
+
 test_expect_success '--from omits redundant in-body header' '
 	git format-patch -1 --stdout --from="A U Thor <author@example.com>" >patch &&
 	cat >expect <<-\EOF &&
@@ -1129,7 +1151,7 @@ test_expect_success 'in-body headers trigger content encoding' '
 append_signoff()
 {
 	C=$(git commit-tree HEAD^^{tree} -p HEAD) &&
-	git format-patch --stdout --signoff $C^..$C >append_signoff.patch &&
+	git format-patch --no-from --stdout --signoff $C^..$C >append_signoff.patch &&
 	sed -n -e "1,/^---$/p" append_signoff.patch |
 		egrep -n "^Subject|Sign|^$"
 }
-- 
git-series 0.8.7

Re: [PATCH v2 1/2] format-patch: Add a config option format.from to set the default for --from

[Jeff King]

On Sat, Jul 30, 2016 at 12:11:11PM -0700, Josh Triplett wrote:

> +enum from {
> +	FROM_AUTHOR,
> +	FROM_USER,
> +	FROM_VALUE,
> +};
> +
> +static void set_from(enum from type, const char *value)
> +{
> +	free(from);
> +	switch (type) {
> +	case FROM_AUTHOR:
> +		from = NULL;
> +		break;
> +	case FROM_USER:
> +		from = xstrdup(git_committer_info(IDENT_NO_DATE));
> +		break;
> +	case FROM_VALUE:
> +		from = xstrdup(value);
> +		break;
> +	}
> +}

Thanks for looking into reducing the duplication. TBH, I am not sure it
is really an improvement, just because of the amount of boilerplate (and
this function interface is kind of weird, because of the rules for when
"value" should or should not be NULL).

I guess another way to do it would be:

  #define FROM_AUTO_IDENT ((const char *)(intptr_t)1))
  void set_from(const char *value)
  {
	if (value == FROM_AUTO_IDENT)
		value = git_committer_info(IDENT_NO_DATE);
	free(from);
	from = xstrdup_or_null(value);
  }

but I think the effort to polish further here is outweighing the
magnitude of the patch itself. So I offer that as "how I would have done
it" in case you like it, but again, I am fine with either this version
or the previous.

-Peff

Re: [PATCH v2 1/2] format-patch: Add a config option format.from to set the default for --from

[Junio C Hamano]

Josh Triplett <josh@joshtriplett.org> writes:

> Subject: Re: [PATCH v2 1/2] format-patch: Add a config option format.from ...

At least s/Add/add/; but I would prefer an even shorter

	format-patch: format.from gives the default for --from

> +static char *from;

The same "this does not quite help the transition" comment applies
to this one.

> +enum from {
> +	FROM_AUTHOR,
> +	FROM_USER,
> +	FROM_VALUE,

Drop trailing comma after the last enum definition (trailing comma
after the last element in an array is OK, though).

> +static void set_from(enum from type, const char *value)
> +{
> +	free(from);
> +	switch (type) {
> +	case FROM_AUTHOR:
> +		from = NULL;
> +		break;
> +	case FROM_USER:
> +		from = xstrdup(git_committer_info(IDENT_NO_DATE));
> +		break;
> +	case FROM_VALUE:
> +		from = xstrdup(value);
> +		break;
> +	}
> +}

I tend to agree with what Jeff said; I'd queue 1/2 from the original
round for now.

Thanks.

Re: [PATCH v2 1/2] format-patch: Add a config option format.from to set the default for --from

[Josh Triplett]

On Mon, Aug 01, 2016 at 01:38:47PM -0400, Jeff King wrote:
> On Sat, Jul 30, 2016 at 12:11:11PM -0700, Josh Triplett wrote:
> 
> > +enum from {
> > +	FROM_AUTHOR,
> > +	FROM_USER,
> > +	FROM_VALUE,
> > +};
> > +
> > +static void set_from(enum from type, const char *value)
> > +{
> > +	free(from);
> > +	switch (type) {
> > +	case FROM_AUTHOR:
> > +		from = NULL;
> > +		break;
> > +	case FROM_USER:
> > +		from = xstrdup(git_committer_info(IDENT_NO_DATE));
> > +		break;
> > +	case FROM_VALUE:
> > +		from = xstrdup(value);
> > +		break;
> > +	}
> > +}
> 
> Thanks for looking into reducing the duplication. TBH, I am not sure it
> is really an improvement, just because of the amount of boilerplate (and
> this function interface is kind of weird, because of the rules for when
> "value" should or should not be NULL).
> 
> I guess another way to do it would be:
> 
>   #define FROM_AUTO_IDENT ((const char *)(intptr_t)1))
>   void set_from(const char *value)
>   {
> 	if (value == FROM_AUTO_IDENT)
> 		value = git_committer_info(IDENT_NO_DATE);
> 	free(from);
> 	from = xstrdup_or_null(value);
>   }

I'd actually seriously considered this exact approach, which I preferred
as well, but I'd discarded it because I figured it'd get rejected.
Given your suggestion, and Junio's comment, I'll go with this version.

- Josh Triplett

Re: [PATCH v2 1/2] format-patch: Add a config option format.from to set the default for --from

[Junio C Hamano]

Josh Triplett <josh@joshtriplett.org> writes:

> I'd actually seriously considered this exact approach, which I preferred
> as well, but I'd discarded it because I figured it'd get rejected.
> Given your suggestion, and Junio's comment, I'll go with this version.

Sorry, but your response is soo delayed that I am not sure what you
are agreeing with and also am not sure if you are planning to reroll
what has already been happily accepted to 'next', which is not quite
welcome.

Re: [PATCH v2 1/2] format-patch: Add a config option format.from to set the default for --from

[Josh Triplett]

On Sun, Aug 07, 2016 at 06:59:09PM -0700, Junio C Hamano wrote:
> Josh Triplett <josh@joshtriplett.org> writes:
> 
> > I'd actually seriously considered this exact approach, which I preferred
> > as well, but I'd discarded it because I figured it'd get rejected.
> > Given your suggestion, and Junio's comment, I'll go with this version.
> 
> Sorry, but your response is soo delayed that I am not sure what you
> are agreeing with

I'm on vacation right now.  I was agreeing with your comment that you
didn't care for the change in v2 to use an enum.

> and also am not sure if you are planning to reroll
> what has already been happily accepted to 'next', which is not quite
> welcome.

I didn't realize you had already taken the patch series into next; I'd
assumed from the various comments that you expected me to reroll it
before you'd take it.

Would you like me to write something up for the release notes regarding
plans to change the default?

- Josh Triplett

Re: [PATCH v2 1/2] format-patch: Add a config option format.from to set the default for --from

[Josh Triplett]

On Mon, Aug 01, 2016 at 02:18:47PM -0700, Junio C Hamano wrote:
> Josh Triplett <josh@joshtriplett.org> writes:
> > +enum from {
> > +	FROM_AUTHOR,
> > +	FROM_USER,
> > +	FROM_VALUE,
> 
> Drop trailing comma after the last enum definition (trailing comma
> after the last element in an array is OK, though).

I realize this code didn't get included in the final version, but for
future reference, what's the rationale for this?  I tend to include a
final comma in cases like these (and likewise for initializers) to avoid
needing to change the last line when introducing a new element, reducing
noise in diffs.  I hadn't seen anything in any of the coding style
documentation talking about trailing commas (either pro or con).

Re: [PATCH v2 1/2] format-patch: Add a config option format.from to set the default for --from

[Jeff King]

On Sun, Aug 07, 2016 at 06:42:07PM -1000, Josh Triplett wrote:

> > Drop trailing comma after the last enum definition (trailing comma
> > after the last element in an array is OK, though).
> 
> I realize this code didn't get included in the final version, but for
> future reference, what's the rationale for this?  I tend to include a
> final comma in cases like these (and likewise for initializers) to avoid
> needing to change the last line when introducing a new element, reducing
> noise in diffs.  I hadn't seen anything in any of the coding style
> documentation talking about trailing commas (either pro or con).

Portability; some compilers choke on it. C89 allows trailing commas in
array initialization but _not_ in enums. Most compilers allow it anyway
(though gcc complains with -Wpedantic).

This definitely broke the build on real systems early in Git's history
(I think the AIX compiler was one culprit), but at this point it's
possible that all of those compilers have died off. It would be nice if
we could start using it (for exactly the reasons you give).
Unfortunately there's not a good way to know except "introduce it and
see if people complain".

-Peff

Re: [PATCH v2 1/2] format-patch: Add a config option format.from to set the default for --from

[Josh Triplett]

On Mon, Aug 08, 2016 at 12:54:41AM -0400, Jeff King wrote:
> On Sun, Aug 07, 2016 at 06:42:07PM -1000, Josh Triplett wrote:
> 
> > > Drop trailing comma after the last enum definition (trailing comma
> > > after the last element in an array is OK, though).
> > 
> > I realize this code didn't get included in the final version, but for
> > future reference, what's the rationale for this?  I tend to include a
> > final comma in cases like these (and likewise for initializers) to avoid
> > needing to change the last line when introducing a new element, reducing
> > noise in diffs.  I hadn't seen anything in any of the coding style
> > documentation talking about trailing commas (either pro or con).
> 
> Portability; some compilers choke on it. C89 allows trailing commas in
> array initialization but _not_ in enums. Most compilers allow it anyway
> (though gcc complains with -Wpedantic).
> 
> This definitely broke the build on real systems early in Git's history
> (I think the AIX compiler was one culprit),

Thanks for the explanation.  I assume such compilers also don't accept
C99?

> but at this point it's
> possible that all of those compilers have died off. It would be nice if
> we could start using it (for exactly the reasons you give).
> Unfortunately there's not a good way to know except "introduce it and
> see if people complain".

Fair enough.  I'll let someone else be the test case for that. :)

Perhaps the next Git user survey could ask "what compiler (including
version) do you use to compile Git", and perhaps "does it accept the
following code:"?

- Josh Triplett

Re: [PATCH v2 1/2] format-patch: Add a config option format.from to set the default for --from

[Jeff King]

On Sun, Aug 07, 2016 at 07:02:18PM -1000, Josh Triplett wrote:

> > Portability; some compilers choke on it. C89 allows trailing commas in
> > array initialization but _not_ in enums. Most compilers allow it anyway
> > (though gcc complains with -Wpedantic).
> > 
> > This definitely broke the build on real systems early in Git's history
> > (I think the AIX compiler was one culprit),
> 
> Thanks for the explanation.  I assume such compilers also don't accept
> C99?

Correct. We don't allow other C99 features like variadic macros, either
(there are some in the code base, but you'll note they can all be
conditionally disabled).

> Perhaps the next Git user survey could ask "what compiler (including
> version) do you use to compile Git", and perhaps "does it accept the
> following code:"?

Maybe. I'm not sure I would consider a lack of responses there to be a
definite sign. It seems that once every few years people on bizarre
systems come out of the woodwork and do a round of portability fixes,
and then problems accrue, and so on. So I'm not sure that the survey
would hit the right people in a timely manner.

I think the breaking point will be just declaring "look, C99 is N years
old; if your compiler can't handle it, that's now your problem". When
Git started, N was only 6. It's now 17.

-Peff

Re: [PATCH v2 1/2] format-patch: Add a config option format.from to set the default for --from

[Junio C Hamano]

Josh Triplett <josh@joshtriplett.org> writes:

> I didn't realize you had already taken the patch series into next; I'd
> assumed from the various comments that you expected me to reroll it
> before you'd take it.
>
> Would you like me to write something up for the release notes regarding
> plans to change the default?

Given that we are at week #8 and -rc0 is coming soon, I suspect that
that note will happen not in this release but in the next one.

The patch in question (1/2) is merely a new convenience feature that
does not have to say anything about the future default, so we are
good with 1/2 as-is (not v2 version of it, but the original one
without enum), I think.