{"thread":{"id":"41722","subject":"[PATCH] rebase -x: do not die without -i","startedAt":"2016-03-17T01:19:44Z","lastAt":"2016-03-17T18:10:22Z","messageCount":6,"participants":["Stefan Beller","Johannes Sixt","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"280993","messageId":"1458177584-11378-1-git-send-email-sbeller@google.com","threadId":"41722","inReplyTo":null,"subject":"[PATCH] rebase -x: do not die without -i","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-17T01:19:44Z","receivedAt":"2016-03-17T01:19:44Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"In the later steps of preparing a patch series I do not want to edit the\npatches any more, but just make sure the test suite passes after each\npatch. Currently I would run\n\n  EDITOR=true git rebase -i <anchor> -x \"make test\"\n\nIn an ideal world the command would be simpler and just be\n\n  git rebase <anchor> -x \"make test\"\n\nto run the test for each commit I am about to send out for review.\nThis patch enables the short line.\n\nWhile at it, remove the double empty lines in t3404.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\n Junio,\n \n This is a slightly different approach.\n \n -x doesn't imply -i any more, but just the internal code does that. The user\n will see --exec working just fine without the -i option given.\n\n Thanks,\n Stefan\n\n git-rebase.sh                 |  4 +++-\n t/t3404-rebase-interactive.sh | 18 +++++-------------\n 2 files changed, 8 insertions(+), 14 deletions(-)\n\ndiff --git a/git-rebase.sh b/git-rebase.sh\nindex cf60c43..232484a 100755\n--- a/git-rebase.sh\n+++ b/git-rebase.sh\n@@ -351,7 +351,9 @@ test $# -gt 2 && usage\n if test -n \"$cmd\" &&\n    test \"$interactive_rebase\" != explicit\n then\n-\tdie \"$(gettext \"The --exec option must be used with the --interactive option\")\"\n+\tinteractive_rebase=implicit\n+\tGIT_EDITOR=true\n+\texport GIT_EDITOR\n fi\n \n if test -n \"$action\"\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex 544f9ad..2064d88 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -771,7 +771,6 @@ test_expect_success 'rebase-i history with funny messages' '\n \ttest_cmp expect actual\n '\n \n-\n test_expect_success 'prepare for rebase -i --exec' '\n \tgit checkout master &&\n \tgit checkout -b execute &&\n@@ -780,7 +779,6 @@ test_expect_success 'prepare for rebase -i --exec' '\n \ttest_commit three_exec main.txt three_exec\n '\n \n-\n test_expect_success 'running \"git rebase -i --exec git show HEAD\"' '\n \tset_fake_editor &&\n \tgit rebase -i --exec \"git show HEAD\" HEAD~2 >actual &&\n@@ -793,7 +791,6 @@ test_expect_success 'running \"git rebase -i --exec git show HEAD\"' '\n \ttest_cmp expected actual\n '\n \n-\n test_expect_success 'running \"git rebase --exec git show HEAD -i\"' '\n \tgit reset --hard execute &&\n \tset_fake_editor &&\n@@ -807,7 +804,6 @@ test_expect_success 'running \"git rebase --exec git show HEAD -i\"' '\n \ttest_cmp expected actual\n '\n \n-\n test_expect_success 'running \"git rebase -ix git show HEAD\"' '\n \tgit reset --hard execute &&\n \tset_fake_editor &&\n@@ -835,7 +831,6 @@ test_expect_success 'rebase -ix with several <CMD>' '\n \ttest_cmp expected actual\n '\n \n-\n test_expect_success 'rebase -ix with several instances of --exec' '\n \tgit reset --hard execute &&\n \tset_fake_editor &&\n@@ -850,7 +845,6 @@ test_expect_success 'rebase -ix with several instances of --exec' '\n \ttest_cmp expected actual\n '\n \n-\n test_expect_success 'rebase -ix with --autosquash' '\n \tgit reset --hard execute &&\n \tgit checkout -b autosquash &&\n@@ -876,16 +870,14 @@ test_expect_success 'rebase -ix with --autosquash' '\n \ttest_cmp expected actual\n '\n \n-\n-test_expect_success 'rebase --exec without -i shows error message' '\n+test_expect_success 'rebase --exec works without -i ' '\n \tgit reset --hard execute &&\n-\tset_fake_editor &&\n-\ttest_must_fail git rebase --exec \"git show HEAD\" HEAD~2 2>actual &&\n-\techo \"The --exec option must be used with the --interactive option\" >expected &&\n-\ttest_i18ncmp expected actual\n+\tgit rebase --exec true HEAD~2 2>actual2 >actual1 &&\n+\techo \"Successfully rebased and updated refs/heads/autosquash_expected.\" >expected &&\n+\ttest_i18ncmp expected actual2 &&\n+\ttest_line_count = 2 actual1\n '\n \n-\n test_expect_success 'rebase -i --exec without <CMD>' '\n \tgit reset --hard execute &&\n \tset_fake_editor &&\n-- \n2.8.0.rc3.1.ge1ac87c.dirty\n"},{"id":"281003","messageId":"56EA4CC1.9060602@kdbg.org","threadId":"41722","inReplyTo":"1458177584-11378-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH] rebase -x: do not die without -i","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2016-03-17T06:20:49Z","receivedAt":"2016-03-17T06:20:49Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 17.03.2016 um 02:19 schrieb Stefan Beller:\n> -test_expect_success 'rebase --exec without -i shows error message' '\n> +test_expect_success 'rebase --exec works without -i ' '\n>   \tgit reset --hard execute &&\n> -\tset_fake_editor &&\n> -\ttest_must_fail git rebase --exec \"git show HEAD\" HEAD~2 2>actual &&\n> -\techo \"The --exec option must be used with the --interactive option\" >expected &&\n> -\ttest_i18ncmp expected actual\n> +\tgit rebase --exec true HEAD~2 2>actual2 >actual1 &&\n> +\techo \"Successfully rebased and updated refs/heads/autosquash_expected.\" >expected &&\n> +\ttest_i18ncmp expected actual2 &&\n> +\ttest_line_count = 2 actual1\n\nWe don't have an explicit guideline, but please do not check stderr \noutput using test_cmp or test_i18ncmp. The reason is that some shells \nwrite trace output to stderr when run under 'set -x'. That is, when you \nrun this test as\n\n      ./t3404-rebase-interactive.sh -x -v\n\nit will fail because there is now more text in actual2 than expected. We \nhave a number of cases like this elsewhere, but we should not stack new \ncases on the pile.\n\nPlease use test_i18ngrep:\n\n\ttest_i18ngrep \"Successfully rebased and updated\" actual2 &&\n\ttest_line_count = 2 actual1\n\n-- Hannes\n"},{"id":"281004","messageId":"alpine.DEB.2.20.1603170745300.4690@virtualbox","threadId":"41722","inReplyTo":"1458177584-11378-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH] rebase -x: do not die without -i","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-03-17T06:48:43Z","receivedAt":"2016-03-17T06:48:43Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Stefan,\n\nOn Wed, 16 Mar 2016, Stefan Beller wrote:\n\n> In the later steps of preparing a patch series I do not want to edit the\n> patches any more, but just make sure the test suite passes after each\n> patch. Currently I would run\n> \n>   EDITOR=true git rebase -i <anchor> -x \"make test\"\n> \n> In an ideal world the command would be simpler and just be\n> \n>   git rebase <anchor> -x \"make test\"\n> \n> to run the test for each commit I am about to send out for review.\n> This patch enables the short line.\n\nMakes sense.\n\n> While at it, remove the double empty lines in t3404.\n\nCould you split those out? They may interfere with some of my work, and\nit's easier to figure things out from a trivial patch than from one\ncombining unrelated changes.\n\n> -test_expect_success 'rebase --exec without -i shows error message' '\n> +test_expect_success 'rebase --exec works without -i ' '\n>  \tgit reset --hard execute &&\n> -\tset_fake_editor &&\n> -\ttest_must_fail git rebase --exec \"git show HEAD\" HEAD~2 2>actual &&\n> -\techo \"The --exec option must be used with the --interactive option\" >expected &&\n> -\ttest_i18ncmp expected actual\n> +\tgit rebase --exec true HEAD~2 2>actual2 >actual1 &&\n\nHow about '--exec \"touch executed\"'? You'd want to verify that the command\nreally was executed...\n\nCiao,\nDscho\n"},{"id":"281010","messageId":"xmqq4mc535n2.fsf@gitster.mtv.corp.google.com","threadId":"41722","inReplyTo":"1458177584-11378-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH] rebase -x: do not die without -i","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-17T07:50:09Z","receivedAt":"2016-03-17T07:50:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> In the later steps of preparing a patch series I do not want to edit the\n> patches any more, but just make sure the test suite passes after each\n> patch. Currently I would run\n>\n>   EDITOR=true git rebase -i <anchor> -x \"make test\"\n\nHmm, I guess that may \"work\" but it sounds like quite a roundabout\nway to \"test all commits\".  \"rebase\" is about replaying history to\nend up with a set of newly minted commits, and being able to poke at\nthe state each commit records in the working tree is a side effect.\n\"rebase -i\" may use the same commit object if you didn't actually\nmake new commit as an optimization, but otherwise, it is like going\nthrough pages of a book, tearing each page to examine it, and\nreplacing each page with a photocopy of it before going to examine\nthe next page.  Which makes me feel somewhat dirty X-<.\n\nIn other words, that looks like a workaround for not having\n\n    $ git for-each-rev -x \"$command\" old..new\n\nwhere you can write \"sh -c 'git checkout $1 && make test' -\" as\nyour $command.\n"},{"id":"281037","messageId":"alpine.DEB.2.20.1603171406080.4690@virtualbox","threadId":"41722","inReplyTo":"xmqq4mc535n2.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] rebase -x: do not die without -i","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-03-17T13:11:01Z","receivedAt":"2016-03-17T13:11:01Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 17 Mar 2016, Junio C Hamano wrote:\n\n> I guess that may \"work\" but it sounds like quite a roundabout\n> way to \"test all commits\".  \"rebase\" is about replaying history to\n> end up with a set of newly minted commits, and being able to poke at\n> the state each commit records in the working tree is a side effect.\n\nI think there is a misunderstanding here. Stefan does not only want to\ntest all of the commits in the patch series. Stefan wants to make sure\nthat all patches in the series result in revisions that pass the test\nsuite.\n\nIn other words, it is not only about testing, it is also about fixing when\nthings break.\n\nAnd that is very, very much the purpose of the interactive rebase.\n\n>     $ git for-each-rev -x \"$command\" old..new\n> \n> where you can write \"sh -c 'git checkout $1 && make test' -\" as\n> your $command.\n\nYou meant\n\n\tgit rev-list old...new |\n\twhile read rev\n\tdo\n\t\t$command || break\n\tdone\n\n?\n\nAgain, this might *test* the revisions. But it does nothing to help fixing\nany patch in the series.\n\nCiao,\nDscho\n"},{"id":"281062","messageId":"xmqq37rp0ycx.fsf@gitster.mtv.corp.google.com","threadId":"41722","inReplyTo":"alpine.DEB.2.20.1603171406080.4690@virtualbox","subject":"Re: [PATCH] rebase -x: do not die without -i","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-17T18:10:22Z","receivedAt":"2016-03-17T18:10:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> And that is very, very much the purpose of the interactive rebase.\n>\n>>     $ git for-each-rev -x \"$command\" old..new\n>> \n>> where you can write \"sh -c 'git checkout $1 && make test' -\" as\n>> your $command.\n>\n> You meant\n>\n> \tgit rev-list old...new |\n> \twhile read rev\n> \tdo\n> \t\t$command || break\n> \tdone\n>\n> ?\n\nYeah, if I actually felt the lack of \"for-each-ref -x\" a problem (I\ndon't), that is what I would have used instead (but with just two\ndots ;-).\n\nBut you are correct to point out that I didn't consider that\n\"... and I want to fix right there if it breaks\" is a part of the\nuse case, mainly because the log message said this:\n\n    In the later steps of preparing a patch series I do not want to\n    edit the patches any more, but just make sure the test suite\n    passes after each patch.\n\nand partly I lack imagination.\n\nIf you throw in that extra \"I want to fix right there if it breaks\"\nrequirement, it makes perfect sense to make use of the sequencing\nmachinery we implement for \"rebase -i\", as the while loop above does\nnot give you the same sequencing without extra work.\n\nSo perhaps the idea of the patch is good with a better explanation.\n"}]}