Re: [PATCH 5/6] sequencer: Expose API to cherry-picking machinery

Hi again,

Jonathan Nieder writes:

Ramkumar Ramachandra wrote:

quoted

--- a/sequencer.h
+++ b/sequencer.h

@@ -7,7 +7,32 @@

 #define SEQ_TODO_FILE        "sequencer/todo"
 #define SEQ_OPTS_FILE        "sequencer/opts"

+#define COMMIT_MESSAGE_INIT  { NULL, NULL, NULL, NULL, NULL };

I don't think this should be exposed.  The rest seems pretty sane,
though I haven't read the patch carefully.

Gah, my stupidity.  What sense does it make to expose
COMMIT_MESSAGE_INIT when 'struct commit_message' itself isn't?  Moved
to sequencer.c now, thanks.

Another thought.  I wonder if it's possible to leave
sequencer_parse_args() private to builtin/revert.c, making the split
a little more logical:

Yes, I'd like this too.  However, there are two new issues:
revert_or_cherry_pick_usage and action_name.  The former has two
callsites: one in prepare_revs (in sequencer.c) and another in
parse_args (in builtin/revert.c).  Unfortunately, action_name is even
more complicated to get rid of: the information from it is used all
over the place.  Attempting to attack the problems one by one:
1. Make prepare_revs and walk_revs_populate_todo return errors to be
handled by callers.  This is a fairly small patch that can come before
the big "code moving patch".
2. Duplicate action_name in both files.  I don't think it's too
serious, and we can fix this later.

It has been enormously annoying to work with this "code moving patch":
everytime I make some changes to the earlier patches, I have to
recreate this one by hand; rebase offers no help whatsoever.  After
throwing away code based on this patch several times, I learnt my
lesson and restricted my series to avoid building on this patch.  I
consider this a very serious glitch* and I'm interested in fixing it.
Thoughts?

Thanks.

* We don't track renames, and I fully subscribe to that design.
However, that doesn't prevent us from building small helpers.

-- Ram