{"thread":{"id":"34237","subject":"[PATCH] Do not ignore merge options in interactive rebase","startedAt":"2013-06-21T05:23:48Z","lastAt":"2013-07-02T08:05:48Z","messageCount":9,"participants":["Arnaud Fontaine","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"221556","messageId":"87bo70dokb.fsf@duckcorp.org","threadId":"34237","inReplyTo":null,"subject":"[PATCH] Do not ignore merge options in interactive rebase","fromName":"Arnaud Fontaine","fromEmail":"arnau@debian.org","sentAt":"2013-06-21T05:23:48Z","receivedAt":"2013-06-21T05:23:48Z","isPatch":true,"sender":{"key":"arnau@debian.org","avatar":null},"body":"Merge strategy and its options can be specified in `git rebase`,\nbut with `--interactive`, they were completely ignored.\n\nSigned-off-by: Arnaud Fontaine <arnau@debian.org>\n---\n git-rebase--interactive.sh    | 11 ++++++++++-\n t/t3404-rebase-interactive.sh | 11 +++++++++++\n 2 files changed, 21 insertions(+), 1 deletion(-)\n mode change 100644 => 100755 git-rebase--interactive.sh\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nold mode 100644\nnew mode 100755\nindex f953d8d..c157fdf\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -239,7 +239,16 @@ pick_one () {\n \n \ttest -d \"$rewritten\" &&\n \t\tpick_one_preserving_merges \"$@\" && return\n-\toutput git cherry-pick $empty_args $ff \"$@\"\n+\n+\tif test -n \"$do_merge\"\n+\tthen\n+\t\ttest -z \"$strategy\" && strategy=recursive\n+\t\toutput git cherry-pick --strategy=$strategy \\\n+\t\t\t$(echo $strategy_opts | sed \"s/'--\\([^']*\\)'/-X\\1/g\") \\\n+\t\t\t$empty_args $ff \"$@\"\n+\telse\n+\t\toutput git cherry-pick $empty_args $ff \"$@\"\n+\tfi\n }\n \n pick_one_preserving_merges () {\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex 79e8d3c..8b6a36f 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -947,4 +947,15 @@ test_expect_success 'rebase -i respects core.commentchar' '\n \ttest B = $(git cat-file commit HEAD^ | sed -ne \\$p)\n '\n \n+test_expect_success 'rebase -i with --strategy and -X' '\n+\tgit checkout -b conflict-merge-use-theirs conflict-branch &&\n+\tgit reset --hard HEAD^ &&\n+\techo five >conflict &&\n+\techo Z >file1 &&\n+\tgit commit -a -m \"one file conflict\" &&\n+\tEDITOR=true git rebase -i --strategy=recursive -Xours conflict-branch &&\n+\ttest $(git show conflict-branch:conflict) = $(cat conflict) &&\n+\ttest $(cat file1) = Z\n+'\n+\n test_done\n-- \n1.8.3.GIT\n"},{"id":"221638","messageId":"7vr4fvkxew.fsf@alter.siamese.dyndns.org","threadId":"34237","inReplyTo":"87bo70dokb.fsf@duckcorp.org","subject":"Re: [PATCH] Do not ignore merge options in interactive rebase","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-21T20:43:03Z","receivedAt":"2013-06-21T20:43:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Arnaud Fontaine <arnau@debian.org> writes:\n\n> Merge strategy and its options can be specified in `git rebase`,\n> but with `--interactive`, they were completely ignored.\n\nAnd why is it a bad thing?  If you meant s/--interactive/-m/ in the\nabove, then I can sort of understand the justification, though.\n\n\n> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\n> old mode 100644\n> new mode 100755\n\nI see an unjustifiable mode change here.\n\n> index f953d8d..c157fdf\n> --- a/git-rebase--interactive.sh\n> +++ b/git-rebase--interactive.sh\n> @@ -239,7 +239,16 @@ pick_one () {\n>  \n>  \ttest -d \"$rewritten\" &&\n>  \t\tpick_one_preserving_merges \"$@\" && return\n> -\toutput git cherry-pick $empty_args $ff \"$@\"\n> +\n> +\tif test -n \"$do_merge\"\n> +\tthen\n\nSo you _did_ mean \"rebase -m\"?\n\n> +\t\ttest -z \"$strategy\" && strategy=recursive\n> +\t\toutput git cherry-pick --strategy=$strategy \\\n\nThis is a bad change.\n\nI would understand if the above were:\n\n\tgit cherry-pick ${strategy+--strategy=$strategy} ...\n\nin other words, \"if there is no strategy specified, do not override\nthe configured default that might be different from recursive\"\n(pull.twohead may be set to resolve).\n\n> +\t\t\t$(echo $strategy_opts | sed \"s/'--\\([^']*\\)'/-X\\1/g\") \\\n\nIs it guaranteed $startegy_opts do not have a space in it?\n\nThere is a call to \"git merge\" that uses \"${strategy+-s $strategy}\",\nbut it does not seem to propagate the strategy option.  Does it need\na similar change?  It seems that the first step might be to factor\nout these calls to the \"git cherry-pick\" and \"git merge\" to helper\nfunctions to make it easier to call them with -s/-X options in a\nconsistent way.\n\n> +\t\t\t$empty_args $ff \"$@\"\n> +\telse\n> +\t\toutput git cherry-pick $empty_args $ff \"$@\"\n> +\tfi\n\nIt seems that there is another call to \"git cherry-pick\" in the\nscript (\"git grep\" for it).  Does it need a similar change?\n"},{"id":"221764","messageId":"87bo6wyn0z.fsf@duckcorp.org","threadId":"34237","inReplyTo":"7vr4fvkxew.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Do not ignore merge options in interactive rebase","fromName":"Arnaud Fontaine","fromEmail":"arnau@debian.org","sentAt":"2013-06-24T07:40:28Z","receivedAt":"2013-06-24T07:40:28Z","isPatch":true,"sender":{"key":"arnau@debian.org","avatar":null},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Arnaud Fontaine <arnau@debian.org> writes:\n>\n>> Merge strategy and its options can  be specified in `git rebase`, but\n>> with `--interactive`, they were completely ignored.\n>\n> And why  is it a bad  thing?  If you meant  s/--interactive/-m/ in the\n> above, then I can sort of understand the justification, though.\n\nSorry, it was not  clear. I meant that you can  do 'rebase -m --strategy\nrecursive'. But with 'rebase --interactive -m --strategy recursive', '-m\n--strategy recursive' is ignored. To  me, this is not consistent because\nthe behavior is  different in interactive mode...   Personally, I needed\nto specify the strategy and its options while using interactive mode and\nit seems I'm not the only one[0].\n\n>> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\n>> old mode 100644\n>> new mode 100755\n>\n> I see an unjustifiable mode change here.\n\nSorry about that, I fixed it.\n\n>> index f953d8d..c157fdf\n>> --- a/git-rebase--interactive.sh\n>> +++ b/git-rebase--interactive.sh\n>> @@ -239,7 +239,16 @@ pick_one () {\n>>  \n>>  \ttest -d \"$rewritten\" &&\n>>  \t\tpick_one_preserving_merges \"$@\" && return\n>> -\toutput git cherry-pick $empty_args $ff \"$@\"\n>> +\n>> +\tif test -n \"$do_merge\"\n>> +\tthen\n>\n> So you _did_ mean \"rebase -m\"?\n\nI really  meant 'rebase --interactive  -m'. do_merge  is set to  true if\neither '--strategy' or '-m' or '-X' is given according to git-rebase.sh.\n\n>> +\t\ttest -z \"$strategy\" && strategy=recursive\n>> +\t\toutput git cherry-pick --strategy=$strategy \\\n>\n> This is a bad change.\n>\n> I would understand if the above were:\n>\n> \tgit cherry-pick ${strategy+--strategy=$strategy} ...\n>\n> in other  words, \"if there is  no strategy specified, do  not override\n> the  configured  default  that  might  be  different  from  recursive\"\n> (pull.twohead may be set to resolve).\n\nIndeed, I did not know about that.  I wrongly thought it was a good idea\nto  do   the  same   as  both   git-rebase  (when   -X  is   given)  and\ngit-rebase--merge  which  do the  same  test  ('test -z  \"$strategy\"  &&\nstrategy=recursive').  However  after checking  more carefully,  I guess\nthat, for the former case, it  is because only recursive currently takes\noptions,  whereas,  for  the  latter  case, it  is  to  call  a  default\ngit-rebase-$strategy.\n\n>> +\t\t\t$(echo $strategy_opts | sed \"s/'--\\([^']*\\)'/-X\\1/g\") \\\n>\n> Is it guaranteed $startegy_opts do not have a space in it?\n\nstrategy_opts may be something like (git-rebase.sh): \"'--foo' '--bar'\",\nbut I'm not sure what is wrong if there is a space in it though.\n\n> There is a  call to \"git merge\" that  uses \"${strategy+-s $strategy}\",\n> but it does not seem to propagate the strategy option.  Does it need a\n> similar change?  It  seems that the first step might  be to factor out\n> these  calls  to the  \"git  cherry-pick\"  and  \"git merge\"  to  helper\n> functions  to make  it easier  to call  them with  -s/-X options  in a\n> consistent way.\n\nAs far as I understand, yes. I changed it. As it is really short, I just\nadded an if/else inside the script itself, not sure if that's ok...\n\n>> +\t\t\t$empty_args $ff \"$@\"\n>> +\telse\n>> +\t\toutput git cherry-pick $empty_args $ff \"$@\"\n>> +\tfi\n>\n> It seems that there is another call to \"git cherry-pick\" in the script\n> (\"git grep\" for it).  Does it need a similar change?\n\nAs far as I understand, yes. So I changed it as well.\n\nI  have sent  the fixed  patch in  my next  email. Many  thanks for  the\nreview!\n\nCheers,\n-- \nArnaud Fontaine\n\n[0] http://git.661346.n2.nabble.com/git-rebase-interactive-does-not-respect-merge-options-td7500093.html\n"},{"id":"221768","messageId":"87ppvcx84r.fsf@duckcorp.org","threadId":"34237","inReplyTo":"87bo6wyn0z.fsf@duckcorp.org","subject":"[PATCH] Do not ignore merge options in interactive rebase","fromName":"Arnaud Fontaine","fromEmail":"arnau@debian.org","sentAt":"2013-06-24T07:47:32Z","receivedAt":"2013-06-24T07:47:32Z","isPatch":true,"sender":{"key":"arnau@debian.org","avatar":null},"body":"Fix inconsistency where `--strategy` and/or `--strategy-option` can be\nspecified in git rebase, but with `--interactive` argument only there\nwere completely ignored.\n\nSigned-off-by: Arnaud Fontaine <arnau@debian.org>\n---\n git-rebase--interactive.sh    | 13 ++++++++++---\n t/t3404-rebase-interactive.sh | 11 +++++++++++\n 2 files changed, 21 insertions(+), 3 deletions(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex f953d8d..e558397 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -80,6 +80,13 @@ amend=\"$state_dir\"/amend\n rewritten_list=\"$state_dir\"/rewritten-list\n rewritten_pending=\"$state_dir\"/rewritten-pending\n \n+strategy_args=\n+if test -n \"$do_merge\"\n+then\n+\tstrategy_args=\"${strategy+--strategy=$strategy}\n+ $(echo $strategy_opts | sed \"s/'--\\([^']*\\)'/-X\\1/g\")\"\n+fi\n+\n GIT_CHERRY_PICK_HELP=\"$resolvemsg\"\n export GIT_CHERRY_PICK_HELP\n \n@@ -239,7 +246,7 @@ pick_one () {\n \n \ttest -d \"$rewritten\" &&\n \t\tpick_one_preserving_merges \"$@\" && return\n-\toutput git cherry-pick $empty_args $ff \"$@\"\n+\toutput git cherry-pick $strategy_args $empty_args $ff \"$@\"\n }\n \n pick_one_preserving_merges () {\n@@ -341,7 +348,7 @@ pick_one_preserving_merges () {\n \t\t\t# No point in merging the first parent, that's HEAD\n \t\t\tnew_parents=${new_parents# $first_parent}\n \t\t\tif ! do_with_author output \\\n-\t\t\t\tgit merge --no-ff ${strategy:+-s $strategy} -m \\\n+\t\t\t\tgit merge --no-ff $strategy_args -m \\\n \t\t\t\t\t\"$msg_content\" $new_parents\n \t\t\tthen\n \t\t\t\tprintf \"%s\\n\" \"$msg_content\" > \"$GIT_DIR\"/MERGE_MSG\n@@ -350,7 +357,7 @@ pick_one_preserving_merges () {\n \t\t\techo \"$sha1 $(git rev-parse HEAD^0)\" >> \"$rewritten_list\"\n \t\t\t;;\n \t\t*)\n-\t\t\toutput git cherry-pick \"$@\" ||\n+\t\t\toutput git cherry-pick $strategy_args \"$@\" ||\n \t\t\t\tdie_with_patch $sha1 \"Could not pick $sha1\"\n \t\t\t;;\n \t\tesac\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex 79e8d3c..8b6a36f 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -947,4 +947,15 @@ test_expect_success 'rebase -i respects core.commentchar' '\n \ttest B = $(git cat-file commit HEAD^ | sed -ne \\$p)\n '\n \n+test_expect_success 'rebase -i with --strategy and -X' '\n+\tgit checkout -b conflict-merge-use-theirs conflict-branch &&\n+\tgit reset --hard HEAD^ &&\n+\techo five >conflict &&\n+\techo Z >file1 &&\n+\tgit commit -a -m \"one file conflict\" &&\n+\tEDITOR=true git rebase -i --strategy=recursive -Xours conflict-branch &&\n+\ttest $(git show conflict-branch:conflict) = $(cat conflict) &&\n+\ttest $(cat file1) = Z\n+'\n+\n test_done\n-- \n1.8.3.GIT\n"},{"id":"221795","messageId":"7vd2rbd26e.fsf@alter.siamese.dyndns.org","threadId":"34237","inReplyTo":"87bo6wyn0z.fsf@duckcorp.org","subject":"Re: [PATCH] Do not ignore merge options in interactive rebase","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-24T14:16:25Z","receivedAt":"2013-06-24T14:16:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Arnaud Fontaine <arnau@debian.org> writes:\n\n>>> +\t\t\t$(echo $strategy_opts | sed \"s/'--\\([^']*\\)'/-X\\1/g\") \\\n>>\n>> Is it guaranteed $startegy_opts do not have a space in it?\n>\n> strategy_opts may be something like (git-rebase.sh): \"'--foo' '--bar'\",\n> but I'm not sure what is wrong if there is a space in it though.\n\nI was primarily worried  about \"'--frotz=nitfol xyzzy'\", where you\nneed to pass -X='frotz=nitfol xyzzy' so that 'xyzzy' part does not\nbecome a separate argument directly given to 'git merge' and friends.\n\nAnd adding '' around \\1 is not sufficient, because the value given\nto the \"--frotz\" may have to be \"nitfol 'n xyzzy\".\n\nA comment next to that sed script that says \"Currently there is no\nstrategy option that needs quoting\" (if it is the case; I didn't\ncheck), together with a guard to protect us against unexpected\nstrategy-opts, might be a workable band-aid, though.\n\t\n"},{"id":"221960","messageId":"7v61x23yv9.fsf@alter.siamese.dyndns.org","threadId":"34237","inReplyTo":"87ppvcx84r.fsf@duckcorp.org","subject":"Re: [PATCH] Do not ignore merge options in interactive rebase","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-25T17:04:58Z","receivedAt":"2013-06-25T17:04:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Arnaud Fontaine <arnau@debian.org> writes:\n\n> Fix inconsistency where `--strategy` and/or `--strategy-option` can be\n> specified in git rebase, but with `--interactive` argument only there\n> were completely ignored.\n>\n> Signed-off-by: Arnaud Fontaine <arnau@debian.org>\n> ---\n>  git-rebase--interactive.sh    | 13 ++++++++++---\n>  t/t3404-rebase-interactive.sh | 11 +++++++++++\n>  2 files changed, 21 insertions(+), 3 deletions(-)\n>\n> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\n> index f953d8d..e558397 100644\n> --- a/git-rebase--interactive.sh\n> +++ b/git-rebase--interactive.sh\n> @@ -80,6 +80,13 @@ amend=\"$state_dir\"/amend\n>  rewritten_list=\"$state_dir\"/rewritten-list\n>  rewritten_pending=\"$state_dir\"/rewritten-pending\n>  \n> +strategy_args=\n> +if test -n \"$do_merge\"\n> +then\n> +\tstrategy_args=\"${strategy+--strategy=$strategy}\n> + $(echo $strategy_opts | sed \"s/'--\\([^']*\\)'/-X\\1/g\")\"\n> +fi\n\nThe \"${var+if_set_use_this}\" interpolates if $var is set (as opposed to\n\"unset\"); this will cause it to have \"--strategy= \" in strategy_args\nwhen $do_merge is set.  If you ran t3421, you would have noticed\nbreakage.\n\nYou should use ${var:+if_set_to_nonempty_use_this} here.\n\nstragety_opts is designed to be a valid shell scriptlet that can be\ninserted as a part of eval'ed string.  git-rebase.sh does this:\n\n\t-X)\n\t\tshift\n\t\tstrategy_opts=\"$strategy_opts $(git rev-parse --sq-quote \"--$1\")\"\n\t\tdo_merge=t\n\nso that git-rebase--merge.sh can use it like so:\n\n\teval 'git-merge-$strategy' $strategy_opts '\"$cmt^\" -- \"$hd\" \"$cmt\"'\n\nwithout having to worry about quoting issues when future strategy\noptions have letters that need quoting, e.g.\n\n\t$ git rebase -X foo=\"bar ' baz\"\n\ngit rev-parse --sq-quote turns it into '--foo=bar '\\'' baz' and then\nin the eval'ed string, it becomes a single argument given to the\ngit-merge-$strategy command, even though it may have spaces and\nsingle quotes and other characters that may be tricky to quote\nmanually without mistakes.\n\nSo munging it manually with sloppy \"sed\" script is simply wrong.\n\n>  GIT_CHERRY_PICK_HELP=\"$resolvemsg\"\n>  export GIT_CHERRY_PICK_HELP\n>  \n> @@ -239,7 +246,7 @@ pick_one () {\n>  \n>  \ttest -d \"$rewritten\" &&\n>  \t\tpick_one_preserving_merges \"$@\" && return\n> -\toutput git cherry-pick $empty_args $ff \"$@\"\n> +\toutput git cherry-pick $strategy_args $empty_args $ff \"$@\"\n>  }\n>  \n>  pick_one_preserving_merges () {\n> @@ -341,7 +348,7 @@ pick_one_preserving_merges () {\n>  \t\t\t# No point in merging the first parent, that's HEAD\n>  \t\t\tnew_parents=${new_parents# $first_parent}\n>  \t\t\tif ! do_with_author output \\\n> -\t\t\t\tgit merge --no-ff ${strategy:+-s $strategy} -m \\\n> +\t\t\t\tgit merge --no-ff $strategy_args -m \\\n>  \t\t\t\t\t\"$msg_content\" $new_parents\n>  \t\t\tthen\n>  \t\t\t\tprintf \"%s\\n\" \"$msg_content\" > \"$GIT_DIR\"/MERGE_MSG\n> @@ -350,7 +357,7 @@ pick_one_preserving_merges () {\n>  \t\t\techo \"$sha1 $(git rev-parse HEAD^0)\" >> \"$rewritten_list\"\n>  \t\t\t;;\n>  \t\t*)\n> -\t\t\toutput git cherry-pick \"$@\" ||\n> +\t\t\toutput git cherry-pick $strategy_args \"$@\" ||\n>  \t\t\t\tdie_with_patch $sha1 \"Could not pick $sha1\"\n>  \t\t\t;;\n>  \t\tesac\n> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> index 79e8d3c..8b6a36f 100755\n> --- a/t/t3404-rebase-interactive.sh\n> +++ b/t/t3404-rebase-interactive.sh\n> @@ -947,4 +947,15 @@ test_expect_success 'rebase -i respects core.commentchar' '\n>  \ttest B = $(git cat-file commit HEAD^ | sed -ne \\$p)\n>  '\n>  \n> +test_expect_success 'rebase -i with --strategy and -X' '\n> +\tgit checkout -b conflict-merge-use-theirs conflict-branch &&\n> +\tgit reset --hard HEAD^ &&\n> +\techo five >conflict &&\n> +\techo Z >file1 &&\n> +\tgit commit -a -m \"one file conflict\" &&\n> +\tEDITOR=true git rebase -i --strategy=recursive -Xours conflict-branch &&\n> +\ttest $(git show conflict-branch:conflict) = $(cat conflict) &&\n> +\ttest $(cat file1) = Z\n> +'\n> +\n>  test_done\n"},{"id":"221967","messageId":"7v61x22av9.fsf@alter.siamese.dyndns.org","threadId":"34237","inReplyTo":"7v61x23yv9.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Do not ignore merge options in interactive rebase","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-25T20:28:42Z","receivedAt":"2013-06-25T20:28:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> You should use ${var:+if_set_to_nonempty_use_this} here.\n> ...\n> So munging it manually with sloppy \"sed\" script is simply wrong.\n\nPerhaps something like this on top of your patch squashed in?\n\nThe eval magic lets the shell to split $strategy_opts back into\nindividual words (e.g. \"--subtree=My Project\" is a single word),\nstrip the leading \"--\", and then uses \"rev-parse --sq-quote\" again\nto turn them into \"-X 'subtree=My Project'\" (two words), which can\nbe inserted into a string later to be eval'ed.\n\n git-rebase--interactive.sh | 18 +++++++++++-------\n 1 file changed, 11 insertions(+), 7 deletions(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex e558397..ae2da7a 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -83,8 +83,13 @@ rewritten_pending=\"$state_dir\"/rewritten-pending\n strategy_args=\n if test -n \"$do_merge\"\n then\n-\tstrategy_args=\"${strategy+--strategy=$strategy}\n- $(echo $strategy_opts | sed \"s/'--\\([^']*\\)'/-X\\1/g\")\"\n+\tstrategy_args=${strategy:+--strategy=$strategy}\n+\teval '\n+\t\tfor strategy_opt in '\"$strategy_opts\"'\n+\t\tdo\n+\t\t\tstrategy_args=\"$strategy_args -X$(git rev-parse --sq-quote \"${strategy_opt#--}\")\"\n+\t\tdone\n+\t'\n fi\n \n GIT_CHERRY_PICK_HELP=\"$resolvemsg\"\n@@ -246,7 +251,7 @@ pick_one () {\n \n \ttest -d \"$rewritten\" &&\n \t\tpick_one_preserving_merges \"$@\" && return\n-\toutput git cherry-pick $strategy_args $empty_args $ff \"$@\"\n+\toutput eval git cherry-pick \"$strategy_args\" $empty_args $ff \"$@\"\n }\n \n pick_one_preserving_merges () {\n@@ -347,9 +352,8 @@ pick_one_preserving_merges () {\n \t\t\tmsg_content=\"$(commit_message $sha1)\"\n \t\t\t# No point in merging the first parent, that's HEAD\n \t\t\tnew_parents=${new_parents# $first_parent}\n-\t\t\tif ! do_with_author output \\\n-\t\t\t\tgit merge --no-ff $strategy_args -m \\\n-\t\t\t\t\t\"$msg_content\" $new_parents\n+\t\t\tif ! do_with_author output eval \\\n+\t\t\t'git merge --no-ff $strategy_args -m \"$msg_content\" $new_parents'\n \t\t\tthen\n \t\t\t\tprintf \"%s\\n\" \"$msg_content\" > \"$GIT_DIR\"/MERGE_MSG\n \t\t\t\tdie_with_patch $sha1 \"Error redoing merge $sha1\"\n@@ -357,7 +361,7 @@ pick_one_preserving_merges () {\n \t\t\techo \"$sha1 $(git rev-parse HEAD^0)\" >> \"$rewritten_list\"\n \t\t\t;;\n \t\t*)\n-\t\t\toutput git cherry-pick $strategy_args \"$@\" ||\n+\t\t\toutput eval git cherry-pick \"$strategy_args\" \"$@\" ||\n \t\t\t\tdie_with_patch $sha1 \"Could not pick $sha1\"\n \t\t\t;;\n \t\tesac\n"},{"id":"222348","messageId":"87sizx2xqe.fsf@duckcorp.org","threadId":"34237","inReplyTo":"7v61x22av9.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Do not ignore merge options in interactive rebase","fromName":"Arnaud Fontaine","fromEmail":"arnau@debian.org","sentAt":"2013-07-02T08:05:13Z","receivedAt":"2013-07-02T08:05:13Z","isPatch":true,"sender":{"key":"arnau@debian.org","avatar":null},"body":"Hello,\n\nSorry for the late reply.\n\nJunio C Hamano <gitster@pobox.com> writes:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> You should use ${var:+if_set_to_nonempty_use_this} here.\n>> ...\n>> So munging it manually with sloppy \"sed\" script is simply wrong.\n>\n> Perhaps something like this on top of your patch squashed in?\n>\n> The  eval magic  lets  the  shell to  split  $strategy_opts back  into\n> individual words (e.g. \"--subtree=My Project\" is a single word), strip\n> the leading \"--\",  and then uses \"rev-parse --sq-quote\"  again to turn\n> them into \"-X 'subtree=My Project'\" (two words), which can be inserted\n> into a string later to be eval'ed.\n>\n>  git-rebase--interactive.sh | 18 +++++++++++-------\n>  1 file changed, 11 insertions(+), 7 deletions(-)\n>\n> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\n> index e558397..ae2da7a 100644\n> --- a/git-rebase--interactive.sh\n> +++ b/git-rebase--interactive.sh\n> @@ -83,8 +83,13 @@ rewritten_pending=\"$state_dir\"/rewritten-pending\n>  strategy_args=\n>  if test -n \"$do_merge\"\n>  then\n> -\tstrategy_args=\"${strategy+--strategy=$strategy}\n> - $(echo $strategy_opts | sed \"s/'--\\([^']*\\)'/-X\\1/g\")\"\n> +\tstrategy_args=${strategy:+--strategy=$strategy}\n> +\teval '\n> +\t\tfor strategy_opt in '\"$strategy_opts\"'\n> +\t\tdo\n> +\t\t\tstrategy_args=\"$strategy_args -X$(git rev-parse --sq-quote \"${strategy_opt#--}\")\"\n> +\t\tdone\n> +\t'\n>  fi\n>  \n>  GIT_CHERRY_PICK_HELP=\"$resolvemsg\"\n> @@ -246,7 +251,7 @@ pick_one () {\n>  \n>  \ttest -d \"$rewritten\" &&\n>  \t\tpick_one_preserving_merges \"$@\" && return\n> -\toutput git cherry-pick $strategy_args $empty_args $ff \"$@\"\n> +\toutput eval git cherry-pick \"$strategy_args\" $empty_args $ff \"$@\"\n>  }\n>  \n>  pick_one_preserving_merges () {\n> @@ -347,9 +352,8 @@ pick_one_preserving_merges () {\n>  \t\t\tmsg_content=\"$(commit_message $sha1)\"\n>  \t\t\t# No point in merging the first parent, that's HEAD\n>  \t\t\tnew_parents=${new_parents# $first_parent}\n> -\t\t\tif ! do_with_author output \\\n> -\t\t\t\tgit merge --no-ff $strategy_args -m \\\n> -\t\t\t\t\t\"$msg_content\" $new_parents\n> +\t\t\tif ! do_with_author output eval \\\n> +\t\t\t'git merge --no-ff $strategy_args -m \"$msg_content\" $new_parents'\n>  \t\t\tthen\n>  \t\t\t\tprintf \"%s\\n\" \"$msg_content\" > \"$GIT_DIR\"/MERGE_MSG\n>  \t\t\t\tdie_with_patch $sha1 \"Error redoing merge $sha1\"\n> @@ -357,7 +361,7 @@ pick_one_preserving_merges () {\n>  \t\t\techo \"$sha1 $(git rev-parse HEAD^0)\" >> \"$rewritten_list\"\n>  \t\t\t;;\n>  \t\t*)\n> -\t\t\toutput git cherry-pick $strategy_args \"$@\" ||\n> +\t\t\toutput eval git cherry-pick \"$strategy_args\" \"$@\" ||\n>  \t\t\t\tdie_with_patch $sha1 \"Could not pick $sha1\"\n>  \t\t\t;;\n>  \t\tesac\n\nIndeed, this is much  better this way. Thank you very  much! I have just\nsquashed your patch into mine and ran successfully t3404 and t3421 tests\nafter rebasing my branch on top of current master.\n\nI will send  the final patch in  another email. Of course,  feel free to\ntake authorship of the patch or add yourself as Signed-off-by.\n\nCheers,\n-- \nArnaud Fontaine\n"},{"id":"222349","messageId":"87r4fh2xpf.fsf@duckcorp.org","threadId":"34237","inReplyTo":"7v61x22av9.fsf@alter.siamese.dyndns.org","subject":"[PATCH] Do not ignore merge options in interactive rebase","fromName":"Arnaud Fontaine","fromEmail":"arnau@debian.org","sentAt":"2013-07-02T08:05:48Z","receivedAt":"2013-07-02T08:05:48Z","isPatch":true,"sender":{"key":"arnau@debian.org","avatar":null},"body":"Merge strategy and its options can be specified in `git rebase`,\nbut with `--interactive`, they were completely ignored.\n\nSigned-off-by: Arnaud Fontaine <arnau@debian.org>\n---\n git-rebase--interactive.sh    | 21 ++++++++++++++++-----\n t/t3404-rebase-interactive.sh | 11 +++++++++++\n 2 files changed, 27 insertions(+), 5 deletions(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex 169e876..157690b 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -80,6 +80,18 @@ amend=\"$state_dir\"/amend\n rewritten_list=\"$state_dir\"/rewritten-list\n rewritten_pending=\"$state_dir\"/rewritten-pending\n \n+strategy_args=\n+if test -n \"$do_merge\"\n+then\n+\tstrategy_args=${strategy:+--strategy=$strategy}\n+\teval '\n+\t\tfor strategy_opt in '\"$strategy_opts\"'\n+\t\tdo\n+\t\t\tstrategy_args=\"$strategy_args -X$(git rev-parse --sq-quote \"${strategy_opt#--}\")\"\n+\t\tdone\n+\t'\n+fi\n+\n GIT_CHERRY_PICK_HELP=\"$resolvemsg\"\n export GIT_CHERRY_PICK_HELP\n \n@@ -239,7 +251,7 @@ pick_one () {\n \n \ttest -d \"$rewritten\" &&\n \t\tpick_one_preserving_merges \"$@\" && return\n-\toutput git cherry-pick $empty_args $ff \"$@\"\n+\toutput eval git cherry-pick \"$strategy_args\" $empty_args $ff \"$@\"\n }\n \n pick_one_preserving_merges () {\n@@ -340,9 +352,8 @@ pick_one_preserving_merges () {\n \t\t\tmsg_content=\"$(commit_message $sha1)\"\n \t\t\t# No point in merging the first parent, that's HEAD\n \t\t\tnew_parents=${new_parents# $first_parent}\n-\t\t\tif ! do_with_author output \\\n-\t\t\t\tgit merge --no-ff ${strategy:+-s $strategy} -m \\\n-\t\t\t\t\t\"$msg_content\" $new_parents\n+\t\t\tif ! do_with_author output eval \\\n+\t\t\t'git merge --no-ff $strategy_args -m \"$msg_content\" $new_parents'\n \t\t\tthen\n \t\t\t\tprintf \"%s\\n\" \"$msg_content\" > \"$GIT_DIR\"/MERGE_MSG\n \t\t\t\tdie_with_patch $sha1 \"Error redoing merge $sha1\"\n@@ -350,7 +361,7 @@ pick_one_preserving_merges () {\n \t\t\techo \"$sha1 $(git rev-parse HEAD^0)\" >> \"$rewritten_list\"\n \t\t\t;;\n \t\t*)\n-\t\t\toutput git cherry-pick \"$@\" ||\n+\t\t\toutput eval git cherry-pick \"$strategy_args\" \"$@\" ||\n \t\t\t\tdie_with_patch $sha1 \"Could not pick $sha1\"\n \t\t\t;;\n \t\tesac\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex d6b4143..8a6ec03 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -950,4 +950,15 @@ test_expect_success 'rebase -i, with <onto> and <upstream> specified as :/quuxer\n \tgit checkout branch1\n '\n \n+test_expect_success 'rebase -i with --strategy and -X' '\n+\tgit checkout -b conflict-merge-use-theirs conflict-branch &&\n+\tgit reset --hard HEAD^ &&\n+\techo five >conflict &&\n+\techo Z >file1 &&\n+\tgit commit -a -m \"one file conflict\" &&\n+\tEDITOR=true git rebase -i --strategy=recursive -Xours conflict-branch &&\n+\ttest $(git show conflict-branch:conflict) = $(cat conflict) &&\n+\ttest $(cat file1) = Z\n+'\n+\n test_done\n-- \n1.8.3.GIT\n"}]}