Hi Jonathan, Jonathan Nieder writes: > Ramkumar Ramachandra wrote: >> +struct replay_insn_list **replay_insn_list_append(enum replay_action action, >> + struct commit *operand, >> + struct replay_insn_list **next); >> +void sequencer_read_todo(struct replay_insn_list **todo_list); >> +void sequencer_read_opts(struct replay_opts **opts_ptr); >> +int sequencer_create_dir(void); >> +void sequencer_save_head(const char *head); >> +void sequencer_save_todo(struct replay_insn_list *todo_list); >> +void sequencer_save_opts(struct replay_opts *opts); > > This looks wrong. What is the expected calling sequence? Would it > be possible to expose fewer functions by moving more to sequencer.c? Thanks for the early review. Yes, I agree with you: No caller can make sense of this API; I want something like sequencer_start, sequencer_handle_conflict, and sequencer_end, but I'm not sure where to start. I would have liked to just move these functions without exposing them, but that would break cherry-pick/ revert. So, we're really faced with two choices: 1. Make this patch enormous by moving as well as refactoring everything into a beautiful public API. I suspect this won't be easy to review at all. 2. Keep this patch as it is, and introduce a future patch to clean up the API. This is the approach I was going for. I can't move these functions bit-by-bit either: a hypothetical sequencer_start will require *everything*. Could you suggest something? -- Ram -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html