On Wed, Aug 15, 2018 at 7:09 PM Stefan Beller <sbeller@xxxxxxxxxx> 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). > 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
