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

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

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

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.

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

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:

quoted

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

quoted

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

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

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

quoted

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

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:

  [off-list ref]

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

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:

quoted

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:

  [off-list ref]

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