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

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Ævar

On 09/04/2021 09:01, Ævar Arnfjörð Bjarmason wrote:
> This fixes a bug where we read config & options when "git rebase -i -x
> make" starts, and will understand the "--no-reschedule-failed-exec"
> option, but once a command fails we'll lose track of having had a
> "--no-reschedule-failed-exec" and will happily re-read the
> "rebase.rescheduleFailedExec=true" config the user might have.
> 
> Thus we'll have config winning over explicit command-line
> options. This series fixes that bug.
> 
> Changes since v1:
> 
>   * I forgot how test_config works, and was doing a test_when_finished
>     "test_unconfig", which isn't needed, duh! Thanks to Phillip Wood
>     for that & other reviews on this series.
> 
>   * There was a discussion about how to add yet another rebase
>     machinery state file. I think the consensus is "let's just do it
>     like this", i.e. we could write some tri-state content to the file,
>     but we'd get into back-compat issues with other git versions.

These patches look fine to me now

Thanks

Phillip

> There was a discussiob about how to manage this whole state (a DB,
> JSON?) in another thread, let's kick the can of how exactly we store
> state down the line, and just fix the bug using the existing state
> saving convention for now.
> 
> Ævar Arnfjörð Bjarmason (2):
>    rebase tests: camel-case rebase.rescheduleFailedExec consistently
>    rebase: don't override --no-reschedule-failed-exec with config
> 
>   Documentation/git-rebase.txt |  8 ++++++++
>   sequencer.c                  |  5 +++++
>   t/t3418-rebase-continue.sh   | 27 +++++++++++++++++++++++++--
>   3 files changed, 38 insertions(+), 2 deletions(-)
> 
> Range-diff against v1:
> 1:  002dc72ee7 = 1:  e0dd2cb82a rebase tests: camel-case rebase.rescheduleFailedExec consistently
> 2:  330b33e7a8 < -:  ---------- rebase tests: use test_unconfig after test_config
> 3:  e00300d58d ! 2:  7991160de3 rebase: don't override --no-reschedule-failed-exec with config
>      @@ Commit message
>           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".
>      +    only takes effect when the exec command fails, at which point we'll
>      +    reschedule the failed "exec" command.
>       
>      -    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.
>      +    Since we only wrote out the positive
>      +    ".git/rebase-merge/reschedule-failed-exec" under
>      +    --reschedule-failed-exec, but nothing with --no-reschedule-failed-exec
>      +    we'll forget that we asked not to reschedule failed "exec", and would
>      +    happily re-read the config and see that
>      +    rebase.rescheduleFailedExec=true is set.
>       
>      -    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.
>      +    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
>      @@ t/t3418-rebase-continue.sh: test_expect_success 'rebase.rescheduleFailedExec onl
>        
>       +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 &&
>      @@ t/t3418-rebase-continue.sh: test_expect_success 'rebase.rescheduleFailedExec onl
>       +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
>