[PATCH] revert.c: Allow to specify -x via git-config

[PATCH] revert.c: Allow to specify -x via git-config

[Guido Günther]

Without this when maintaining stable branches it's easy to forget to use
-x to track where a patch was cherry-picked from.

Signed-off-by: Guido Günther <agx@sigxcpu.org>
---
 Documentation/git-cherry-pick.txt |  8 ++++++++
 builtin/revert.c                  | 10 ++++++++++
 2 files changed, 18 insertions(+)

diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
index c205d23..c35064f 100644
--- a/Documentation/git-cherry-pick.txt
+++ b/Documentation/git-cherry-pick.txt
@@ -215,6 +215,14 @@ the working tree.
 spending extra time to avoid mistakes based on incorrectly matching
 context lines.
 
+CONFIGURATION
+-------------
+
+See linkgit:git-config[1] for core variables.
+
+cherrypick.record-origin::
+	Default for the `-x` option. Defaults to `false`.
+
 SEE ALSO
 --------
 linkgit:git-revert[1]
diff --git a/builtin/revert.c b/builtin/revert.c
index 87659c9..df9718f 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -196,6 +196,15 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
 	return res;
 }
 
+static int git_cherry_pick_config(const char *var, const char *value, void *cb)
+{
+	struct replay_opts *opts = cb;
+
+	if (!strcmp(var, "cherrypick.record-origin"))
+		opts->record_origin = git_config_bool (var, value);
+	return 0;
+}
+
 int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
 {
 	struct replay_opts opts;
@@ -204,6 +213,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
 	memset(&opts, 0, sizeof(opts));
 	opts.action = REPLAY_PICK;
 	git_config(git_default_config, NULL);
+	git_config(git_cherry_pick_config, &opts);
 	parse_args(argc, argv, &opts);
 	res = sequencer_pick_revisions(&opts);
 	if (res < 0)
-- 
1.9.0.rc3

Re: [PATCH] revert.c: Allow to specify -x via git-config

[John Keeping]

On Tue, Feb 18, 2014 at 07:56:20AM +0100, Guido Günther wrote:
> Without this when maintaining stable branches it's easy to forget to use
> -x to track where a patch was cherry-picked from.
> 
> Signed-off-by: Guido Günther <agx@sigxcpu.org>
> ---
>  Documentation/git-cherry-pick.txt |  8 ++++++++
>  builtin/revert.c                  | 10 ++++++++++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
> index c205d23..c35064f 100644
> --- a/Documentation/git-cherry-pick.txt
> +++ b/Documentation/git-cherry-pick.txt
> @@ -215,6 +215,14 @@ the working tree.
>  spending extra time to avoid mistakes based on incorrectly matching
>  context lines.
>  
> +CONFIGURATION
> +-------------
> +
> +See linkgit:git-config[1] for core variables.
> +
> +cherrypick.record-origin::

For consistency, this should be "cherrypick.recordOrigin", although I
wonder if we should permit "cherry-pick" here so as to keep the config
section the same as the name of the command.

I think this should also be added to git-config(1)

> +	Default for the `-x` option. Defaults to `false`.
> +
>  SEE ALSO
>  --------
>  linkgit:git-revert[1]
> diff --git a/builtin/revert.c b/builtin/revert.c
> index 87659c9..df9718f 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -196,6 +196,15 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
>  	return res;
>  }
>  
> +static int git_cherry_pick_config(const char *var, const char *value, void *cb)
> +{
> +	struct replay_opts *opts = cb;
> +
> +	if (!strcmp(var, "cherrypick.record-origin"))
> +		opts->record_origin = git_config_bool (var, value);
> +	return 0;

If you change this to:

    return git_default_config(var, value, cb);

then the code below won't end up walking the config twice.

> +}
> +
>  int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
>  {
>  	struct replay_opts opts;
> @@ -204,6 +213,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
>  	memset(&opts, 0, sizeof(opts));
>  	opts.action = REPLAY_PICK;
>  	git_config(git_default_config, NULL);
> +	git_config(git_cherry_pick_config, &opts);
>  	parse_args(argc, argv, &opts);
>  	res = sequencer_pick_revisions(&opts);
>  	if (res < 0)

Re: [PATCH] revert.c: Allow to specify -x via git-config

[Jonathan Nieder]

Hi,

Guido Günther wrote:

> Without this when maintaining stable branches it's easy to forget to use
> -x to track where a patch was cherry-picked from.
[...]
> --- a/Documentation/git-cherry-pick.txt
> +++ b/Documentation/git-cherry-pick.txt
> @@ -215,6 +215,14 @@ the working tree.
>  spending extra time to avoid mistakes based on incorrectly matching
>  context lines.
>  
> +CONFIGURATION
> +-------------
> +
> +See linkgit:git-config[1] for core variables.
> +
> +cherrypick.record-origin::
> +	Default for the `-x` option. Defaults to `false`.

I'm not convinced this is a good idea.  Even if I always want -x when
cherry-picking to stable, isn't this going to add the extra clutter
line when I cherry-pick on other branches?  It's especially worrying
because there would be no way to override the configuration with a
flag on the command line.  ("-r" which used to do that is now a
no-op.)

I would be more easily convinced by a '[branch "foo"]
recordcherrypickorigins' option that makes cherry-pick default to '-x'
when and only when on the "foo" branch.

Can you say more about the context?  Why is it important to record the
original commit id?  Is it a matter of keeping a reminder of the
commits' similarity (which cherry-pick without '-x' does ok by reusing
the same message) or are people reviewing the change downstream going
to be judging the change based on the recorded upstream commit id?
(Like linux's stable-<version> branches --- but those have other
requirements so I don't think this configuration would work as is
there.)

[...]
> +++ b/builtin/revert.c
> @@ -196,6 +196,15 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
[...]
> +	if (!strcmp(var, "cherrypick.record-origin"))
> +		opts->record_origin = git_config_bool (var, value);

More nitpicky: git uses camelCase, not dash-delimited, for multiword
configuration items.  The config parsing machinery normalizes to
lowercase, so this would then be "cherrypick.recordorigin".

Hope that helps,
Jonathan

Re: [PATCH] revert.c: Allow to specify -x via git-config

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1349 bytes --]

On Tue, Feb 18, 2014 at 09:49:13AM -0800, Jonathan Nieder wrote:
> Can you say more about the context?  Why is it important to record the
> original commit id?  Is it a matter of keeping a reminder of the
> commits' similarity (which cherry-pick without '-x' does ok by reusing
> the same message) or are people reviewing the change downstream going
> to be judging the change based on the recorded upstream commit id?
> (Like linux's stable-<version> branches --- but those have other
> requirements so I don't think this configuration would work as is
> there.)

I can provide a use case.  At work, we merge into the maintenance and
development branches and cherry-pick from the maintenance to the stable
branches.  We want committers to always use -x -s because we need to
know which reviewer backported the change and we want to be able to
track which commits have been backported and whether any reverts also
need to be cherry-picked.  We also have automated tools that want this
information.

I usually solve this with an alias (backport = cherry-pick -x -s), but I
can see how this might be a useful option.

-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] revert.c: Allow to specify -x via git-config

[Jonathan Nieder]

brian m. carlson wrote:

> I can provide a use case.  At work, we merge into the maintenance and
> development branches and cherry-pick from the maintenance to the stable
> branches.  We want committers to always use -x -s because we need to
> know which reviewer backported the change and we want to be able to
> track which commits have been backported and whether any reverts also
> need to be cherry-picked.  We also have automated tools that want this
> information.
>
> I usually solve this with an alias (backport = cherry-pick -x -s), but I
> can see how this might be a useful option.

Thanks for the example.

Here the alias seems like a nicer solution than a configuration
changing the default behavior of "git cherry-pick" because

 (1) it is easy to temporarily turn the effect off, by using cherry-pick
     directly instead of using the alias

 (2) it doesn't affect scripts that use cherry-pick

 (3) The caller explicitly specifies their intent by running "git
     backport".  It doesn't affect unrelated uses of cherry-pick on
     other branches.

 (4) it can be set up globally and takes effect when appropriate
     without needing any special setup per repository

 (5) it also implements the other part of your backporting policy (-s)
     and if in the future you had more backporting rules (e.g., requirement
     to specify which commit introduced the regression that this backport
     solves) then it could be easily tweaked

All that said, I wouldn't mind some magic that changes the default
behavior of "git cherry-pick" as long as (1), (2), and (3) are
resolved somehow.  E.g.

 (i)   a working --no-record-origin option that overrides this config

 (ii)  a --porcelain or similar option or just better documentation
       about how scripts can protect themselves from further changes
       to 'git cherry-pick' behavior

 (iii) config applying per destination branch instead of globally

Hope that helps,
Jonathan

[PATCH v2 0/3] Allow to configure cherry-pick's record origin

[Guido Günther]

The main motivation is to be able to configure repos that are used for
maintaining backports/stable branches and not having to remember to use a
special invocation of git cherry-pick.

Changes from last version:

* add --no-record-origin so scripts can make sure they'll never record
  a commit id
* add --record-origin for symmetry with config file option and
  --no-record-origin
* Add docs to git-config as well

Guido Günther (3):
  revert.c: Allow to specify -x via git-config
  revert.c: Add --record-origin
  revert.c Allow to override cherrypick.recordOrigin

 Documentation/config.txt          |  4 ++++
 Documentation/git-cherry-pick.txt | 13 +++++++++++++
 builtin/revert.c                  | 22 ++++++++++++++++++++--
 3 files changed, 37 insertions(+), 2 deletions(-)

-- 
1.9.0.rc3

[PATCH v2 1/3] revert.c: Allow to specify -x via git-config

[Guido Günther]

Without this when maintaining stable branches it's easy to forget to use
-x to track where a patch was cherry-picked from.
---
 Documentation/config.txt          |  4 ++++
 Documentation/git-cherry-pick.txt |  8 ++++++++
 builtin/revert.c                  | 14 +++++++++++++-
 3 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 5f4d793..a47d045 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -794,6 +794,10 @@ browser.<tool>.path::
 	browse HTML help (see '-w' option in linkgit:git-help[1]) or a
 	working repository in gitweb (see linkgit:git-instaweb[1]).
 
+cherrypick.recordOrigin::
+	When cherry picking patches record the sha1 of the original
+	commit in the commit message. Defaults to `false`.
+
 clean.requireForce::
 	A boolean to make git-clean do nothing unless given -f,
 	-i or -n.   Defaults to true.
diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
index c205d23..c0274bd 100644
--- a/Documentation/git-cherry-pick.txt
+++ b/Documentation/git-cherry-pick.txt
@@ -215,6 +215,14 @@ the working tree.
 spending extra time to avoid mistakes based on incorrectly matching
 context lines.
 
+CONFIGURATION
+-------------
+
+See linkgit:git-config[1] for core variables.
+
+cherrypick.recordOrigin::
+	Default for the `-x` option. Defaults to `false`.
+
 SEE ALSO
 --------
 linkgit:git-revert[1]
diff --git a/builtin/revert.c b/builtin/revert.c
index 87659c9..0c14af4 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -196,6 +196,18 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
 	return res;
 }
 
+static int git_cherry_pick_config(const char *var, const char *value, void *cb)
+{
+	struct replay_opts *opts = cb;
+
+	if (!strcmp(var, "cherrypick.recordorigin")) {
+		opts->record_origin = git_config_bool (var, value);
+		return 0;
+	} else {
+		return git_config(git_default_config, NULL);
+	}
+}
+
 int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
 {
 	struct replay_opts opts;
@@ -203,7 +215,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
 
 	memset(&opts, 0, sizeof(opts));
 	opts.action = REPLAY_PICK;
-	git_config(git_default_config, NULL);
+	git_config(git_cherry_pick_config, &opts);
 	parse_args(argc, argv, &opts);
 	res = sequencer_pick_revisions(&opts);
 	if (res < 0)
-- 
1.9.0.rc3

Re: [PATCH v2 1/3] revert.c: Allow to specify -x via git-config

[Junio C Hamano]

Guido Günther <agx@sigxcpu.org> writes:

> Without this when maintaining stable branches it's easy to forget to use
> -x to track where a patch was cherry-picked from.
> ---
>  Documentation/config.txt          |  4 ++++
>  Documentation/git-cherry-pick.txt |  8 ++++++++
>  builtin/revert.c                  | 14 +++++++++++++-
>  3 files changed, 25 insertions(+), 1 deletion(-)

Hmph.  Does this round address the issues raised in the previous
discussion in any way?

How does it affect people's existing scripts that use cherry-pick
and rely on it not doing the unwanted -x thing if such a
configuration variable is introduced as the first step in the
series, without even giving them to override the configured default
from the command line?

For that matter, I do not think a new override option from the
command line is a great solution, as that approach forces people's
existing script to be adjusted.

I personally found the way Jonathan explained why "git backport"
alias is the best solution (not just a usable workaround) very
compelling, especially his point (3):

 (3) The caller explicitly specifies their intent by running "git
     backport".  It doesn't affect unrelated uses of cherry-pick on
     other branches.

I do not even mind throwing something like this:

	#!/bin/sh
	# "git backport" - cherry-pick with -x always on.
        exec git cherry-pick -x "$@"

in contrib/ somewhere, which feels like a far more appropriate
solution to your "easy to forget" problem, at least to me.

[PATCH v2 2/3] revert.c: Add --record-origin

[Guido Günther]

This makes sure we have a command line option that corresponds with the
config file option.
---
 Documentation/git-cherry-pick.txt | 1 +
 builtin/revert.c                  | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
index c0274bd..63db07d 100644
--- a/Documentation/git-cherry-pick.txt
+++ b/Documentation/git-cherry-pick.txt
@@ -57,6 +57,7 @@ OPTIONS
 	message prior to committing.
 
 -x::
+--record-origin::
 	When recording the commit, append a line that says
 	"(cherry picked from commit ...)" to the original commit
 	message in order to indicate which commit this change was
diff --git a/builtin/revert.c b/builtin/revert.c
index 0c14af4..899f3e4 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -99,7 +99,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 
 	if (opts->action == REPLAY_PICK) {
 		struct option cp_extra[] = {
-			OPT_BOOL('x', NULL, &opts->record_origin, N_("append commit name")),
+			OPT_BOOL('x', "record-origin", &opts->record_origin, N_("append commit name")),
 			OPT_BOOL(0, "ff", &opts->allow_ff, N_("allow fast-forward")),
 			OPT_BOOL(0, "allow-empty", &opts->allow_empty, N_("preserve initially empty commits")),
 			OPT_BOOL(0, "allow-empty-message", &opts->allow_empty_message, N_("allow commits with empty messages")),
-- 
1.9.0.rc3

[PATCH v2 3/3] revert.c Allow to override cherrypick.recordOrigin

[Guido Günther]

--no-record-origin can be used by scripts to be sure to not record
origin information when cherry-picking.
---
 Documentation/git-cherry-pick.txt | 4 ++++
 builtin/revert.c                  | 6 ++++++
 2 files changed, 10 insertions(+)

diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
index 63db07d..b063b76 100644
--- a/Documentation/git-cherry-pick.txt
+++ b/Documentation/git-cherry-pick.txt
@@ -58,6 +58,7 @@ OPTIONS
 
 -x::
 --record-origin::
+--no-record-origin::
 	When recording the commit, append a line that says
 	"(cherry picked from commit ...)" to the original commit
 	message in order to indicate which commit this change was
@@ -70,6 +71,9 @@ OPTIONS
 	maintenance branch for an older release from a
 	development branch), adding this information can be
 	useful.
++
+Use `--no-record-origin` if you want to avoid recording the commit id
+even when the `cherrypick.recordOrigin` option is in effect.
 
 -r::
 	It used to be that the command defaulted to do `-x`
diff --git a/builtin/revert.c b/builtin/revert.c
index 899f3e4..14d90ba 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -76,6 +76,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 	const char * const * usage_str = revert_or_cherry_pick_usage(opts);
 	const char *me = action_name(opts);
 	int cmd = 0;
+	int no_record_origin = 0;
 	struct option options[] = {
 		OPT_CMDMODE(0, "quit", &cmd, N_("end revert or cherry-pick sequence"), 'q'),
 		OPT_CMDMODE(0, "continue", &cmd, N_("resume revert or cherry-pick sequence"), 'c'),
@@ -95,11 +96,13 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 		OPT_END(),
 		OPT_END(),
 		OPT_END(),
+		OPT_END(),
 	};
 
 	if (opts->action == REPLAY_PICK) {
 		struct option cp_extra[] = {
 			OPT_BOOL('x', "record-origin", &opts->record_origin, N_("append commit name")),
+			OPT_BOOL(0, "no-record-origin", &no_record_origin, N_("don't append commit name")),
 			OPT_BOOL(0, "ff", &opts->allow_ff, N_("allow fast-forward")),
 			OPT_BOOL(0, "allow-empty", &opts->allow_empty, N_("preserve initially empty commits")),
 			OPT_BOOL(0, "allow-empty-message", &opts->allow_empty_message, N_("allow commits with empty messages")),
@@ -118,6 +121,9 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 	if (opts->keep_redundant_commits)
 		opts->allow_empty = 1;
 
+	if (no_record_origin)
+		opts->record_origin = 0;
+
 	/* Set the subcommand */
 	if (cmd == 'q')
 		opts->subcommand = REPLAY_REMOVE_STATE;
-- 
1.9.0.rc3