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

From: Ramkumar Ramachandra <artagnon@gmail.com>
Date: Aug 13, 2011, 14:00 UTC
Message-ID: <CALkWK0migSRUmhPp0069O_NiRs3gQJbrU8QLdwUJ-kUYAsLz4Q@mail.gmail.com>
In-Reply-To: <20110811215650.GA13839@elie.gateway.2wire.net>

Hi again,

Jonathan Nieder writes:

Show 11 quoted lines

> Ramkumar Ramachandra wrote:
>> --- 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