{"thread":{"id":"51391","subject":"[PATCH 0/1] Make rebase.reschedulefailedexec less overzealous","startedAt":"2019-06-27T08:12:52Z","lastAt":"2019-07-01T11:58:18Z","messageCount":13,"participants":["Johannes Schindelin via GitGitGadget","Junio C Hamano","Johannes Schindelin","Phillip Wood","Vas Sudanagunta"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"378125","messageId":"pull.253.git.gitgitgadget@gmail.com","threadId":"51391","inReplyTo":null,"subject":"[PATCH 0/1] Make rebase.reschedulefailedexec less overzealous","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-06-27T08:12:48Z","receivedAt":"2019-06-27T08:12:52Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"This config setting is pretty useful, but it unfortunately stops all\nnon-interactive rebases with a bogus error message. This patch fixes that.\n\nReported via a commit comment on GitHub\n[https://github.com/git/git/commit/969de3ff0e0#commitcomment-33257187].\n\nJohannes Schindelin (1):\n Let rebase.reschedulefailedexec only affect interactive rebases\n\n builtin/rebase.c | 7 +++++--\n t/t3418-rebase-continue.sh | 8 ++++++++\n 2 files changed, 13 insertions(+), 2 deletions(-)\n\n\nbase-commit: e11ff8975bedc0aae82632c3cb72578c3d7fc0b2\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-253%2Fdscho%2Freschedule-failed-exec-gently-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-253/dscho/reschedule-failed-exec-gently-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/253\n-- \ngitgitgadget\n"},{"id":"378126","messageId":"fab124da41858b849ae74dfb1fe403bb834870f1.1561623167.git.gitgitgadget@gmail.com","threadId":"51391","inReplyTo":"pull.253.git.gitgitgadget@gmail.com","subject":"[PATCH 1/1] Let rebase.reschedulefailedexec only affect interactive rebases","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-06-27T08:12:49Z","receivedAt":"2019-06-27T08:12:53Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIt does not make sense to stop non-interactive rebases when that config\nsetting is set to `true`.\n\nReported by Vas Sudanagunta via:\nhttps://github.com/git/git/commit/969de3ff0e0#commitcomment-33257187\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/rebase.c | 7 +++++--\n t/t3418-rebase-continue.sh | 8 ++++++++\n 2 files changed, 13 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 4839e52555..78c3bd0634 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -834,6 +834,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \tstruct string_list strategy_options = STRING_LIST_INIT_NODUP;\n \tstruct object_id squash_onto;\n \tchar *squash_onto_name = NULL;\n+\tint reschedule_failed_exec = -1;\n \tstruct option builtin_rebase_options[] = {\n \t\tOPT_STRING(0, \"onto\", &options.onto_name,\n \t\t\t N_(\"revision\"),\n@@ -929,7 +930,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\tOPT_BOOL(0, \"root\", &options.root,\n \t\t\t N_(\"rebase all reachable commits up to the root(s)\")),\n \t\tOPT_BOOL(0, \"reschedule-failed-exec\",\n-\t\t\t &options.reschedule_failed_exec,\n+\t\t\t &reschedule_failed_exec,\n \t\t\t N_(\"automatically re-schedule any `exec` that fails\")),\n \t\tOPT_END(),\n \t};\n@@ -1227,8 +1228,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\tbreak;\n \t}\n \n-\tif (options.reschedule_failed_exec && !is_interactive(&options))\n+\tif (reschedule_failed_exec > 0 && !is_interactive(&options))\n \t\tdie(_(\"--reschedule-failed-exec requires an interactive rebase\"));\n+\tif (reschedule_failed_exec >= 0)\n+\t\toptions.reschedule_failed_exec = reschedule_failed_exec;\n \n \tif (options.git_am_opts.argc) {\n \t\t/* all am options except -q are compatible only with --am */\ndiff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh\nindex bdaa511bb0..4eff14dae5 100755\n--- a/t/t3418-rebase-continue.sh\n+++ b/t/t3418-rebase-continue.sh\n@@ -265,4 +265,12 @@ test_expect_success '--reschedule-failed-exec' '\n \ttest_i18ngrep \"has been rescheduled\" err\n '\n \n+test_expect_success 'rebase.reschedulefailedexec only affects `rebase -i`' '\n+\ttest_config rebase.reschedulefailedexec true &&\n+\ttest_must_fail git rebase -x false HEAD^ &&\n+\tgrep \"^exec false\" .git/rebase-merge/git-rebase-todo &&\n+\tgit rebase --abort &&\n+\tgit rebase HEAD^\n+'\n+\n test_done\n-- \ngitgitgadget\n"},{"id":"378183","messageId":"xmqqzhm2ang5.fsf@gitster-ct.c.googlers.com","threadId":"51391","inReplyTo":"fab124da41858b849ae74dfb1fe403bb834870f1.1561623167.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/1] Let rebase.reschedulefailedexec only affect interactive rebases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-06-27T18:32:42Z","receivedAt":"2019-06-27T18:32:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> From: Johannes Schindelin \n>\n> It does not make sense to stop non-interactive rebases when that config\n> setting is set to `true`.\n\nThe reader is assumed to know that that config setting is only about\ninteractive rebases, including \"rebase -x\", which probably is an OK\nexplanation.\n\nThe subject needs a bit more work, though. \n\n\"rebase: ignore rebase.reschedulefailedexec unless interative\"\n\nor something like that, perhaps.\n\n> @@ -929,7 +930,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> \t\tOPT_BOOL(0, \"root\", &options.root,\n> \t\t\t N_(\"rebase all reachable commits up to the root(s)\")),\n> \t\tOPT_BOOL(0, \"reschedule-failed-exec\",\n> -\t\t\t &options.reschedule_failed_exec,\n> +\t\t\t &reschedule_failed_exec,\n> \t\t\t N_(\"automatically re-schedule any `exec` that fails\")),\n> \t\tOPT_END(),\n> \t};\n> @@ -1227,8 +1228,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> \t\tbreak;\n> \t}\n> \n> -\tif (options.reschedule_failed_exec && !is_interactive(&options))\n> +\tif (reschedule_failed_exec > 0 && !is_interactive(&options))\n\nOK, it used to be that we got affected by what came from \"options\",\nwhich was read from the configuration. Now we only pay attention to\nthe command line, which makes sense.\n\nAt this point, we have already examined '-x' and called\nimply_interative(), so this should trigger for '-x' (without '-i'),\nright?\n\n> \t\tdie(_(\"--reschedule-failed-exec requires an interactive rebase\"));\n\nI wonder if users understand that '-x' is \"an interctive rebase\".\nThe documentation can read both ways, and one of these may want to\nbe clarified.\n\n\t-x , --exec \n\t...\n\tThis uses the --interactive machinery internally, but it can\n\tbe run without an explicit --interactive.\n\nIs it saying that use of interactive machinery is an impelementation\ndetail the users should not concern themselves (in which case, the\nmessage given to \"die()\" above is misleading---not a new problem\nwith this patch, though)? Is it saying \"-x\" makes it plenty clear\nthat the user wants interactive behaviour, so the users do not need\nto spell out --interactive in order to ask for it (in which case,\n\"die()\" message is fine, but \"... internally, but ...\" is\nmisleading)?\n\n> +\tif (reschedule_failed_exec >= 0)\n> +\t\toptions.reschedule_failed_exec = reschedule_failed_exec;\n\nOK, here we recover the bit that is only stored in a local variable\nand pass it into cmd_rebase__interactive() machinery via the options\nstructure, which lets the codepath after this point oblivious to\nthis change, which is good ;-).\n\n> \tif (options.git_am_opts.argc) {\n> \t\t/* all am options except -q are compatible only with --am */\n> diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh\n> index bdaa511bb0..4eff14dae5 100755\n> --- a/t/t3418-rebase-continue.sh\n> +++ b/t/t3418-rebase-continue.sh\n> @@ -265,4 +265,12 @@ test_expect_success '--reschedule-failed-exec' '\n> \ttest_i18ngrep \"has been rescheduled\" err\n> '\n> \n> +test_expect_success 'rebase.reschedulefailedexec only affects `rebase -i`' '\n> +\ttest_config rebase.reschedulefailedexec true &&\n> +\ttest_must_fail git rebase -x false HEAD^ &&\n\nThese three lines gives us a concise summary of this patch ;-)\n\n - The test title can serve as a starting point for a much better\n patch title.\n\n - We trigger for '-x' without requiring '-i'.\n\n> +\tgrep \"^exec false\" .git/rebase-merge/git-rebase-todo &&\n> +\tgit rebase --abort &&\n> +\tgit rebase HEAD^\n> +'\n> +\n> test_done\n"},{"id":"378258","messageId":"nycvar.QRO.7.76.6.1906281342280.44@tvgsbejvaqbjf.bet","threadId":"51391","inReplyTo":"xmqqzhm2ang5.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/1] Let rebase.reschedulefailedexec only affect interactive rebases","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-06-28T11:49:46Z","receivedAt":"2019-06-28T11:49:31Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 27 Jun 2019, Junio C Hamano wrote:\n\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n>\n> > From: Johannes Schindelin \n> >\n> > It does not make sense to stop non-interactive rebases when that config\n> > setting is set to `true`.\n>\n> The reader is assumed to know that that config setting is only about\n> interactive rebases, including \"rebase -x\", which probably is an OK\n> explanation.\n\nYou're right, it is okay, but could be better.\n\n> The subject needs a bit more work, though.\n>\n> \"rebase: ignore rebase.reschedulefailedexec unless interative\"\n>\n> or something like that, perhaps.\n\nRight.\n\n> > @@ -929,7 +930,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> > \t\tOPT_BOOL(0, \"root\", &options.root,\n> > \t\t\t N_(\"rebase all reachable commits up to the root(s)\")),\n> > \t\tOPT_BOOL(0, \"reschedule-failed-exec\",\n> > -\t\t\t &options.reschedule_failed_exec,\n> > +\t\t\t &reschedule_failed_exec,\n> > \t\t\t N_(\"automatically re-schedule any `exec` that fails\")),\n> > \t\tOPT_END(),\n> > \t};\n> > @@ -1227,8 +1228,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> > \t\tbreak;\n> > \t}\n> >\n> > -\tif (options.reschedule_failed_exec && !is_interactive(&options))\n> > +\tif (reschedule_failed_exec > 0 && !is_interactive(&options))\n>\n> OK, it used to be that we got affected by what came from \"options\",\n> which was read from the configuration. Now we only pay attention to\n> the command line, which makes sense.\n>\n> At this point, we have already examined '-x' and called\n> imply_interative(), so this should trigger for '-x' (without '-i'),\n> right?\n\nYes, at this point we have done all the parsing and automatic implying,\nand check for incompatible options.\n\n> > \t\tdie(_(\"--reschedule-failed-exec requires an interactive rebase\"));\n>\n> I wonder if users understand that '-x' is \"an interctive rebase\".\n> The documentation can read both ways, and one of these may want to\n> be clarified.\n>\n> \t-x , --exec \n> \t...\n> \tThis uses the --interactive machinery internally, but it can\n> \tbe run without an explicit --interactive.\n>\n> Is it saying that use of interactive machinery is an impelementation\n> detail the users should not concern themselves (in which case, the\n> message given to \"die()\" above is misleading---not a new problem\n> with this patch, though)? Is it saying \"-x\" makes it plenty clear\n> that the user wants interactive behaviour, so the users do not need\n> to spell out --interactive in order to ask for it (in which case,\n> \"die()\" message is fine, but \"... internally, but ...\" is\n> misleading)?\n\nHmm. What would you think about:\n\n \t\tdie(_(\"--reschedule-failed-exec requires --exec or --interactive\"));\n\n?\n\nIt is still not _complete_, but at least it should be a ton less\nconfusing.\n\n> > +\tif (reschedule_failed_exec >= 0)\n> > +\t\toptions.reschedule_failed_exec = reschedule_failed_exec;\n>\n> OK, here we recover the bit that is only stored in a local variable\n> and pass it into cmd_rebase__interactive() machinery via the options\n> structure, which lets the codepath after this point oblivious to\n> this change, which is good ;-).\n>\n> > \tif (options.git_am_opts.argc) {\n> > \t\t/* all am options except -q are compatible only with --am */\n> > diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh\n> > index bdaa511bb0..4eff14dae5 100755\n> > --- a/t/t3418-rebase-continue.sh\n> > +++ b/t/t3418-rebase-continue.sh\n> > @@ -265,4 +265,12 @@ test_expect_success '--reschedule-failed-exec' '\n> > \ttest_i18ngrep \"has been rescheduled\" err\n> > '\n> >\n> > +test_expect_success 'rebase.reschedulefailedexec only affects `rebase -i`' '\n> > +\ttest_config rebase.reschedulefailedexec true &&\n> > +\ttest_must_fail git rebase -x false HEAD^ &&\n>\n> These three lines gives us a concise summary of this patch ;-)\n>\n> - The test title can serve as a starting point for a much better\n> patch title.\n>\n> - We trigger for '-x' without requiring '-i'.\n\nI changed the oneline to\n\n\trebase --am: ignore rebase.reschedulefailedexec\n\nThis gives credit to the implementation details, as appropriate for commit\nmessages, and the error message still tries to be as helpful as possible\nfor users (who do not necessarily need to know that there are two\nbackends).\n\nAt this point, I am _really_ glad that we only have two backends left (for\nall practical purposes, I don't count --preserve-merges).\n\nCiao,\nDscho\n\n> > +\tgrep \"^exec false\" .git/rebase-merge/git-rebase-todo &&\n> > +\tgit rebase --abort &&\n> > +\tgit rebase HEAD^\n> > +'\n> > +\n> > test_done\n>\n"},{"id":"378269","messageId":"e332eb5f-0818-763c-9de0-889644446a35@gmail.com","threadId":"51391","inReplyTo":"nycvar.QRO.7.76.6.1906281342280.44@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 1/1] Let rebase.reschedulefailedexec only affect interactive rebases","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-06-28T13:44:07Z","receivedAt":"2019-06-28T13:44:13Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Junio and Dscho\n\nOn 28/06/2019 12:49, Johannes Schindelin wrote:\n> Hi Junio,\n> \n> On Thu, 27 Jun 2019, Junio C Hamano wrote:\n> \n>> \"Johannes Schindelin via GitGitGadget\" \n>> writes:\n>>>\n>>> -\tif (options.reschedule_failed_exec && !is_interactive(&options))\n>>> +\tif (reschedule_failed_exec > 0 && !is_interactive(&options))\n>>\n>> OK, it used to be that we got affected by what came from \"options\",\n>> which was read from the configuration. Now we only pay attention to\n>> the command line, which makes sense.\n>>\n>> At this point, we have already examined '-x' and called\n>> imply_interative(), so this should trigger for '-x' (without '-i'),\n>> right?\n> \n> Yes, at this point we have done all the parsing and automatic implying,\n> and check for incompatible options.\n> \n>>> \t\tdie(_(\"--reschedule-failed-exec requires an interactive rebase\"));\n>>\n>> I wonder if users understand that '-x' is \"an interctive rebase\".\n>> The documentation can read both ways, and one of these may want to\n>> be clarified.\n>>\n>> \t-x , --exec \n>> \t...\n>> \tThis uses the --interactive machinery internally, but it can\n>> \tbe run without an explicit --interactive.\n>>\n>> Is it saying that use of interactive machinery is an impelementation\n>> detail the users should not concern themselves (in which case, the\n>> message given to \"die()\" above is misleading---not a new problem\n>> with this patch, though)? Is it saying \"-x\" makes it plenty clear\n>> that the user wants interactive behaviour, so the users do not need\n>> to spell out --interactive in order to ask for it (in which case,\n>> \"die()\" message is fine, but \"... internally, but ...\" is\n>> misleading)?\n> \n> Hmm. What would you think about:\n> \n> \t\tdie(_(\"--reschedule-failed-exec requires --exec or --interactive\"));\n> \n\nI was wondering about requiring --exec with --reschedule-failed-exec \nrather than checking is_interactive() as that would be easier to \nunderstand. One potential problem is if someone has an alias that always \nsets --reschedule-failed-exec but does not always add --exec to the \ncommand line. We could just emit a warning along the lines of \"ignoring \n--reschedule-failed-exec without --exec\". I'm not sure that we really \nneed to error out, unless we think that the missing --exec is an \nindication that the user forgot --exec and so would not want the rebase \nto start, in which case just dying on --reschedule-failed-exec without \n--exec would be fine.\n\nBest Wishes\n\nPhillip\n\n> \n> It is still not _complete_, but at least it should be a ton less\n> confusing.\n> \n>>> +\tif (reschedule_failed_exec >= 0)\n>>> +\t\toptions.reschedule_failed_exec = reschedule_failed_exec;\n>>\n>> OK, here we recover the bit that is only stored in a local variable\n>> and pass it into cmd_rebase__interactive() machinery via the options\n>> structure, which lets the codepath after this point oblivious to\n>> this change, which is good ;-).\n>>\n>>> \tif (options.git_am_opts.argc) {\n>>> \t\t/* all am options except -q are compatible only with --am */\n>>> diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh\n>>> index bdaa511bb0..4eff14dae5 100755\n>>> --- a/t/t3418-rebase-continue.sh\n>>> +++ b/t/t3418-rebase-continue.sh\n>>> @@ -265,4 +265,12 @@ test_expect_success '--reschedule-failed-exec' '\n>>> \ttest_i18ngrep \"has been rescheduled\" err\n>>> '\n>>>\n>>> +test_expect_success 'rebase.reschedulefailedexec only affects `rebase -i`' '\n>>> +\ttest_config rebase.reschedulefailedexec true &&\n>>> +\ttest_must_fail git rebase -x false HEAD^ &&\n>>\n>> These three lines gives us a concise summary of this patch ;-)\n>>\n>> - The test title can serve as a starting point for a much better\n>> patch title.\n>>\n>> - We trigger for '-x' without requiring '-i'.\n> \n> I changed the oneline to\n> \n> \trebase --am: ignore rebase.reschedulefailedexec\n> \n> This gives credit to the implementation details, as appropriate for commit\n> messages, and the error message still tries to be as helpful as possible\n> for users (who do not necessarily need to know that there are two\n> backends).\n> \n> At this point, I am _really_ glad that we only have two backends left (for\n> all practical purposes, I don't count --preserve-merges).\n> \n> Ciao,\n> Dscho\n> \n>>> +\tgrep \"^exec false\" .git/rebase-merge/git-rebase-todo &&\n>>> +\tgit rebase --abort &&\n>>> +\tgit rebase HEAD^\n>>> +'\n>>> +\n>>> test_done\n>>\n"},{"id":"378288","messageId":"xmqq4l498irq.fsf@gitster-ct.c.googlers.com","threadId":"51391","inReplyTo":"e332eb5f-0818-763c-9de0-889644446a35@gmail.com","subject":"Re: [PATCH 1/1] Let rebase.reschedulefailedexec only affect interactive rebases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-06-28T22:08:57Z","receivedAt":"2019-06-28T22:09:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n>>> I wonder if users understand that '-x' is \"an interctive rebase\".\n>>> The documentation can read both ways, and one of these may want to\n>>> be clarified.\n>>>\n>>> \t-x , --exec \n>>> \t...\n>>> \tThis uses the --interactive machinery internally, but it can\n>>> \tbe run without an explicit --interactive.\n>>>\n>>> Is it saying that use of interactive machinery is an impelementation\n>>> detail the users should not concern themselves (in which case, the\n>>> message given to \"die()\" above is misleading---not a new problem\n>>> with this patch, though)? Is it saying \"-x\" makes it plenty clear\n>>> that the user wants interactive behaviour, so the users do not need\n>>> to spell out --interactive in order to ask for it (in which case,\n>>> \"die()\" message is fine, but \"... internally, but ...\" is\n>>> misleading)?\n>>\n>> Hmm. What would you think about:\n>>\n>> \t\tdie(_(\"--reschedule-failed-exec requires --exec or --interactive\"));\n\nI was leaning towards admitting that the use of the interactive\nmachinery in \"-x\" is not merely an implementation detail and fixing\nthe documentation, leaving the die() message in the patch as-is.\n\nBut ...\n\n> I was wondering about requiring --exec with --reschedule-failed-exec\n> rather than checking is_interactive() as that would be easier to\n> understand.\n\n... I find this a reasonable way to think about the issue. The\noption only matters when we are doing \"--exec\". And the usual\nconvenience measure we'd use, i.e. with --reschedule-failed-exec we\nconsider that we are implicitly in --exec mode, would not work\nbecause there is no default \"command\" to be executed.\n\n> One potential problem is if someone has an alias that\n> always sets --reschedule-failed-exec but does not always add --exec to\n> the command line.\n\nSuch a use case would be hitting this die() already without this\ntopic, wouldn't it? In which case we can say there is no \"someone\"\nwith such an alias.\n"},{"id":"378303","messageId":"5862CF8D-2D4E-40B4-81A1-4BA9B59AC52E@commonkarma.org","threadId":"51391","inReplyTo":"xmqq4l498irq.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/1] Let rebase.reschedulefailedexec only affect interactive rebases","fromName":"Vas Sudanagunta","fromEmail":"vas@commonkarma.org","sentAt":"2019-06-28T23:52:59Z","receivedAt":"2019-06-28T23:53:08Z","isPatch":true,"sender":{"key":"vas@commonkarma.org","avatar":null},"body":"Hi, original reporter of the issue here. Some thoughts from a user perspective, from outside the black box of “the machinery\":\n\n\nIt seems unintuitive that including a command to execute between each rebase merge implies an interactive rebase. It’s no more interactive than a sequence of rebase merges without intervening commands. A command failure interrupts a rebase just as a merge conflict does. If the fact that a rebase can be interrupts mid-sequence, then all rebases are interactive. \n\nThe intuitive behavior for “rebase.reschedulefailedexec” is that it has no impact if nothing is scheduled. It has nothing to do with interactivity. \n\nvas\n\n\n\nOn Jun 28, 2019, at 6:08 PM, Junio C Hamano wrote:\n\nPhillip Wood writes:\n\n>>> I wonder if users understand that '-x' is \"an interctive rebase\".\n>>> The documentation can read both ways, and one of these may want to\n>>> be clarified.\n>>> \n>>> \t-x , --exec \n>>> \t...\n>>> \tThis uses the --interactive machinery internally, but it can\n>>> \tbe run without an explicit --interactive.\n>>> \n>>> Is it saying that use of interactive machinery is an impelementation\n>>> detail the users should not concern themselves (in which case, the\n>>> message given to \"die()\" above is misleading---not a new problem\n>>> with this patch, though)? Is it saying \"-x\" makes it plenty clear\n>>> that the user wants interactive behaviour, so the users do not need\n>>> to spell out --interactive in order to ask for it (in which case,\n>>> \"die()\" message is fine, but \"... internally, but ...\" is\n>>> misleading)?\n>> \n>> Hmm. What would you think about:\n>> \n>> \t\tdie(_(\"--reschedule-failed-exec requires --exec or --interactive\"));\n\nI was leaning towards admitting that the use of the interactive\nmachinery in \"-x\" is not merely an implementation detail and fixing\nthe documentation, leaving the die() message in the patch as-is.\n\nBut ...\n\n> I was wondering about requiring --exec with --reschedule-failed-exec\n> rather than checking is_interactive() as that would be easier to\n> understand.\n\n... I find this a reasonable way to think about the issue. The\noption only matters when we are doing \"--exec\". And the usual\nconvenience measure we'd use, i.e. with --reschedule-failed-exec we\nconsider that we are implicitly in --exec mode, would not work\nbecause there is no default \"command\" to be executed.\n\n> One potential problem is if someone has an alias that\n> always sets --reschedule-failed-exec but does not always add --exec to\n> the command line.\n\nSuch a use case would be hitting this die() already without this\ntopic, wouldn't it? In which case we can say there is no \"someone\"\nwith such an alias.\n\n"},{"id":"378342","messageId":"70648c5b-00ff-d7aa-f1f7-8bb7bf663829@gmail.com","threadId":"51391","inReplyTo":"xmqq4l498irq.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/1] Let rebase.reschedulefailedexec only affect interactive rebases","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-06-30T10:03:15Z","receivedAt":"2019-06-30T10:04:09Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 28/06/2019 23:08, Junio C Hamano wrote:\n> Phillip Wood writes:\n> \n>>>> I wonder if users understand that '-x' is \"an interctive rebase\".\n>>>> The documentation can read both ways, and one of these may want to\n>>>> be clarified.\n>>>>\n>>>> \t-x , --exec \n>>>> \t...\n>>>> \tThis uses the --interactive machinery internally, but it can\n>>>> \tbe run without an explicit --interactive.\n>>>>\n>>>> Is it saying that use of interactive machinery is an impelementation\n>>>> detail the users should not concern themselves (in which case, the\n>>>> message given to \"die()\" above is misleading---not a new problem\n>>>> with this patch, though)? Is it saying \"-x\" makes it plenty clear\n>>>> that the user wants interactive behaviour, so the users do not need\n>>>> to spell out --interactive in order to ask for it (in which case,\n>>>> \"die()\" message is fine, but \"... internally, but ...\" is\n>>>> misleading)?\n>>>\n>>> Hmm. What would you think about:\n>>>\n>>> \t\tdie(_(\"--reschedule-failed-exec requires --exec or --interactive\"));\n> \n> I was leaning towards admitting that the use of the interactive\n> machinery in \"-x\" is not merely an implementation detail and fixing\n> the documentation, leaving the die() message in the patch as-is.\n\nI'd really like to try and hide that as much as possible from users - \nit's just confusing. (though sometimes we can't)\n> But ...\n> \n>> I was wondering about requiring --exec with --reschedule-failed-exec\n>> rather than checking is_interactive() as that would be easier to\n>> understand.\n> \n> ... I find this a reasonable way to think about the issue. The\n> option only matters when we are doing \"--exec\". And the usual\n> convenience measure we'd use, i.e. with --reschedule-failed-exec we\n> consider that we are implicitly in --exec mode, would not work\n> because there is no default \"command\" to be executed.\n >\n>> One potential problem is if someone has an alias that\n>> always sets --reschedule-failed-exec but does not always add --exec to\n>> the command line.\n> \n> Such a use case would be hitting this die() already without this\n> topic, wouldn't it? In which case we can say there is no \"someone\"\n> with such an alias.\n\nIt depends what else the alias includes, if it also includes \n-i/-k/-r/--signoff then it wont have been dying but will if we start \nrequiring --exec and they don't set that.\n\nBest Wishes\n\nPhillip\n"},{"id":"378361","messageId":"nycvar.QRO.7.76.6.1907011346411.44@tvgsbejvaqbjf.bet","threadId":"51391","inReplyTo":"0F745CE4-3203-4447-B1D5-937CCDCC64C7@commonkarma.org","subject":"Re: [PATCH 1/1] Let rebase.reschedulefailedexec only affect interactive rebases","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-07-01T11:48:12Z","receivedAt":"2019-07-01T11:47:55Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 28 Jun 2019, Vas Sudanagunta wrote:\n\n> It seems unintuitive that including a command to execute between each\n> rebase merge implies an interactive rebase. It’s no more interactive\n> than a sequence of rebase merges without intervening commands. A command\n> failure interrupts a rebase just as a merge conflict does. If the fact\n> that a rebase can be interrupts mid-sequence, then all rebases are\n> interactive.\n\nIndeed. I would consider it an implementation detail that the `--exec`\noption uses the interactive rebase backend, and I would hate to force\nusers to know about this.\n\nCiao,\nJohannes\n"},{"id":"378362","messageId":"nycvar.QRO.7.76.6.1907011348280.44@tvgsbejvaqbjf.bet","threadId":"51391","inReplyTo":"xmqq4l498irq.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/1] Let rebase.reschedulefailedexec only affect interactive rebases","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-07-01T11:49:53Z","receivedAt":"2019-07-01T11:49:34Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 28 Jun 2019, Junio C Hamano wrote:\n\n> Phillip Wood writes:\n>\n> > I was wondering about requiring --exec with --reschedule-failed-exec\n> > rather than checking is_interactive() as that would be easier to\n> > understand.\n>\n> ... I find this a reasonable way to think about the issue.\n\nI don't: the `exec` command in interactive rebases' todo lists predates\nthe `--exec` option by quite some time IIRC.\n\nIn other words, `--reschedule-failed-exec` can very well influence a `git\nrebase -i` without `--exec`.\n\nCiao,\nDscho\n"},{"id":"378363","messageId":"nycvar.QRO.7.76.6.1907011350210.44@tvgsbejvaqbjf.bet","threadId":"51391","inReplyTo":"70648c5b-00ff-d7aa-f1f7-8bb7bf663829@gmail.com","subject":"Re: [PATCH 1/1] Let rebase.reschedulefailedexec only affect interactive rebases","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-07-01T11:52:50Z","receivedAt":"2019-07-01T11:52:33Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 30 Jun 2019, Phillip Wood wrote:\n\n> On 28/06/2019 23:08, Junio C Hamano wrote:\n> > Phillip Wood writes:\n> >\n> > > One potential problem is if someone has an alias that always sets\n> > > --reschedule-failed-exec but does not always add --exec to the\n> > > command line.\n> >\n> > Such a use case would be hitting this die() already without this\n> > topic, wouldn't it? In which case we can say there is no \"someone\"\n> > with such an alias.\n>\n> It depends what else the alias includes, if it also includes\n> -i/-k/-r/--signoff then it wont have been dying but will if we start\n> requiring --exec and they don't set that.\n\nThe entire reasoning behind the config variable was that it would allow\nchanging the behavior just in case that the interactive rebase backend was\nin use.\n\nIn other words: I tried to introduce that config variable to prevent\npeople from having afore-mentioned problems with the command-line option.\n\nTherefore, from my perspective, it makes more sense to define such an\nalias using `-c ...` than using `--reschedule-failed-exec`, unless the\nalias is definitely only intended to launch interactive rebases.\n\nCiao,\nDscho\n"},{"id":"378364","messageId":"pull.253.v2.git.gitgitgadget@gmail.com","threadId":"51391","inReplyTo":"pull.253.git.gitgitgadget@gmail.com","subject":"[PATCH v2 0/1] Make rebase.reschedulefailedexec less overzealous","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-07-01T11:58:14Z","receivedAt":"2019-07-01T11:58:18Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"This config setting is pretty useful, but it unfortunately stops all\nnon-interactive rebases with a bogus error message. This patch fixes that.\n\nReported via a commit comment on GitHub\n[https://github.com/git/git/commit/969de3ff0e0#commitcomment-33257187].\n\nChanges since v1:\n\n * Based on Junio's advice, the commit message was improved considerably.\n * The error message now also mentions --exec, so that users do not have to\n know that --exec implies the interactive backend.\n\nJohannes Schindelin (1):\n rebase --am: ignore rebase.reschedulefailedexec\n\n builtin/rebase.c | 10 +++++++---\n t/t3418-rebase-continue.sh | 8 ++++++++\n 2 files changed, 15 insertions(+), 3 deletions(-)\n\n\nbase-commit: e11ff8975bedc0aae82632c3cb72578c3d7fc0b2\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-253%2Fdscho%2Freschedule-failed-exec-gently-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-253/dscho/reschedule-failed-exec-gently-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/253\n\nRange-diff vs v1:\n\n 1: fab124da41 ! 1: beaeb24bc0 Let rebase.reschedulefailedexec only affect interactive rebases\n @@ -1,9 +1,13 @@\n Author: Johannes Schindelin \n \n - Let rebase.reschedulefailedexec only affect interactive rebases\n + rebase --am: ignore rebase.reschedulefailedexec\n \n - It does not make sense to stop non-interactive rebases when that config\n - setting is set to `true`.\n + The `exec` command is specific to the interactive backend, therefore it\n + does not make sense for non-interactive rebases to heed that config\n + setting.\n +\n + We still want to error out if a non-interactive rebase is started with\n + `--reschedule-failed-exec`, of course.\n \n Reported by Vas Sudanagunta via:\n https://github.com/git/git/commit/969de3ff0e0#commitcomment-33257187\n @@ -35,8 +39,10 @@\n \t}\n \n -\tif (options.reschedule_failed_exec && !is_interactive(&options))\n +-\t\tdie(_(\"--reschedule-failed-exec requires an interactive rebase\"));\n +\tif (reschedule_failed_exec > 0 && !is_interactive(&options))\n - \t\tdie(_(\"--reschedule-failed-exec requires an interactive rebase\"));\n ++\t\tdie(_(\"--reschedule-failed-exec requires \"\n ++\t\t \"--exec or --interactive\"));\n +\tif (reschedule_failed_exec >= 0)\n +\t\toptions.reschedule_failed_exec = reschedule_failed_exec;\n \n\n-- \ngitgitgadget\n"},{"id":"378365","messageId":"beaeb24bc0cee7d1d80dcc377f135e80782567af.1561982294.git.gitgitgadget@gmail.com","threadId":"51391","inReplyTo":"pull.253.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v2 1/1] rebase --am: ignore rebase.reschedulefailedexec","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-07-01T11:58:15Z","receivedAt":"2019-07-01T11:58:18Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe `exec` command is specific to the interactive backend, therefore it\ndoes not make sense for non-interactive rebases to heed that config\nsetting.\n\nWe still want to error out if a non-interactive rebase is started with\n`--reschedule-failed-exec`, of course.\n\nReported by Vas Sudanagunta via:\nhttps://github.com/git/git/commit/969de3ff0e0#commitcomment-33257187\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/rebase.c | 10 +++++++---\n t/t3418-rebase-continue.sh | 8 ++++++++\n 2 files changed, 15 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 4839e52555..b33d7f74d6 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -834,6 +834,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \tstruct string_list strategy_options = STRING_LIST_INIT_NODUP;\n \tstruct object_id squash_onto;\n \tchar *squash_onto_name = NULL;\n+\tint reschedule_failed_exec = -1;\n \tstruct option builtin_rebase_options[] = {\n \t\tOPT_STRING(0, \"onto\", &options.onto_name,\n \t\t\t N_(\"revision\"),\n@@ -929,7 +930,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\tOPT_BOOL(0, \"root\", &options.root,\n \t\t\t N_(\"rebase all reachable commits up to the root(s)\")),\n \t\tOPT_BOOL(0, \"reschedule-failed-exec\",\n-\t\t\t &options.reschedule_failed_exec,\n+\t\t\t &reschedule_failed_exec,\n \t\t\t N_(\"automatically re-schedule any `exec` that fails\")),\n \t\tOPT_END(),\n \t};\n@@ -1227,8 +1228,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\tbreak;\n \t}\n \n-\tif (options.reschedule_failed_exec && !is_interactive(&options))\n-\t\tdie(_(\"--reschedule-failed-exec requires an interactive rebase\"));\n+\tif (reschedule_failed_exec > 0 && !is_interactive(&options))\n+\t\tdie(_(\"--reschedule-failed-exec requires \"\n+\t\t \"--exec or --interactive\"));\n+\tif (reschedule_failed_exec >= 0)\n+\t\toptions.reschedule_failed_exec = reschedule_failed_exec;\n \n \tif (options.git_am_opts.argc) {\n \t\t/* all am options except -q are compatible only with --am */\ndiff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh\nindex bdaa511bb0..4eff14dae5 100755\n--- a/t/t3418-rebase-continue.sh\n+++ b/t/t3418-rebase-continue.sh\n@@ -265,4 +265,12 @@ test_expect_success '--reschedule-failed-exec' '\n \ttest_i18ngrep \"has been rescheduled\" err\n '\n \n+test_expect_success 'rebase.reschedulefailedexec only affects `rebase -i`' '\n+\ttest_config rebase.reschedulefailedexec true &&\n+\ttest_must_fail git rebase -x false HEAD^ &&\n+\tgrep \"^exec false\" .git/rebase-merge/git-rebase-todo &&\n+\tgit rebase --abort &&\n+\tgit rebase HEAD^\n+'\n+\n test_done\n-- \ngitgitgadget\n"}]}