Re: [PATCH] sequencer: don't re-read todo for revert and cherry-pick
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Nov 25, 2019, 01:10 UTC
- Message-ID
- <xmqqo8x0zros.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <8c21662f-6548-a46e-9c87-eb364355cb78@gmail.com>
Phillip Wood <phillip.wood123@gmail.com> writes:
Show 18 quoted lines
>>> - } 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*]. 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.