{"thread":{"id":"25698","subject":"[PATCH v2] rebase -X: do not clobber strategy","startedAt":"2010-11-10T07:14:26Z","lastAt":"2010-11-12T20:55:27Z","messageCount":8,"participants":["Martin von Zweigbergk","Sverre Rabbelier","Thomas Rast","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"155590","messageId":"1289373266-32593-1-git-send-email-martin.von.zweigbergk@gmail.com","threadId":"25698","inReplyTo":null,"subject":"[PATCH v2] rebase -X: do not clobber strategy","fromName":"Martin von Zweigbergk","fromEmail":"martin.von.zweigbergk@gmail.com","sentAt":"2010-11-10T07:14:26Z","receivedAt":"2010-11-10T07:14:26Z","isPatch":true,"sender":{"key":"martinvonz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/891642?v=4"},"body":"If any strategy options are passed to -X, the strategy will always be\nset to 'recursive'. According to the documentation, it should default to\n'recursive' if it is not set, but it should be possible to set it to\nother values.\n\nSigned-off-by: Martin von Zweigbergk \n---\n git-rebase.sh | 4 ----\n 1 files changed, 0 insertions(+), 4 deletions(-)\n\ndiff --git a/git-rebase.sh b/git-rebase.sh\nindex 42c0628..ec08f9c 100755\n--- a/git-rebase.sh\n+++ b/git-rebase.sh\n@@ -312,10 +312,6 @@ do\n \t\tesac\n \t\tstrategy_opts=\"$strategy_opts $(git rev-parse --sq-quote \"--$newopt\")\"\n \t\tdo_merge=t\n-\t\tif test -n \"$strategy\"\n-\t\tthen\n-\t\t\tstrategy=recursive\n-\t\tfi\n \t\t;;\n \t-s=*|--s=*|--st=*|--str=*|--stra=*|--strat=*|--strate=*|\\\n \t\t--strateg=*|--strategy=*|\\\n-- \n1.7.3.2.167.ga361b\n"},{"id":"155610","messageId":"AANLkTimEvp2mQu1QWhx-FDEx37NLi8jose_p+A1y1iHK@mail.gmail.com","threadId":"25698","inReplyTo":"1289373266-32593-1-git-send-email-martin.von.zweigbergk@gmail.com","subject":"Re: [PATCH v2] rebase -X: do not clobber strategy","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2010-11-10T18:56:10Z","receivedAt":"2010-11-10T18:56:10Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Wed, Nov 10, 2010 at 08:14, Martin von Zweigbergk\n wrote:\n> If any strategy options are passed to -X, the strategy will always be\n> set to 'recursive'. According to the documentation, it should default to\n> 'recursive' if it is not set, but it should be possible to set it to\n> other values.\n\nRepeat of other threat since this is a new patch: can we have a test for this?\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"155639","messageId":"AANLkTiks87caBYo78Xh1hwnEfDF9yKwBn5q3wa87hBo3@mail.gmail.com","threadId":"25698","inReplyTo":"AANLkTimEvp2mQu1QWhx-FDEx37NLi8jose_p+A1y1iHK@mail.gmail.com","subject":"Re: [PATCH v2] rebase -X: do not clobber strategy","fromName":"Martin von Zweigbergk","fromEmail":"martin.von.zweigbergk@gmail.com","sentAt":"2010-11-11T01:11:04Z","receivedAt":"2010-11-11T01:11:04Z","isPatch":true,"sender":{"key":"martinvonz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/891642?v=4"},"body":"On Wed, Nov 10, 2010 at 1:56 PM, Sverre Rabbelier wrote:\n> On Wed, Nov 10, 2010 at 08:14, Martin von Zweigbergk\n> wrote:\n>> If any strategy options are passed to -X, the strategy will always be\n>> set to 'recursive'. According to the documentation, it should default to\n>> 'recursive' if it is not set, but it should be possible to set it to\n>> other values.\n>\n> Repeat of other threat since this is a new patch: can we have a test for this?\n\n\nI don't think there are any merge strategies other than recursive that\naccept options, so what I could add a test case for is that e.g.\n'-s ours -X foo' uses the 'ours' strategy, even though 'foo' will be\nignored. I have very little experience with merge strategies, but I will\ngive it a try. Hopefully there is some existing test case I can copy and\nmodify.\n\nBtw, why is the default (if no strategy is specifed) for 'git rebase' to\nuse 'recursive', while for 'git merge' \"a built-in list of strategies is\nused instead (git merge-recursive when merging a single head, git\nmerge-octopus otherwise)\"?\n"},{"id":"155660","messageId":"201011111141.33623.trast@student.ethz.ch","threadId":"25698","inReplyTo":"AANLkTiks87caBYo78Xh1hwnEfDF9yKwBn5q3wa87hBo3@mail.gmail.com","subject":"Re: [PATCH v2] rebase -X: do not clobber strategy","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2010-11-11T10:41:33Z","receivedAt":"2010-11-11T10:41:33Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Martin von Zweigbergk wrote:\n> Btw, why is the default (if no strategy is specifed) for 'git rebase' to\n> use 'recursive', while for 'git merge' \"a built-in list of strategies is\n> used instead (git merge-recursive when merging a single head, git\n> merge-octopus otherwise)\"?\n\nBecause rebase does a tree-level merge, so it never attempts to merge\nthan one branch, so octopus never enters the picture.\n\nSorry for the original breakage; while it has Mike Lundy assigned as\nauthor, I resurrected and resubmitted his patch and should have\nnoticed.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"155682","messageId":"AANLkTinBhDR2+SGtz4rmj8CrOh4yLPAB3A74BV-yBCxa@mail.gmail.com","threadId":"25698","inReplyTo":"201011111141.33623.trast@student.ethz.ch","subject":"Re: [PATCH v2] rebase -X: do not clobber strategy","fromName":"Martin von Zweigbergk","fromEmail":"martin.von.zweigbergk@gmail.com","sentAt":"2010-11-11T12:59:06Z","receivedAt":"2010-11-11T12:59:06Z","isPatch":true,"sender":{"key":"martinvonz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/891642?v=4"},"body":"On Thu, Nov 11, 2010 at 5:41 AM, Thomas Rast wrote:\n> Martin von Zweigbergk wrote:\n>> Btw, why is the default (if no strategy is specifed) for 'git rebase' to\n>> use 'recursive', while for 'git merge' \"a built-in list of strategies is\n>> used instead (git merge-recursive when merging a single head, git\n>> merge-octopus otherwise)\"?\n>\n> Because rebase does a tree-level merge, so it never attempts to merge\n> than one branch, so octopus never enters the picture.\n\nI can see why octopus doesn't make sense when doing a linearizing\nrebase, but what if it's merge-preserving rebase? The call to\n'git merge' in git-rebase--interactive.sh looks like this:\n new_parents=${new_parents# $first_parent}\n [...]\n git merge $strategy -m \"$msg\" $new_parents\n\nAlso, it sounds like \"a built-in list\" is something that could one day\nbe expanded. So my question is then whether it would make sense to call\n'git merge' without specifying a strategy when 'git rebase' is called\nwithout a strategy?\n\n> Sorry for the original breakage; while it has Mike Lundy assigned as\n> author, I resurrected and resubmitted his patch and should have\n> noticed.\n\nWell, since there are no other strategies than recursive that support\nstrategy options, it was not really a breakage in reality, at least as\nfar as I can see.\n\nSpeaking of that and about my earlier comment about writing a test case,\nwhat should really happen if the user calls 'git rebase -s ours -X foo'?\nShould it really be allowed? (I tried it and it does work, though.)\n"},{"id":"155740","messageId":"7vpqubfrkj.fsf@alter.siamese.dyndns.org","threadId":"25698","inReplyTo":"AANLkTiks87caBYo78Xh1hwnEfDF9yKwBn5q3wa87hBo3@mail.gmail.com","subject":"Re: [PATCH v2] rebase -X: do not clobber strategy","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-11-11T22:01:16Z","receivedAt":"2010-11-11T22:01:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin von Zweigbergk writes:\n\n> I don't think there are any merge strategies other than recursive that\n> accept options, so what I could add a test case for is that e.g.\n> '-s ours -X foo' uses the 'ours' strategy, even though 'foo' will be\n> ignored.\n\nYou would do something like this.\n\n-- >8 --\nSubject: [PATCH] t3402: test \"rebase -s -X\"\n\nSigned-off-by: Junio C Hamano \n---\n t/t3402-rebase-merge.sh | 21 +++++++++++++++++++++\n 1 files changed, 21 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t3402-rebase-merge.sh b/t/t3402-rebase-merge.sh\nindex 2bea656..be8c1d5 100755\n--- a/t/t3402-rebase-merge.sh\n+++ b/t/t3402-rebase-merge.sh\n@@ -117,4 +117,25 @@ test_expect_success 'picking rebase' '\n \tesac\n '\n \n+test_expect_success 'rebase -s funny -Xopt' '\n+\ttest_when_finished \"rm -fr test-bin funny.was.run\" &&\n+\tmkdir test-bin &&\n+\tcat >test-bin/git-merge-funny <<-EOF &&\n+\t#!$SHELL_PATH\n+\tcase \"\\$1\" in --opt) ;; *) exit 2 ;; esac\n+\tshift &&\n+\t>funny.was.run &&\n+\texec git merge-recursive \"\\$@\"\n+\tEOF\n+\tchmod +x test-bin/git-merge-funny &&\n+\tgit reset --hard &&\n+\tgit checkout -b test-funny master^ &&\n+\ttest_commit funny &&\n+\t(\n+\t\tPATH=./test-bin:$PATH\n+\t\tgit rebase -s funny -Xopt master\n+\t) &&\n+\ttest -f funny.was.run\n+'\n+\n test_done\n-- \n1.7.3.2.334.gd1031\n"},{"id":"155750","messageId":"AANLkTinrhfmeeToMZfW942xvJGz9vgiQd6+2bXkSawJU@mail.gmail.com","threadId":"25698","inReplyTo":"7vpqubfrkj.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] rebase -X: do not clobber strategy","fromName":"Martin von Zweigbergk","fromEmail":"martin.von.zweigbergk@gmail.com","sentAt":"2010-11-12T01:31:08Z","receivedAt":"2010-11-12T01:31:08Z","isPatch":true,"sender":{"key":"martinvonz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/891642?v=4"},"body":"On Thu, Nov 11, 2010 at 5:01 PM, Junio C Hamano wrote:\n> Martin von Zweigbergk writes:\n>\n>> I don't think there are any merge strategies other than recursive that\n>> accept options, so what I could add a test case for is that e.g.\n>> '-s ours -X foo' uses the 'ours' strategy, even though 'foo' will be\n>> ignored.\n>\n> You would do something like this.\n>\n> -- >8 --\n> Subject: [PATCH] t3402: test \"rebase -s -X\"\n>\n> Signed-off-by: Junio C Hamano \n> ---\n>  t/t3402-rebase-merge.sh |   21 +++++++++++++++++++++\n>  1 files changed, 21 insertions(+), 0 deletions(-)\n>\n> diff --git a/t/t3402-rebase-merge.sh b/t/t3402-rebase-merge.sh\n> index 2bea656..be8c1d5 100755\n> --- a/t/t3402-rebase-merge.sh\n> +++ b/t/t3402-rebase-merge.sh\n> @@ -117,4 +117,25 @@ test_expect_success 'picking rebase' '\n>        esac\n>  '\n>\n> +test_expect_success 'rebase -s funny -Xopt' '\n> +       test_when_finished \"rm -fr test-bin funny.was.run\" &&\n> +       mkdir test-bin &&\n> +       cat >test-bin/git-merge-funny <<-EOF &&\n> +       #!$SHELL_PATH\n> +       case \"\\$1\" in --opt) ;; *) exit 2 ;; esac\n> +       shift &&\n> +       >funny.was.run &&\n> +       exec git merge-recursive \"\\$@\"\n> +       EOF\n> +       chmod +x test-bin/git-merge-funny &&\n> +       git reset --hard &&\n> +       git checkout -b test-funny master^ &&\n> +       test_commit funny &&\n> +       (\n> +               PATH=./test-bin:$PATH\n> +               git rebase -s funny -Xopt master\n> +       ) &&\n> +       test -f funny.was.run\n> +'\n> +\n>  test_done\n> --\n> 1.7.3.2.334.gd1031\n\nThat's clever. Thank you!\n\nThe fix for this bug touches the same parts of the code as my\nrefactoring of git rebase. If you don't mind, I will post your test\ncase and the fix as part of the refactoring series.\n"},{"id":"155799","messageId":"7vaalefeio.fsf@alter.siamese.dyndns.org","threadId":"25698","inReplyTo":"AANLkTinrhfmeeToMZfW942xvJGz9vgiQd6+2bXkSawJU@mail.gmail.com","subject":"Re: [PATCH v2] rebase -X: do not clobber strategy","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-11-12T20:55:27Z","receivedAt":"2010-11-12T20:55:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin von Zweigbergk writes:\n\n> That's clever. Thank you!\n>\n> The fix for this bug touches the same parts of the code as my\n> refactoring of git rebase. If you don't mind, I will post your test\n> case and the fix as part of the refactoring series.\n\nNah, I'd rather squash the test script into the fix.\n"}]}