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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Aug 14, 2011, 13:13 UTC
Message-ID: <20110814131303.GF18466@elie.gateway.2wire.net>
In-Reply-To: <1313310789-10216-7-git-send-email-artagnon@gmail.com>

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.

Show 11 quoted lines

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

Show 8 quoted lines

> +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).

Show 10 quoted lines

> +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).

Show 6 quoted lines

>  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

[...]

Show 7 quoted lines

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