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

> 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".

> 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.

> 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
>