Re: [PATCH 2/2] cherry-pick: fix --quit not deleting CHERRY_PICK_HEAD

From: Duy Nguyen <pclouds@gmail.com>
Date: Aug 15, 2018, 17:17 UTC
Message-ID: <CACsJy8Br==4ugLh5G2jU9NV1XRsH8iBmYkMZ2Ke_9jZgwuyJ7A@mail.gmail.com>
In-Reply-To: <CAGZ79kYo-icUHfezTMg77s_2GtKcgH6jysNTu11XOoL4UT-9bQ@mail.gmail.com>

On Wed, Aug 15, 2018 at 7:09 PM Stefan Beller <sbeller@google.com> wrote:

> Technically you would not need patch 1 in this series, as you could call
> remove_branch_state(void) as before that patch to do the same thing here.
> I guess that patch 1 is more of a drive by cleanup, then?

Yes.

> It looks a bit interesting as sequencer_remove_state actually
> takes no arguments and assumes the_repository, but I guess that is fine.

Don't worry. My effort to kill the_index will make sequencer.c take 'struct repository *' (its operations are so wide that passing just struct index_state * does not make sense).

> I wondered if we need to have this patch for 'a' as well, and it looks like
> which does a sequencer_rollback, which is just some logic before
> attempting sequencer_remove_state. So I'd think remove_branch_state
> could be done in sequencer_remove_state as well?

sequencer_rollback does not need this remove_branch_state() line because it calls reset_for_rollback() which does this deletion. Patch 1/1 kinda hints at that because it touches all remove_branch_state() ;-)

Another part of the reason I did not put remove_branch_state() in sequencer_remove_state() is I was not sure if it could be used in a different way (I did not study all of its call sites and am not very familiar with sequencer code).

Show 6 quoted lines

> Or are there functions that would definitely not want sequencer_remove_state
> after sequencer_remove_state?
>
> Going down on that I just realize sequencer_remove_state could also
> be returning void, as of now it always returns 0, so the condition here
> with !ret is just confusing the reader?

-- 
Duy