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.
I guess I would suggest clarifying that you mean exposing further
functionality through this API for more callers, rather than just
generally bloating git further.
[...]
> +++ b/sequencer.c
> @@ -1,13 +1,656 @@
[...]
> +#define COMMIT_MESSAGE_INIT { NULL, NULL, NULL, NULL, NULL };
As much as I like this change (and I do, modulo the stray semicolon),
it does not belong in a patch where it can be unnoticed in the code
movement.
> +static const char *action_keyword(const struct replay_opts *opts)
> +{
> + return opts->action == REPLAY_REVERT ? "revert" : "cherry-pick";
> +}
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.
> +static void write_cherry_pick_head(struct commit *commit)
> +{
> + int fd;
> + struct strbuf buf = STRBUF_INIT;
> +
> + strbuf_addf(&buf, "%s\n", sha1_to_hex(commit->object.sha1));
> +
> + fd = open(git_path("CHERRY_PICK_HEAD"), O_WRONLY | O_CREAT, 0666);
> + if (fd < 0)
> + die_errno(_("Could not open '%s' for writing"),
> + git_path("CHERRY_PICK_HEAD"));
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.
> +static struct tree *empty_tree(void)
> +{
> + struct tree *tree = xcalloc(1, sizeof(struct tree));
> +
> + tree->object.parsed = 1;
> + tree->object.type = OBJ_TREE;
> + pretend_sha1_file(NULL, 0, OBJ_TREE, tree->object.sha1);
> + return tree;
This tree is leaked (for example if you cherry-pick a sequence of
root commits).
> +static int fast_forward_to(const unsigned char *to, const unsigned char *from)
> +{
> + struct ref_lock *ref_lock;
> +
> + read_cache();
> + if (checkout_fast_forward(from, to))
> + exit(1); /* the callee should have complained already */
> + ref_lock = lock_any_ref_for_update("HEAD", from, 0);
> + return write_ref_sha1(ref_lock, to, "cherry-pick");
> +}
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).
> void remove_sequencer_state(int aggressive)
> {
> struct strbuf seq_dir = STRBUF_INIT;
> struct strbuf seq_old_dir = STRBUF_INIT;
> -
> strbuf_addf(&seq_dir, "%s", git_path(SEQ_DIR));
Unrelated change snuck in?
> --- a/sequencer.h
> +++ b/sequencer.h
[...]
> @@ -25,4 +49,7 @@ struct replay_insn_list {
> */
> void remove_sequencer_state(int aggressive);
>
> +void sequencer_parse_args(int argc, const char **argv, struct replay_opts *opts);
> +int sequencer_pick_revisions(struct replay_opts *opts);
> +
I thought sequencer_parse_args() wasn't being exposed.
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.
Ciao,
Jonathan
--
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