[RFC/PATCH] cherry-pick/revert: add support for -X/--strategy-option

[RFC/PATCH] cherry-pick/revert: add support for -X/--strategy-option

[Jonathan Nieder]

For example, this would allow cherry-picking or reverting patches from
a piece of history with a different end-of-line style, like so:

	$ git revert -Xrenormalize old-problematic-commit

Currently that is possible with manual use of merge-recursive but the
cherry-pick/revert porcelain does not expose the functionality.

While at it, document the existing support for --strategy.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Thoughts?

 Documentation/git-cherry-pick.txt  |   32 ++++++++++++++++++++++++++++++++
 Documentation/git-revert.txt       |   10 ++++++++++
 builtin/merge.c                    |    6 ++++--
 builtin/revert.c                   |   29 ++++++++++++++++++++++-------
 contrib/examples/git-revert.sh     |   13 ++++++++++++-
 merge-recursive.h                  |    4 +++-
 t/t3032-merge-recursive-options.sh |   14 ++++++++++++++
 7 files changed, 97 insertions(+), 11 deletions(-)

diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
index 7300870..749d68a 100644
--- a/Documentation/git-cherry-pick.txt
+++ b/Documentation/git-cherry-pick.txt
@@ -79,6 +79,16 @@ effect to your index in a row.
 	cherry-pick'ed commit, then a fast forward to this commit will
 	be performed.
 
+--strategy=<strategy>::
+	Use the given merge strategy.  Should only be used once.
+	See the MERGE STRATEGIES section in linkgit:git-merge[1]
+	for details.
+
+-X<option>::
+--strategy-option=<option>::
+	Pass the merge strategy-specific option through to the
+	merge strategy.  See linkgit:git-merge[1] for details.
+
 EXAMPLES
 --------
 git cherry-pick master::
@@ -120,6 +130,28 @@ git rev-list --reverse master \-- README | git cherry-pick -n --stdin::
 	so the result can be inspected and made into a single new
 	commit if suitable.
 
+The following sequence attempts to backport a patch, bails out because
+the code the patch applies to has changed too much, and then tries
+again, this time exercising more care about matching up context lines.
+
+------------
+$ git cherry-pick topic^             <1>
+$ git diff                           <2>
+$ git reset --merge ORIG_HEAD        <3>
+$ git cherry-pick -Xpatience topic^  <4>
+------------
+<1> apply the change that would be shown by `git show topic^`.
+In this example, the patch does not apply cleanly, so
+information about the conflict is written to the index and
+working tree and no new commit results.
+<2> summarize changes to be reconciled
+<3> cancel the cherry-pick.  In other words, return to the
+pre-cherry-pick state, preserving any local modifications you had in
+the working tree.
+<4> try to apply the change introduced by `topic^` again,
+spending extra time to avoid mistakes based on incorrectly matching
+context lines.
+
 Author
 ------
 Written by Junio C Hamano <gitster@pobox.com>
diff --git a/Documentation/git-revert.txt b/Documentation/git-revert.txt
index 752fc88..45be851 100644
--- a/Documentation/git-revert.txt
+++ b/Documentation/git-revert.txt
@@ -80,6 +80,16 @@ effect to your index in a row.
 --signoff::
 	Add Signed-off-by line at the end of the commit message.
 
+--strategy=<strategy>::
+	Use the given merge strategy.  Should only be used once.
+	See the MERGE STRATEGIES section in linkgit:git-merge[1]
+	for details.
+
+-X<option>::
+--strategy-option=<option>::
+	Pass the merge strategy-specific option through to the
+	merge strategy.  See linkgit:git-merge[1] for details.
+
 EXAMPLES
 --------
 git revert HEAD~3::
diff --git a/builtin/merge.c b/builtin/merge.c
index 3921cd3..c57d06e 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -582,7 +582,8 @@ static void write_tree_trivial(unsigned char *sha1)
 		die("git write-tree failed to write a tree");
 }
 
-int try_merge_command(const char *strategy, struct commit_list *common,
+int try_merge_command(const char *strategy, size_t xopts_nr,
+		      const char **xopts, struct commit_list *common,
 		      const char *head_arg, struct commit_list *remotes)
 {
 	const char **args;
@@ -680,7 +681,8 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
 		rollback_lock_file(lock);
 		return clean ? 0 : 1;
 	} else {
-		return try_merge_command(strategy, common, head_arg, remoteheads);
+		return try_merge_command(strategy, xopts_nr, xopts,
+						common, head_arg, remoteheads);
 	}
 }
 
diff --git a/builtin/revert.c b/builtin/revert.c
index bb6e9e8..dc1b702 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -44,7 +44,11 @@ static const char **commit_argv;
 static int allow_rerere_auto;
 
 static const char *me;
+
+/* Merge strategy. */
 static const char *strategy;
+static const char **xopts;
+static size_t xopts_nr, xopts_alloc;
 
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
 
@@ -55,6 +59,17 @@ static const char * const *revert_or_cherry_pick_usage(void)
 	return action == REVERT ? revert_usage : cherry_pick_usage;
 }
 
+static int option_parse_x(const struct option *opt,
+			  const char *arg, int unset)
+{
+	if (unset)
+		return 0;
+
+	ALLOC_GROW(xopts, xopts_nr + 1, xopts_alloc);
+	xopts[xopts_nr++] = xstrdup(arg);
+	return 0;
+}
+
 static void parse_args(int argc, const char **argv)
 {
 	const char * const * usage_str = revert_or_cherry_pick_usage();
@@ -67,6 +82,8 @@ static void parse_args(int argc, const char **argv)
 		OPT_INTEGER('m', "mainline", &mainline, "parent number"),
 		OPT_RERERE_AUTOUPDATE(&allow_rerere_auto),
 		OPT_STRING(0, "strategy", &strategy, "strategy", "merge strategy"),
+		OPT_CALLBACK('X', "strategy-option", &xopts, "option",
+			"option for merge strategy", option_parse_x),
 		OPT_END(),
 		OPT_END(),
 		OPT_END(),
@@ -311,18 +328,13 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
 	struct merge_options o;
 	struct tree *result, *next_tree, *base_tree, *head_tree;
 	int clean, index_fd;
+	const char **xopt;
 	static struct lock_file index_lock;
 
 	index_fd = hold_locked_index(&index_lock, 1);
 
 	read_cache();
 
-	/*
-	 * NEEDSWORK: cherry-picking between branches with
-	 * different end-of-line normalization is a pain;
-	 * plumb in an option to set o.renormalize?
-	 * (or better: arbitrary -X options)
-	 */
 	init_merge_options(&o);
 	o.ancestor = base ? base_label : "(empty tree)";
 	o.branch1 = "HEAD";
@@ -332,6 +344,9 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
 	next_tree = next ? next->tree : empty_tree();
 	base_tree = base ? base->tree : empty_tree();
 
+	for (xopt = xopts; xopt != xopts + xopts_nr; xopt++)
+		parse_merge_opt(&o, *xopt);
+
 	clean = merge_trees(&o,
 			    head_tree,
 			    next_tree, base_tree, &result);
@@ -503,7 +518,7 @@ static int do_pick_commit(void)
 
 		commit_list_insert(base, &common);
 		commit_list_insert(next, &remotes);
-		res = try_merge_command(strategy, common,
+		res = try_merge_command(strategy, xopts_nr, xopts, common,
 					sha1_to_hex(head), remotes);
 		free_commit_list(common);
 		free_commit_list(remotes);
diff --git a/contrib/examples/git-revert.sh b/contrib/examples/git-revert.sh
index 60a05a8..6bf155c 100755
--- a/contrib/examples/git-revert.sh
+++ b/contrib/examples/git-revert.sh
@@ -26,6 +26,7 @@ require_work_tree
 cd_to_toplevel
 
 no_commit=
+xopt=
 while case "$#" in 0) break ;; esac
 do
 	case "$1" in
@@ -44,6 +45,16 @@ do
 	-x|--i-really-want-to-expose-my-private-commit-object-name)
 		replay=
 		;;
+	-X?*)
+		xopt="$xopt$(git rev-parse --sq-quote "--${1#-X}")"
+		;;
+	--strategy-option=*)
+		xopt="$xopt$(git rev-parse --sq-quote "--${1#--strategy-option=}")"
+		;;
+	-X|--strategy-option)
+		shift
+		xopt="$xopt$(git rev-parse --sq-quote "--$1")"
+		;;
 	-*)
 		usage
 		;;
@@ -159,7 +170,7 @@ export GITHEAD_$head GITHEAD_$next
 # and $prev on top of us (when reverting), or the change between
 # $prev and $commit on top of us (when cherry-picking or replaying).
 
-git-merge-recursive $base -- $head $next &&
+eval "git merge-recursive $xopt $base -- $head $next" &&
 result=$(git-write-tree 2>/dev/null) || {
 	mv -f .msg "$GIT_DIR/MERGE_MSG"
 	{
diff --git a/merge-recursive.h b/merge-recursive.h
index c8135b0..981ed6a 100644
--- a/merge-recursive.h
+++ b/merge-recursive.h
@@ -57,6 +57,8 @@ struct tree *write_tree_from_memory(struct merge_options *o);
 int parse_merge_opt(struct merge_options *out, const char *s);
 
 /* builtin/merge.c */
-int try_merge_command(const char *strategy, struct commit_list *common, const char *head_arg, struct commit_list *remotes);
+int try_merge_command(const char *strategy, size_t xopts_nr,
+		const char **xopts, struct commit_list *common,
+		const char *head_arg, struct commit_list *remotes);
 
 #endif
diff --git a/t/t3032-merge-recursive-options.sh b/t/t3032-merge-recursive-options.sh
index 2293797..796f616 100755
--- a/t/t3032-merge-recursive-options.sh
+++ b/t/t3032-merge-recursive-options.sh
@@ -107,6 +107,20 @@ test_expect_success '--ignore-space-change makes merge succeed' '
 	git merge-recursive --ignore-space-change HEAD^ -- HEAD remote
 '
 
+test_expect_success 'naive cherry-pick fails' '
+	git read-tree --reset -u HEAD &&
+	test_must_fail git cherry-pick --no-commit remote &&
+	git read-tree --reset -u HEAD &&
+	test_must_fail git cherry-pick remote &&
+	test_must_fail git update-index --refresh &&
+	grep "<<<<<<" text.txt
+'
+
+test_expect_success '-Xignore-space-change makes cherry-pick succeed' '
+	git read-tree --reset -u HEAD &&
+	git cherry-pick --no-commit -Xignore-space-change remote
+'
+
 test_expect_success '--ignore-space-change: our w/s-only change wins' '
 	q_to_cr <<-\EOF >expected &&
 	    justice and holiness and is the nurse of his age and theQ
-- 
1.7.2.4

Re: [RFC/PATCH] cherry-pick/revert: add support for -X/--strategy-option

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> For example, this would allow cherry-picking or reverting patches from
> a piece of history with a different end-of-line style, like so:
>
> 	$ git revert -Xrenormalize old-problematic-commit
>
> Currently that is possible with manual use of merge-recursive but the
> cherry-pick/revert porcelain does not expose the functionality.
>
> While at it, document the existing support for --strategy.
>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
> Thoughts?

I guess this can also take "ignore whitespace", which might be a better
option for this particular use case?

Re: [RFC/PATCH] cherry-pick/revert: add support for -X/--strategy-option

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:

>> 	$ git revert -Xrenormalize old-problematic-commit
[...]
> I guess this can also take "ignore whitespace", which might be a better
> option for this particular use case?

Suppose in olden days you checked in files with \r\n line endings and
now you have switched to \n (attribute "crlf" or "text"), and in
between there was a day in which the line endings were switched.

Now you notice that old-problematic-commit is broken.  If that commit
removed lines (which had \r\n line endings), then even with
-Xignore-space-at-eol, "git revert" will add them back verbatim.  By
contrast, "git revert -Xrenormalize" would add them back in such a way
as to follow the current line ending style.

Re: [RFC/PATCH] cherry-pick/revert: add support for -X/--strategy-option

[Jonathan Nieder]

Jonathan Nieder wrote:

> For example, this would allow cherry-picking or reverting patches from
> a piece of history with a different end-of-line style, like so:
> 
> 	$ git revert -Xrenormalize old-problematic-commit
> 
> Currently that is possible with manual use of merge-recursive but the
> cherry-pick/revert porcelain does not expose the functionality.
> 
> While at it, document the existing support for --strategy.
> 
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
> Thoughts?

Ping?  I use this with -Xpatience fairly often.  Am I the only one who
has wanted such a thing?

Re: [RFC/PATCH] cherry-pick/revert: add support for -X/--strategy-option

[Martin von Zweigbergk]

On Mon, 27 Dec 2010, Jonathan Nieder wrote:

> Jonathan Nieder wrote:
> 
> > For example, this would allow cherry-picking or reverting patches from
> > a piece of history with a different end-of-line style, like so:
> > 
> > 	$ git revert -Xrenormalize old-problematic-commit
> > 
> > Currently that is possible with manual use of merge-recursive but the
> > cherry-pick/revert porcelain does not expose the functionality.
> > 
> > While at it, document the existing support for --strategy.
> > 
> > Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> > ---
> > Thoughts?
> 
> Ping?  I use this with -Xpatience fairly often.  Am I the only one who
> has wanted such a thing?

FWIW, I have wanted it for the end-of-line scenario you describe
above.

/Martin

Re: [RFC/PATCH] cherry-pick/revert: add support for -X/--strategy-option

[Justin Frankel]

I think this is a good thing -- all commands that use an underlying 
merge should support these options.

Would it make sense at some level to allow some other means of cleanly 
passing this information around? Perhaps an environment variable for 
merge options?


Jonathan Nieder wrote:
> Jonathan Nieder wrote:
> 
>> For example, this would allow cherry-picking or reverting patches from
>> a piece of history with a different end-of-line style, like so:
>>
>> 	$ git revert -Xrenormalize old-problematic-commit
>>
>> Currently that is possible with manual use of merge-recursive but the
>> cherry-pick/revert porcelain does not expose the functionality.
>>
>> While at it, document the existing support for --strategy.
>>
>> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
>> ---
>> Thoughts?
> 
> Ping?  I use this with -Xpatience fairly often.  Am I the only one who
> has wanted such a thing?

Re: [RFC/PATCH] cherry-pick/revert: add support for -X/--strategy-option

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Jonathan Nieder wrote:
>
>> For example, this would allow cherry-picking or reverting patches from
>> a piece of history with a different end-of-line style, like so:
>> 
>> 	$ git revert -Xrenormalize old-problematic-commit
>> 
>> Currently that is possible with manual use of merge-recursive but the
>> cherry-pick/revert porcelain does not expose the functionality.
>> 
>> While at it, document the existing support for --strategy.
>> 
>> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
>> ---
>> Thoughts?
>
> Ping?  I use this with -Xpatience fairly often.  Am I the only one who
> has wanted such a thing?

Pong, if you are pinging me.  I thought you wanted to only discuss it
first with RFC and meant to follow up on a real-for-inclusion patch.

The goal stated in the proposed commit log is a good thing to have, and
exactly the kind of thing -X<strategy option> interface was invented for
in the first place, I think.

I didn't look at the code though.

Re: [RFC/PATCH] cherry-pick/revert: add support for -X/--strategy-option

[Jonathan Nieder]

Junio C Hamano wrote:

> Pong, if you are pinging me.  I thought you wanted to only discuss it
> first with RFC and meant to follow up on a real-for-inclusion patch.

You thought correctly.  In this case I can't think of any change this
needs on top to make this real for inclusion so I just bumped the
thread.

There were two hints at improvements in this thread (separate topic):
the "git merge" manual could use some discussion of the distinction
between --renormalize and --ignore-whitespace-at-eol, and am, rebase,
stash, and "checkout -m <tree>" ought to have -X plumbed, too.