Re: [PATCH 3/3] rebase: don't override --no-reschedule-failed-exec with config

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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@gmail.com

>> 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@gmail.com>
>> ---
>>   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
>>