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

[PATCH 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                          | 13 ++++++++-
 contrib/completion/git-completion.bash |  1 +-
 t/t4014-format-patch.sh                | 40 +++++++++++++++++++++++++++-
 4 files changed, 63 insertions(+), 1 deletion(-)

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..1f116be 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;
@@ -807,6 +808,17 @@ 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);
+		free(from);
+		if (b < 0)
+			from = xstrdup(value);
+		else if (b)
+			from = xstrdup(git_committer_info(IDENT_NO_DATE));
+		else
+			from = NULL;
+		return 0;
+	}
 
 	return git_log_config(var, value, cb);
 }
@@ -1384,7 +1396,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;
 
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 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 1f116be..53b2f62 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1472,6 +1472,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	extra_hdr.strdup_strings = 1;
 	extra_to.strdup_strings = 1;
 	extra_cc.strdup_strings = 1;
+	from = xstrdup(git_committer_info(IDENT_NO_DATE));
 	init_log_defaults();
 	git_config(git_format_config, NULL);
 	init_revisions(&rev, prefix);
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 1/2] format-patch: Add a config option format.from to set the default for --from

[Jeff King]

On Sat, Jul 30, 2016 at 02:41:56AM -0700, Josh Triplett wrote:

> @@ -807,6 +808,17 @@ 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);
> +		free(from);
> +		if (b < 0)
> +			from = xstrdup(value);
> +		else if (b)
> +			from = xstrdup(git_committer_info(IDENT_NO_DATE));
> +		else
> +			from = NULL;
> +		return 0;
> +	}

This "free old, then handle tri-state" mirrors the code in the parseopt
callback pretty closely. I wonder if they could share the logic (it is
not many lines, but we would want the logic to stay identical). I
suspect the helper function would end up with more boilerplate than it's
worth, though, trying to handle the unset and default cases.

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

These tests follow a different style from the "--from" tests later in
the script (and your second patch does follow it, and puts its test
close there). Any reason not to have all of the "from" tests together,
and using the same style?


Overall, the whole thing looks cleanly done, and I don't mind it going
in as-is. These are just two things I noticed while reading it over.

-Peff

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

[Josh Triplett]

On Sat, Jul 30, 2016 at 11:40:34AM -0400, Jeff King wrote:
> On Sat, Jul 30, 2016 at 02:41:56AM -0700, Josh Triplett wrote:
> 
> > @@ -807,6 +808,17 @@ 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);
> > +		free(from);
> > +		if (b < 0)
> > +			from = xstrdup(value);
> > +		else if (b)
> > +			from = xstrdup(git_committer_info(IDENT_NO_DATE));
> > +		else
> > +			from = NULL;
> > +		return 0;
> > +	}
> 
> This "free old, then handle tri-state" mirrors the code in the parseopt
> callback pretty closely. I wonder if they could share the logic (it is
> not many lines, but we would want the logic to stay identical). I
> suspect the helper function would end up with more boilerplate than it's
> worth, though, trying to handle the unset and default cases.

I looked at trying to share that code for exactly that reason, but
didn't find a convenient way to share the two, because from_callback
checked two separate variables (unset and arg), while the logic above
checks one.  So, while the *bodies* of the three-way if are duplicated,
the *conditions* aren't.

However, if you'd like to avoid the duplication between the three
values, I can do that with a set_from function that takes an enum and a
new value; it'll actually increase lines of code, but remove the
duplication (as well as the second patch's third copy of setting from to
the committer info).

> > 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
> > +'
> 
> These tests follow a different style from the "--from" tests later in
> the script (and your second patch does follow it, and puts its test
> close there). Any reason not to have all of the "from" tests together,
> and using the same style?

The tests covered different things.  The later --from tests made sure
that --from behaved as expected.  These tests made sure that format.from
and --from/--no-from interacted in the expected way, with the
command-line options overriding the configuration.  So, I put them next
to the tests for other options like format.to and format.cc, which
tested the same thing (overriding those with --no-to, --no-cc, etc).

> Overall, the whole thing looks cleanly done, and I don't mind it going
> in as-is. These are just two things I noticed while reading it over.

I'll send a v2 with the code duplication fixed.

- Josh Triplett

[PATCH v2 0/2] format-patch: Transition the default to --from to avoid spoofed mails

[Josh Triplett]

As discussed, this patch series allows transitioning the default
behavior of format-patch to --from, to avoid spoofing mails when
formatting commits not written by the user.

The first patch introduces the format.from option to set the default
value of format-patch --from.  This patch doesn't change the default
behavior of format-patch, so it can go in without any transition.

The second patch changes the default to --from.  If you'd like to delay
this patch for a release and mention the planned change in the release
notes, let me know and I'll provide text for the release notes; if you
don't think this needs a transition period, you can go ahead and apply
the second patch.

v2: Unify the various places setting from into a single helper function.

Josh Triplett (2):
  format-patch: Add a config option format.from to set the default for --from
  format-patch: Default to --from

 Documentation/config.txt               | 10 ++++-
 builtin/log.c                          | 47 +++++++++++++++----
 contrib/completion/git-completion.bash |  1 +-
 t/t4014-format-patch.sh                | 68 +++++++++++++++++++++++++--
 4 files changed, 114 insertions(+), 12 deletions(-)

-- 
git-series 0.8.7

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

[Jeff King]

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

> > These tests follow a different style from the "--from" tests later in
> > the script (and your second patch does follow it, and puts its test
> > close there). Any reason not to have all of the "from" tests together,
> > and using the same style?
> 
> The tests covered different things.  The later --from tests made sure
> that --from behaved as expected.  These tests made sure that format.from
> and --from/--no-from interacted in the expected way, with the
> command-line options overriding the configuration.  So, I put them next
> to the tests for other options like format.to and format.cc, which
> tested the same thing (overriding those with --no-to, --no-cc, etc).

OK. I would have grouped by "things that influence this area of
behavior", not by "config versus command-line". But I don't think either
is wrong or right. And since you are the one writing the patch, "how I
would have done it" is not a compelling review comment.

-Peff

Re: [PATCH v2 0/2] format-patch: Transition the default to --from to avoid spoofed mails

[Jeff King]

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

> Josh Triplett (2):
>   format-patch: Add a config option format.from to set the default for --from
>   format-patch: Default to --from

By the way, I notice that the threading between your patches and cover
letter are broken. Since I see you are also working on a tool for
handling such things, I'd suspect the tool (or your workflow) has a bug.
:)

The message-id of this message is:

  <20160730191104.2ps5k7eji7aqgufg@x>

but the patches have both "References" and "In-Reply-To" set to:

  <cover.4d006cadf197f80d899ad7d7d56d8ba41f574adf.1469905775.git-series.josh@joshtriplett.org>

I also see your MUA is mutt, and I think I saw you mention using "mutt
-H" elsewhere. IIRC, when I started using a similar workflow years ago,
I tried the same thing and had the same problem: "-H" treats the input
file as a template, not a message, and thus generates a new message-id.

I switched to using mutt's internal "resend-message" function, which
does a more literal re-send. I don't think I ever found a way to
convince mutt to do a resend from the command line.

-Peff

Re: [PATCH v2 0/2] format-patch: Transition the default to --from to avoid spoofed mails

[Josh Triplett]

On Mon, Aug 01, 2016 at 01:47:24PM -0400, Jeff King wrote:
> On Sat, Jul 30, 2016 at 12:11:05PM -0700, Josh Triplett wrote:
> 
> > Josh Triplett (2):
> >   format-patch: Add a config option format.from to set the default for --from
> >   format-patch: Default to --from
> 
> By the way, I notice that the threading between your patches and cover
> letter are broken. Since I see you are also working on a tool for
> handling such things, I'd suspect the tool (or your workflow) has a bug.
> :)

My workflow, fortunately. :)

> The message-id of this message is:
> 
>   <20160730191104.2ps5k7eji7aqgufg@x>
> 
> but the patches have both "References" and "In-Reply-To" set to:
> 
>   <cover.4d006cadf197f80d899ad7d7d56d8ba41f574adf.1469905775.git-series.josh@joshtriplett.org>
> 
> I also see your MUA is mutt, and I think I saw you mention using "mutt
> -H" elsewhere. IIRC, when I started using a similar workflow years ago,
> I tried the same thing and had the same problem: "-H" treats the input
> file as a template, not a message, and thus generates a new message-id.
> 
> I switched to using mutt's internal "resend-message" function, which
> does a more literal re-send. I don't think I ever found a way to
> convince mutt to do a resend from the command line.

I actually tried using mutt's resend function (alt-e) with an mbox; I
checked the Message-Id and In-Reply-To headers, and they looked correct.
I've used mutt -H successfully before without breaking threads.  The
Debian mutt packages recently upgraded to the "neomutt" fork; I wonder
if something broke recently?

Thanks for letting me know; I'll investigate and try to figure out the
problem.

- Josh Triplett

Re: [PATCH 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:

> +static char *from;
>  static const char *signature = git_version_string;
>  static const char *signature_file;
>  static int config_cover_letter;
> @@ -807,6 +808,17 @@ 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);
> +		free(from);
> +		if (b < 0)
> +			from = xstrdup(value);
> +		else if (b)
> +			from = xstrdup(git_committer_info(IDENT_NO_DATE));
> +		else
> +			from = NULL;
> +		return 0;
> +	}

One potential issue I see here is that if we ever plan to switch the
default, we may want to issue a warning message to users who do not
have any format.from configured when they do run the program on a
commit that will get a different result before and after the switch
in a release of Git before that default switch happens.  The message
would say something like "you are formatting somebody else's commit.
the output will change in future versions of Git and show an explicit
in-body From: header; if you want to keep the current behaviour, set
format.from configuration variable to false".

But you cannot tell by looking at from that is NULL between two
cases, it is NULL because we defaulted to it (in which case we do
want to warn), or the user set it explicitly to false (we do not
want to give the warning).

So "... makes it easier to change the default in the future." in the
log message is a bit of exaggeration.  The change in this patch is
not there yet.

> 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

I know you are only mimicking the style of the existing tests but
the piping loses a potential error exit code from format-patch.  I'd
suggest leaving this as low-hanging fruit for later, not fixing them
as part of this series.

This stops at the blank after the header, so there is no point doing
master..side to format three patches.  Just do "-1 side" instead?

More importantly, this only checks the From: in the header, which is
just the half of what --from does.  Shouldn't we be testing that the
in-body From: is added as necessary?

Thanks.

Re: [PATCH 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:32:49PM -0700, Junio C Hamano wrote:
> Josh Triplett <josh@joshtriplett.org> writes:
> 
> > +static char *from;
> >  static const char *signature = git_version_string;
> >  static const char *signature_file;
> >  static int config_cover_letter;
> > @@ -807,6 +808,17 @@ 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);
> > +		free(from);
> > +		if (b < 0)
> > +			from = xstrdup(value);
> > +		else if (b)
> > +			from = xstrdup(git_committer_info(IDENT_NO_DATE));
> > +		else
> > +			from = NULL;
> > +		return 0;
> > +	}
> 
> One potential issue I see here is that if we ever plan to switch the
> default, we may want to issue a warning message to users who do not
> have any format.from configured when they do run the program on a
> commit that will get a different result before and after the switch
> in a release of Git before that default switch happens.  The message
> would say something like "you are formatting somebody else's commit.
> the output will change in future versions of Git and show an explicit
> in-body From: header; if you want to keep the current behaviour, set
> format.from configuration variable to false".

The previous discussion between you and Jeff King seemed to suggest that
a mention in the release notes might suffice, rather than a "noisy
deprecation" warning.

> But you cannot tell by looking at from that is NULL between two
> cases, it is NULL because we defaulted to it (in which case we do
> want to warn), or the user set it explicitly to false (we do not
> want to give the warning).

If we wanted to issue such a warning, I'd suggest a separate boolean
"from_set", set when either the configuration or command line sets
"from", and then the code that handles "from" could emit a warning to
stderr if it would produce different results and !from_set.

- Josh Triplett