Re: [PATCH] Do not ignore merge options in interactive rebase
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jun 25, 2013, 17:04 UTC
- Message-ID
- <7v61x23yv9.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <87ppvcx84r.fsf@duckcorp.org>
Arnaud Fontaine <arnau@debian.org> writes:
Show 24 quoted lines
> Fix inconsistency where `--strategy` and/or `--strategy-option` can be
> specified in git rebase, but with `--interactive` argument only there
> were completely ignored.
>
> Signed-off-by: Arnaud Fontaine <arnau@debian.org>
> ---
> git-rebase--interactive.sh | 13 ++++++++++---
> t/t3404-rebase-interactive.sh | 11 +++++++++++
> 2 files changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index f953d8d..e558397 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -80,6 +80,13 @@ amend="$state_dir"/amend
> rewritten_list="$state_dir"/rewritten-list
> rewritten_pending="$state_dir"/rewritten-pending
>
> +strategy_args=
> +if test -n "$do_merge"
> +then
> + strategy_args="${strategy+--strategy=$strategy}
> + $(echo $strategy_opts | sed "s/'--\([^']*\)'/-X\1/g")"
> +fiThe "${var+if_set_use_this}" interpolates if $var is set (as opposed to "unset"); this will cause it to have "--strategy= " in strategy_args when $do_merge is set. If you ran t3421, you would have noticed breakage.
You should use ${var:+if_set_to_nonempty_use_this} here.stragety_opts is designed to be a valid shell scriptlet that can be inserted as a part of eval'ed string. git-rebase.sh does this:
-X) shift strategy_opts="$strategy_opts $(git rev-parse --sq-quote "--$1")" do_merge=t
so that git-rebase--merge.sh can use it like so:
eval 'git-merge-$strategy' $strategy_opts '"$cmt^" -- "$hd" "$cmt"'
without having to worry about quoting issues when future strategy options have letters that need quoting, e.g.
$ git rebase -X foo="bar ' baz"
git rev-parse --sq-quote turns it into '--foo=bar '\'' baz' and then in the eval'ed string, it becomes a single argument given to the git-merge-$strategy command, even though it may have spaces and single quotes and other characters that may be tricky to quote manually without mistakes.
So munging it manually with sloppy "sed" script is simply wrong.
Show 50 quoted lines
> GIT_CHERRY_PICK_HELP="$resolvemsg"
> export GIT_CHERRY_PICK_HELP
>
> @@ -239,7 +246,7 @@ pick_one () {
>
> test -d "$rewritten" &&
> pick_one_preserving_merges "$@" && return
> - output git cherry-pick $empty_args $ff "$@"
> + output git cherry-pick $strategy_args $empty_args $ff "$@"
> }
>
> pick_one_preserving_merges () {
> @@ -341,7 +348,7 @@ pick_one_preserving_merges () {
> # No point in merging the first parent, that's HEAD
> new_parents=${new_parents# $first_parent}
> if ! do_with_author output \
> - git merge --no-ff ${strategy:+-s $strategy} -m \
> + git merge --no-ff $strategy_args -m \
> "$msg_content" $new_parents
> then
> printf "%s\n" "$msg_content" > "$GIT_DIR"/MERGE_MSG
> @@ -350,7 +357,7 @@ pick_one_preserving_merges () {
> echo "$sha1 $(git rev-parse HEAD^0)" >> "$rewritten_list"
> ;;
> *)
> - output git cherry-pick "$@" ||
> + output git cherry-pick $strategy_args "$@" ||
> die_with_patch $sha1 "Could not pick $sha1"
> ;;
> esac
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 79e8d3c..8b6a36f 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -947,4 +947,15 @@ test_expect_success 'rebase -i respects core.commentchar' '
> test B = $(git cat-file commit HEAD^ | sed -ne \$p)
> '
>
> +test_expect_success 'rebase -i with --strategy and -X' '
> + git checkout -b conflict-merge-use-theirs conflict-branch &&
> + git reset --hard HEAD^ &&
> + echo five >conflict &&
> + echo Z >file1 &&
> + git commit -a -m "one file conflict" &&
> + EDITOR=true git rebase -i --strategy=recursive -Xours conflict-branch &&
> + test $(git show conflict-branch:conflict) = $(cat conflict) &&
> + test $(cat file1) = Z
> +'
> +
> test_done