Re: [PATCH] sequencer: don't re-read todo for revert and cherry-pick

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Junio

On 25/11/2019 01:10, Junio C Hamano wrote:
Phillip Wood <phillip.wood123@xxxxxxxxx> 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.




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux