From: Phillip Wood Date: Mon, 25 Nov 2019 10:47:27 GMT Subject: Re: [PATCH] sequencer: don't re-read todo for revert and cherry-pick Message-ID: <8881749f-195e-7f24-6f38-3b0a53ef8e1e@gmail.com> In-Reply-To: Hi Junio On 25/11/2019 01:10, Junio C Hamano wrote: > Phillip Wood writes: > >>>> - } else if (check_todo && !res) { >>>> + } else if (is_rebase_i(opts) && check_todo && !res) { >>> >>> It is a bit sad that setting of check_todo is not something a single >>> helper function can decide, so that this is_rebase_i(opts) can be >>> taken into account when that helper function (the logical place >>> would be do_pick_commit()) decides to set (or not set) check_todo. >>> >>> Unfortunately, that is not sufficient, I suspect. Why did a47ba3c7 >>> ("rebase -i: check for updated todo after squash and reword", >>> 2019-08-19) decide to flip check_todo on when running TODO_EXEC? >> >> I'm not sure what you mean by this >> >> This is what I had before I saw Gábor's patch (the tests are pretty >> similar but I think we should check that the messages of all the picks >> are actually edited with --edit - that does not seem to be checked by >> the current tests)... > > I first thought that unsetting *check_todo in do_pick_commit(), when > !is_rebase_i(), was a clean solution. But sadly it is *not* a godo > equivalent to Gábor's patch, because check_todo can be set to true > without looking at is_rebase_i() in pick_commits() [*1*]. That only happens if we're running a rebase as exec commands are not used by cherry-pick and revert so the unconditional setting that remains after my suggested fix should be safe. Prior to a47ba3c7 ("rebase -i: check for updated todo after squash and reword", 2019-08-19) we always checked for an updated todo list after processing an exec command without checking is_rebase_i() Best Wishes Phillip > To ignore > that setting where the variable's value is used, the hunk we see > above in the beginning of this message is necessary. > > That was what I meant. I think the "This is what I had before" > patch matches my "I first thought" version, so we were on the same > page and both wrong ;-) > > Thanks. > > > [Footnote] > > *1* I still do not know why a47ba3c7 ("rebase -i: check for updated > todo after squash and reword", 2019-08-19) sets check_todo to true > without looking at is_rebase_i(). If this unconditonal setting in > TODO_EXEC did not exist, I think your "This is what I had before" > patch would have been equivalent to Gábor's patch.