From: Ramkumar Ramachandra <artagnon@gmail.com>
Date: Wed, 10 Aug 2011 15:34:02 GMT
Subject: Re: [PATCH 4/5] sequencer: Expose code that handles files in .git/sequencer
Message-ID: <CALkWK0nJUTtNgCHF6CE2-w+3ZwdBrscBRw0e0L8wX86Za0G=DA@mail.gmail.com>
In-Reply-To: <20110810152126.GE31315@elie.gateway.2wire.net>

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