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

[Ramkumar Ramachandra <artagnon@gmail.com>]

Hi again,

Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
>> Move code from builtin/revert.c to sequencer.c and expose a public API
>> without making any functional changes.  Although it is useful only to
>> existing callers of cherry-pick and revert now, this patch lays the
>> foundation for future expansion.
>
> :)  It sounds like you are running a business.

*laughs* Now that you mention it, it certainly does :)

>> +++ b/sequencer.c
>> @@ -1,13 +1,656 @@
>> [...]
>> +#define COMMIT_MESSAGE_INIT  { NULL, NULL, NULL, NULL, NULL };

I'll get rid of this.

>> +static const char *action_keyword(const struct replay_opts *opts)
>> [...]
>
> Another (non-functional) change.  Probably (?) this renaming has a
> good reason to be part of this patch, but it should definitely be
> mentioned in the commit message.

Yes, I want the rename to be part of this patch (see Daniel's comment
and my agreement).  Will clarify in the commit message.

> git_path() calls vsnprintf which clobbers errno, so depending on the
> platform this can print messages like
>
>        fatal: Could not open '.git/CHERRY_PICK_HEAD' for writing: Success
>
> The natural fix would be to add a local for it (as a separate patch).
> Sorry I missed this when the code first arrived.

Ugh, yet another "bugfix patch" to queue near the beginning of the
series.  Thanks for catching this.

>> +static struct tree *empty_tree(void)
>> [...]
>
> This tree is leaked (for example if you cherry-pick a sequence of
> root commits).

This is not something I introduced -- it can wait until later, no?

>> +static int fast_forward_to(const unsigned char *to, const unsigned char *from)
>> [...]
>
> The exit code here violates the usual "exit with status 128 for
> errors other than conflicts" rule.  Perhaps it should be changed to
> "return -1" in a separate patch (to accompany the patch that returns
> error() instead of die()-ing so often to allow callers to give
> additional context to errors from this machinery).

Great catch!  Will fix.

>> --- a/sequencer.h
>> +++ b/sequencer.h
> [...]
>> +void sequencer_parse_args(int argc, const char **argv, struct replay_opts *opts);
>
> I thought sequencer_parse_args() wasn't being exposed.

Rebase fail, sorry.  There is no sequencer_parse_args().

> Except where noted above, I hope this is just simple code movement,
> but I haven't checked.  If I could be sure, it would be easier to
> review.

It is a simple code movement.  Is there something I can do to help?

-- Ram