Re: [PATCH v2 0/2] rebase: don't override --no-reschedule-failed-exec with config
Hi Ævar
On 09/04/2021 09:01, Ævar Arnfjörð Bjarmason wrote:
Show 19 quoted lines
> 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
Show 63 quoted lines
> 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
>