* [PATCH] Do not ignore merge options in interactive rebase @ 2013-06-21 5:23 Arnaud Fontaine 2013-06-21 20:43 ` Junio C Hamano 0 siblings, 1 reply; 9+ messages in thread From: Arnaud Fontaine @ 2013-06-21 5:23 UTC (permalink / raw) To: git; +Cc: Martin von Zweigbergk, Junio C Hamano Merge strategy and its options can be specified in `git rebase`, but with `--interactive`, they were completely ignored. Signed-off-by: Arnaud Fontaine <arnau@debian.org> --- git-rebase--interactive.sh | 11 ++++++++++- t/t3404-rebase-interactive.sh | 11 +++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) mode change 100644 => 100755 git-rebase--interactive.sh diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh old mode 100644 new mode 100755 index f953d8d..c157fdf --- a/git-rebase--interactive.sh +++ b/git-rebase--interactive.sh @@ -239,7 +239,16 @@ pick_one () { test -d "$rewritten" && pick_one_preserving_merges "$@" && return - output git cherry-pick $empty_args $ff "$@" + + if test -n "$do_merge" + then + test -z "$strategy" && strategy=recursive + output git cherry-pick --strategy=$strategy \ + $(echo $strategy_opts | sed "s/'--\([^']*\)'/-X\1/g") \ + $empty_args $ff "$@" + else + output git cherry-pick $empty_args $ff "$@" + fi } pick_one_preserving_merges () { 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 -- 1.8.3.GIT ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] Do not ignore merge options in interactive rebase 2013-06-21 5:23 [PATCH] Do not ignore merge options in interactive rebase Arnaud Fontaine @ 2013-06-21 20:43 ` Junio C Hamano 2013-06-24 7:40 ` Arnaud Fontaine 0 siblings, 1 reply; 9+ messages in thread From: Junio C Hamano @ 2013-06-21 20:43 UTC (permalink / raw) To: Arnaud Fontaine; +Cc: git, Martin von Zweigbergk Arnaud Fontaine <arnau@debian.org> writes: > Merge strategy and its options can be specified in `git rebase`, > but with `--interactive`, they were completely ignored. And why is it a bad thing? If you meant s/--interactive/-m/ in the above, then I can sort of understand the justification, though. > diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh > old mode 100644 > new mode 100755 I see an unjustifiable mode change here. > index f953d8d..c157fdf > --- a/git-rebase--interactive.sh > +++ b/git-rebase--interactive.sh > @@ -239,7 +239,16 @@ pick_one () { > > test -d "$rewritten" && > pick_one_preserving_merges "$@" && return > - output git cherry-pick $empty_args $ff "$@" > + > + if test -n "$do_merge" > + then So you _did_ mean "rebase -m"? > + test -z "$strategy" && strategy=recursive > + output git cherry-pick --strategy=$strategy \ This is a bad change. I would understand if the above were: git cherry-pick ${strategy+--strategy=$strategy} ... in other words, "if there is no strategy specified, do not override the configured default that might be different from recursive" (pull.twohead may be set to resolve). > + $(echo $strategy_opts | sed "s/'--\([^']*\)'/-X\1/g") \ Is it guaranteed $startegy_opts do not have a space in it? There is a call to "git merge" that uses "${strategy+-s $strategy}", but it does not seem to propagate the strategy option. Does it need a similar change? It seems that the first step might be to factor out these calls to the "git cherry-pick" and "git merge" to helper functions to make it easier to call them with -s/-X options in a consistent way. > + $empty_args $ff "$@" > + else > + output git cherry-pick $empty_args $ff "$@" > + fi It seems that there is another call to "git cherry-pick" in the script ("git grep" for it). Does it need a similar change? ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Do not ignore merge options in interactive rebase 2013-06-21 20:43 ` Junio C Hamano @ 2013-06-24 7:40 ` Arnaud Fontaine 2013-06-24 7:47 ` Arnaud Fontaine 2013-06-24 14:16 ` Junio C Hamano 0 siblings, 2 replies; 9+ messages in thread From: Arnaud Fontaine @ 2013-06-24 7:40 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Martin von Zweigbergk Junio C Hamano <gitster@pobox.com> writes: > Arnaud Fontaine <arnau@debian.org> writes: > >> Merge strategy and its options can be specified in `git rebase`, but >> with `--interactive`, they were completely ignored. > > And why is it a bad thing? If you meant s/--interactive/-m/ in the > above, then I can sort of understand the justification, though. Sorry, it was not clear. I meant that you can do 'rebase -m --strategy recursive'. But with 'rebase --interactive -m --strategy recursive', '-m --strategy recursive' is ignored. To me, this is not consistent because the behavior is different in interactive mode... Personally, I needed to specify the strategy and its options while using interactive mode and it seems I'm not the only one[0]. >> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh >> old mode 100644 >> new mode 100755 > > I see an unjustifiable mode change here. Sorry about that, I fixed it. >> index f953d8d..c157fdf >> --- a/git-rebase--interactive.sh >> +++ b/git-rebase--interactive.sh >> @@ -239,7 +239,16 @@ pick_one () { >> >> test -d "$rewritten" && >> pick_one_preserving_merges "$@" && return >> - output git cherry-pick $empty_args $ff "$@" >> + >> + if test -n "$do_merge" >> + then > > So you _did_ mean "rebase -m"? I really meant 'rebase --interactive -m'. do_merge is set to true if either '--strategy' or '-m' or '-X' is given according to git-rebase.sh. >> + test -z "$strategy" && strategy=recursive >> + output git cherry-pick --strategy=$strategy \ > > This is a bad change. > > I would understand if the above were: > > git cherry-pick ${strategy+--strategy=$strategy} ... > > in other words, "if there is no strategy specified, do not override > the configured default that might be different from recursive" > (pull.twohead may be set to resolve). Indeed, I did not know about that. I wrongly thought it was a good idea to do the same as both git-rebase (when -X is given) and git-rebase--merge which do the same test ('test -z "$strategy" && strategy=recursive'). However after checking more carefully, I guess that, for the former case, it is because only recursive currently takes options, whereas, for the latter case, it is to call a default git-rebase-$strategy. >> + $(echo $strategy_opts | sed "s/'--\([^']*\)'/-X\1/g") \ > > Is it guaranteed $startegy_opts do not have a space in it? strategy_opts may be something like (git-rebase.sh): "'--foo' '--bar'", but I'm not sure what is wrong if there is a space in it though. > There is a call to "git merge" that uses "${strategy+-s $strategy}", > but it does not seem to propagate the strategy option. Does it need a > similar change? It seems that the first step might be to factor out > these calls to the "git cherry-pick" and "git merge" to helper > functions to make it easier to call them with -s/-X options in a > consistent way. As far as I understand, yes. I changed it. As it is really short, I just added an if/else inside the script itself, not sure if that's ok... >> + $empty_args $ff "$@" >> + else >> + output git cherry-pick $empty_args $ff "$@" >> + fi > > It seems that there is another call to "git cherry-pick" in the script > ("git grep" for it). Does it need a similar change? As far as I understand, yes. So I changed it as well. I have sent the fixed patch in my next email. Many thanks for the review! Cheers, -- Arnaud Fontaine [0] http://git.661346.n2.nabble.com/git-rebase-interactive-does-not-respect-merge-options-td7500093.html ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] Do not ignore merge options in interactive rebase 2013-06-24 7:40 ` Arnaud Fontaine @ 2013-06-24 7:47 ` Arnaud Fontaine 2013-06-25 17:04 ` Junio C Hamano 2013-06-24 14:16 ` Junio C Hamano 1 sibling, 1 reply; 9+ messages in thread From: Arnaud Fontaine @ 2013-06-24 7:47 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Martin von Zweigbergk 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")" +fi + 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 -- 1.8.3.GIT ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] Do not ignore merge options in interactive rebase 2013-06-24 7:47 ` Arnaud Fontaine @ 2013-06-25 17:04 ` Junio C Hamano 2013-06-25 20:28 ` Junio C Hamano 0 siblings, 1 reply; 9+ messages in thread From: Junio C Hamano @ 2013-06-25 17:04 UTC (permalink / raw) To: Arnaud Fontaine; +Cc: git, Martin von Zweigbergk Arnaud Fontaine <arnau@debian.org> writes: > 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")" > +fi The "${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. > 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Do not ignore merge options in interactive rebase 2013-06-25 17:04 ` Junio C Hamano @ 2013-06-25 20:28 ` Junio C Hamano 2013-07-02 8:05 ` Arnaud Fontaine 2013-07-02 8:05 ` Arnaud Fontaine 0 siblings, 2 replies; 9+ messages in thread From: Junio C Hamano @ 2013-06-25 20:28 UTC (permalink / raw) To: Arnaud Fontaine; +Cc: git, Martin von Zweigbergk Junio C Hamano <gitster@pobox.com> writes: > You should use ${var:+if_set_to_nonempty_use_this} here. > ... > So munging it manually with sloppy "sed" script is simply wrong. Perhaps something like this on top of your patch squashed in? The eval magic lets the shell to split $strategy_opts back into individual words (e.g. "--subtree=My Project" is a single word), strip the leading "--", and then uses "rev-parse --sq-quote" again to turn them into "-X 'subtree=My Project'" (two words), which can be inserted into a string later to be eval'ed. git-rebase--interactive.sh | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh index e558397..ae2da7a 100644 --- a/git-rebase--interactive.sh +++ b/git-rebase--interactive.sh @@ -83,8 +83,13 @@ 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")" + strategy_args=${strategy:+--strategy=$strategy} + eval ' + for strategy_opt in '"$strategy_opts"' + do + strategy_args="$strategy_args -X$(git rev-parse --sq-quote "${strategy_opt#--}")" + done + ' fi GIT_CHERRY_PICK_HELP="$resolvemsg" @@ -246,7 +251,7 @@ pick_one () { test -d "$rewritten" && pick_one_preserving_merges "$@" && return - output git cherry-pick $strategy_args $empty_args $ff "$@" + output eval git cherry-pick "$strategy_args" $empty_args $ff "$@" } pick_one_preserving_merges () { @@ -347,9 +352,8 @@ pick_one_preserving_merges () { msg_content="$(commit_message $sha1)" # 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_args -m \ - "$msg_content" $new_parents + if ! do_with_author output eval \ + 'git merge --no-ff $strategy_args -m "$msg_content" $new_parents' then printf "%s\n" "$msg_content" > "$GIT_DIR"/MERGE_MSG die_with_patch $sha1 "Error redoing merge $sha1" @@ -357,7 +361,7 @@ pick_one_preserving_merges () { echo "$sha1 $(git rev-parse HEAD^0)" >> "$rewritten_list" ;; *) - output git cherry-pick $strategy_args "$@" || + output eval git cherry-pick "$strategy_args" "$@" || die_with_patch $sha1 "Could not pick $sha1" ;; esac ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] Do not ignore merge options in interactive rebase 2013-06-25 20:28 ` Junio C Hamano @ 2013-07-02 8:05 ` Arnaud Fontaine 2013-07-02 8:05 ` Arnaud Fontaine 1 sibling, 0 replies; 9+ messages in thread From: Arnaud Fontaine @ 2013-07-02 8:05 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Martin von Zweigbergk Hello, Sorry for the late reply. Junio C Hamano <gitster@pobox.com> writes: > Junio C Hamano <gitster@pobox.com> writes: > >> You should use ${var:+if_set_to_nonempty_use_this} here. >> ... >> So munging it manually with sloppy "sed" script is simply wrong. > > Perhaps something like this on top of your patch squashed in? > > The eval magic lets the shell to split $strategy_opts back into > individual words (e.g. "--subtree=My Project" is a single word), strip > the leading "--", and then uses "rev-parse --sq-quote" again to turn > them into "-X 'subtree=My Project'" (two words), which can be inserted > into a string later to be eval'ed. > > git-rebase--interactive.sh | 18 +++++++++++------- > 1 file changed, 11 insertions(+), 7 deletions(-) > > diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh > index e558397..ae2da7a 100644 > --- a/git-rebase--interactive.sh > +++ b/git-rebase--interactive.sh > @@ -83,8 +83,13 @@ 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")" > + strategy_args=${strategy:+--strategy=$strategy} > + eval ' > + for strategy_opt in '"$strategy_opts"' > + do > + strategy_args="$strategy_args -X$(git rev-parse --sq-quote "${strategy_opt#--}")" > + done > + ' > fi > > GIT_CHERRY_PICK_HELP="$resolvemsg" > @@ -246,7 +251,7 @@ pick_one () { > > test -d "$rewritten" && > pick_one_preserving_merges "$@" && return > - output git cherry-pick $strategy_args $empty_args $ff "$@" > + output eval git cherry-pick "$strategy_args" $empty_args $ff "$@" > } > > pick_one_preserving_merges () { > @@ -347,9 +352,8 @@ pick_one_preserving_merges () { > msg_content="$(commit_message $sha1)" > # 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_args -m \ > - "$msg_content" $new_parents > + if ! do_with_author output eval \ > + 'git merge --no-ff $strategy_args -m "$msg_content" $new_parents' > then > printf "%s\n" "$msg_content" > "$GIT_DIR"/MERGE_MSG > die_with_patch $sha1 "Error redoing merge $sha1" > @@ -357,7 +361,7 @@ pick_one_preserving_merges () { > echo "$sha1 $(git rev-parse HEAD^0)" >> "$rewritten_list" > ;; > *) > - output git cherry-pick $strategy_args "$@" || > + output eval git cherry-pick "$strategy_args" "$@" || > die_with_patch $sha1 "Could not pick $sha1" > ;; > esac Indeed, this is much better this way. Thank you very much! I have just squashed your patch into mine and ran successfully t3404 and t3421 tests after rebasing my branch on top of current master. I will send the final patch in another email. Of course, feel free to take authorship of the patch or add yourself as Signed-off-by. Cheers, -- Arnaud Fontaine ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] Do not ignore merge options in interactive rebase 2013-06-25 20:28 ` Junio C Hamano 2013-07-02 8:05 ` Arnaud Fontaine @ 2013-07-02 8:05 ` Arnaud Fontaine 1 sibling, 0 replies; 9+ messages in thread From: Arnaud Fontaine @ 2013-07-02 8:05 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Martin von Zweigbergk Merge strategy and its options can be specified in `git rebase`, but with `--interactive`, they were completely ignored. Signed-off-by: Arnaud Fontaine <arnau@debian.org> --- git-rebase--interactive.sh | 21 ++++++++++++++++----- t/t3404-rebase-interactive.sh | 11 +++++++++++ 2 files changed, 27 insertions(+), 5 deletions(-) diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh index 169e876..157690b 100644 --- a/git-rebase--interactive.sh +++ b/git-rebase--interactive.sh @@ -80,6 +80,18 @@ 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} + eval ' + for strategy_opt in '"$strategy_opts"' + do + strategy_args="$strategy_args -X$(git rev-parse --sq-quote "${strategy_opt#--}")" + done + ' +fi + GIT_CHERRY_PICK_HELP="$resolvemsg" export GIT_CHERRY_PICK_HELP @@ -239,7 +251,7 @@ pick_one () { test -d "$rewritten" && pick_one_preserving_merges "$@" && return - output git cherry-pick $empty_args $ff "$@" + output eval git cherry-pick "$strategy_args" $empty_args $ff "$@" } pick_one_preserving_merges () { @@ -340,9 +352,8 @@ pick_one_preserving_merges () { msg_content="$(commit_message $sha1)" # 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 \ - "$msg_content" $new_parents + if ! do_with_author output eval \ + 'git merge --no-ff $strategy_args -m "$msg_content" $new_parents' then printf "%s\n" "$msg_content" > "$GIT_DIR"/MERGE_MSG die_with_patch $sha1 "Error redoing merge $sha1" @@ -350,7 +361,7 @@ pick_one_preserving_merges () { echo "$sha1 $(git rev-parse HEAD^0)" >> "$rewritten_list" ;; *) - output git cherry-pick "$@" || + output eval 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 d6b4143..8a6ec03 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -950,4 +950,15 @@ test_expect_success 'rebase -i, with <onto> and <upstream> specified as :/quuxer git checkout branch1 ' +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 -- 1.8.3.GIT ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] Do not ignore merge options in interactive rebase 2013-06-24 7:40 ` Arnaud Fontaine 2013-06-24 7:47 ` Arnaud Fontaine @ 2013-06-24 14:16 ` Junio C Hamano 1 sibling, 0 replies; 9+ messages in thread From: Junio C Hamano @ 2013-06-24 14:16 UTC (permalink / raw) To: Arnaud Fontaine; +Cc: git, Martin von Zweigbergk Arnaud Fontaine <arnau@debian.org> writes: >>> + $(echo $strategy_opts | sed "s/'--\([^']*\)'/-X\1/g") \ >> >> Is it guaranteed $startegy_opts do not have a space in it? > > strategy_opts may be something like (git-rebase.sh): "'--foo' '--bar'", > but I'm not sure what is wrong if there is a space in it though. I was primarily worried about "'--frotz=nitfol xyzzy'", where you need to pass -X='frotz=nitfol xyzzy' so that 'xyzzy' part does not become a separate argument directly given to 'git merge' and friends. And adding '' around \1 is not sufficient, because the value given to the "--frotz" may have to be "nitfol 'n xyzzy". A comment next to that sed script that says "Currently there is no strategy option that needs quoting" (if it is the case; I didn't check), together with a guard to protect us against unexpected strategy-opts, might be a workable band-aid, though. ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2013-07-02 8:06 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-06-21 5:23 [PATCH] Do not ignore merge options in interactive rebase Arnaud Fontaine 2013-06-21 20:43 ` Junio C Hamano 2013-06-24 7:40 ` Arnaud Fontaine 2013-06-24 7:47 ` Arnaud Fontaine 2013-06-25 17:04 ` Junio C Hamano 2013-06-25 20:28 ` Junio C Hamano 2013-07-02 8:05 ` Arnaud Fontaine 2013-07-02 8:05 ` Arnaud Fontaine 2013-06-24 14:16 ` Junio C Hamano
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).