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

Subsystems: documentation, the rest

STALE3663d

3 messages, 2 authors, 2016-08-07 · open the first message on its own page

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

From: Josh Triplett <josh@joshtriplett.org>
Date: 2016-07-30 19:11:26

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

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

From: Jeff King <hidden>
Date: 2016-08-01 18:06:56

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

From: Josh Triplett <josh@joshtriplett.org>
Date: 2016-08-07 22:57:25

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:
quoted
+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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help