From: Phillip Wood Date: Thu, 15 Apr 2021 15:24:30 GMT Subject: Re: [PATCH v2 0/2] rebase: don't override --no-reschedule-failed-exec with config Message-ID: <63774b95-7de2-b160-385b-6c7d99e867ed@gmail.com> In-Reply-To: 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 >