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
--
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