On Mon, Mar 29 2021, Phillip Wood wrote: > Hi Ævar > > On 22/03/2021 11:48, Ævar Arnfjörð Bjarmason wrote: >> Fix a bug in how --no-reschedule-failed-exec interacts with >> rebase.rescheduleFailedExec=true being set in the config. Before this >> change the --no-reschedule-failed-exec config option would be >> overridden by the config. >> This bug happened because of the particulars of how "rebase" works >> v.s. most other git commands when it comes to parsing options and >> config: >> When we read the config and parse the CLI options we correctly >> prefer >> the --no-reschedule-failed-exec option over >> rebase.rescheduleFailedExec=true in the config. So far so good. >> However the --reschedule-failed-exec option doesn't take effect when >> the rebase starts (we'd just create a >> ".git/rebase-merge/reschedule-failed-exec" file if it was true). It >> only takes effect when the exec command fails, and the user wants to >> run "rebase --continue". > > The exec command is rescheduled in the todo file as soon as it fails, > we do not wait for the user to run 'rebase --continue' to reschedule > it. However if it still fails after restarting or a later exec fails > we have the problem you describe. Right, as noted in [1] I grokked those internals eventually, but the commit message is written from the viewpoint of a hypothetical user... B.t.w. if you've had a chance to read [1] over it would be interesting to get some thoughts on that rambling. So far I only managed to upset Johannes :) 1. http://lore.kernel.org/git/cover.1616411973.git.avarab@xxxxxxxxx >> At that point we'll have forgotten that we asked for >> --no-reschedule-failed-exec at the start, and will happily re-read the >> config. >> We'll then see that rebase.rescheduleFailedExec=true is set. At that >> point we have no record of having set --no-reschedule-failed-exec >> earlier. So the config will effectively override the user having >> explicitly disabled the option on the command-line. >> Even more confusingly: Since rebase accepts different options based >> on >> its state there wasn't even a way to get around this with "rebase >> --continue --no-reschedule-failed-exec" (but you could of course set >> the config with "rebase -c ..."). >> I think the least bad way out of this is to declare that for such >> options and config whatever we decide at the beginning of the rebase >> goes. So we'll now always create either a "reschedule-failed-exec" or >> a "no-reschedule-failed-exec file at the start, not just the former if >> we decided we wanted the feature. > > Thanks for working on this and for the detailed commit message. I'm > not entirely convinced we want yet another state file in > .git/rebase-merge. We could we start writing the setting to the file > rather than having different files for whether the option is on or > off. If we use the contents of the file it could be -1 for 'use > config', 0 'off', 1 'on'. The downside is that starting 'rebase > --no-reschedule-failed-exec' with a new version of git and then > continuing with an old version would do the wrong thing. Yes I suppose, but it seems much simpler indeed to just represent this sort of tri-state as a ENOENT/no-FOO/FOO neither exists + file pair with how the current code is set up, especially because (as you note) we'd need to phase-in any writing of the content across multiple versions or something, least in-progress rebases across versions subtly behave weirdly. Well, in this case it's not such a big deal, but I'd rather not establish the pattern for something that *does* matter. > Best Wishes > > Phillip > >> With this new worldview you can no longer change the setting once a >> rebase has started except by manually removing the state files >> discussed above. I think making it work like that is the the least >> confusing thing we can do. >> In the future we might want to learn to change the setting in the >> middle by combining "--edit-todo" with >> "--[no-]reschedule-failed-exec", we currently don't support combining >> those options, or any other way to change the state in the middle of >> the rebase short of manually editing the files in >> ".git/rebase-merge/*". >> The bug being fixed here originally came about because of a >> combination of the behavior of the code added in d421afa0c66 (rebase: >> introduce --reschedule-failed-exec, 2018-12-10) and the addition of >> the config variable in 969de3ff0e0 (rebase: add a config option to >> default to --reschedule-failed-exec, 2018-12-10). >> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> >> --- >> Documentation/git-rebase.txt | 8 ++++++++ >> sequencer.c | 5 +++++ >> t/t3418-rebase-continue.sh | 25 +++++++++++++++++++++++++ >> 3 files changed, 38 insertions(+) >> diff --git a/Documentation/git-rebase.txt >> b/Documentation/git-rebase.txt >> index a0487b5cc58..b48e6225769 100644 >> --- a/Documentation/git-rebase.txt >> +++ b/Documentation/git-rebase.txt >> @@ -622,6 +622,14 @@ See also INCOMPATIBLE OPTIONS below. >> --no-reschedule-failed-exec:: >> Automatically reschedule `exec` commands that failed. This only makes >> sense in interactive mode (or when an `--exec` option was provided). >> ++ >> +Even though this option applies once a rebase is started, it's set for >> +the whole rebase at the start based on either the >> +`rebase.rescheduleFailedExec` configuration (see linkgit:git-config[1] >> +or "CONFIGURATION" below) or whether this option is >> +provided. Otherwise an explicit `--no-reschedule-failed-exec` at the >> +start would be overridden by the presence of >> +`rebase.rescheduleFailedExec=true` configuration. >> INCOMPATIBLE OPTIONS >> -------------------- >> diff --git a/sequencer.c b/sequencer.c >> index 848204d3dc3..59735fdff62 100644 >> --- a/sequencer.c >> +++ b/sequencer.c >> @@ -164,6 +164,7 @@ static GIT_PATH_FUNC(rebase_path_strategy, "rebase-merge/strategy") >> static GIT_PATH_FUNC(rebase_path_strategy_opts, "rebase-merge/strategy_opts") >> static GIT_PATH_FUNC(rebase_path_allow_rerere_autoupdate, "rebase-merge/allow_rerere_autoupdate") >> static GIT_PATH_FUNC(rebase_path_reschedule_failed_exec, "rebase-merge/reschedule-failed-exec") >> +static GIT_PATH_FUNC(rebase_path_no_reschedule_failed_exec, "rebase-merge/no-reschedule-failed-exec") >> static GIT_PATH_FUNC(rebase_path_drop_redundant_commits, "rebase-merge/drop_redundant_commits") >> static GIT_PATH_FUNC(rebase_path_keep_redundant_commits, "rebase-merge/keep_redundant_commits") >> @@ -2672,6 +2673,8 @@ static int read_populate_opts(struct >> replay_opts *opts) >> if (file_exists(rebase_path_reschedule_failed_exec())) >> opts->reschedule_failed_exec = 1; >> + else if (file_exists(rebase_path_no_reschedule_failed_exec())) >> + opts->reschedule_failed_exec = 0; >> if (file_exists(rebase_path_drop_redundant_commits())) >> opts->drop_redundant_commits = 1; >> @@ -2772,6 +2775,8 @@ int write_basic_state(struct replay_opts *opts, const char *head_name, >> write_file(rebase_path_ignore_date(), "%s", ""); >> if (opts->reschedule_failed_exec) >> write_file(rebase_path_reschedule_failed_exec(), "%s", ""); >> + else >> + write_file(rebase_path_no_reschedule_failed_exec(), "%s", ""); >> return 0; >> } >> diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh >> index ea14ef496cb..9553d969646 100755 >> --- a/t/t3418-rebase-continue.sh >> +++ b/t/t3418-rebase-continue.sh >> @@ -291,4 +291,29 @@ test_expect_success 'rebase.rescheduleFailedExec only affects `rebase -i`' ' >> git rebase HEAD^ >> ' >> +test_expect_success 'rebase.rescheduleFailedExec=true & >> --no-reschedule-failed-exec' ' >> + test_when_finished "git rebase --abort" && >> + test_when_finished "test_unconfig rebase.rescheduleFailedExec" && >> + test_config rebase.rescheduleFailedExec true && >> + test_must_fail git rebase -x false --no-reschedule-failed-exec HEAD~2 && >> + test_must_fail git rebase --continue 2>err && >> + ! grep "has been rescheduled" err >> +' >> + >> +test_expect_success 'new rebase.rescheduleFailedExec=true setting in an ongoing rebase is ignored' ' >> + test_when_finished "git rebase --abort" && >> + test_must_fail git rebase -x false HEAD~2 && >> + test_when_finished "test_unconfig rebase.rescheduleFailedExec" && >> + test_config rebase.rescheduleFailedExec true && >> + test_must_fail git rebase --continue 2>err && >> + ! grep "has been rescheduled" err >> +' >> + >> +test_expect_success 'there is no --no-reschedule-failed-exec in an ongoing rebase' ' >> + test_when_finished "git rebase --abort" && >> + test_must_fail git rebase -x false HEAD~2 && >> + test_expect_code 129 git rebase --continue --no-reschedule-failed-exec && >> + test_expect_code 129 git rebase --edit-todo --no-reschedule-failed-exec >> +' >> + >> test_done >>
