Hi again,
Jonathan Nieder writes:
> 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
--
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