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

2014-02-18 Thread 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 a...@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)
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2014-02-18 Thread 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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2014-02-18 Thread brian m. carlson
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


signature.asc
Description: Digital signature