Re: [PATCH 10/13] revert: Persist data for continuation

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Jun 21, 2011, 17:11 UTC
Message-ID: <20110621171139.GO15461@elie>
In-Reply-To: <1308661489-20080-11-git-send-email-artagnon@gmail.com>

Ramkumar Ramachandra wrote:

Show 7 quoted lines

> Ever since v1.7.2-rc1~4^2~7 (revert: allow cherry-picking more than
> one commit, 2010-06-02), a single invocation of "git cherry-pick" or
> "git revert" can perform picks of several individual commits.  To
> implement features like "--continue" to continue the whole operation,
> we will need to store some information about the state and the plan at
> the beginning.  Introduce a ".git/sequencer/head" file to store this
> state, and ".git/sequencer/todo" file to store the plan.

Makes a lot of sense.

> Don't touch
> CHERRY_PICK_HEAD -- it will still be useful when a conflict is
> encountered.

I think there's some logical connector or something missing. Why would introducing a .git/sequencer dir involve touching CHERRY_PICK_HEAD?

Maybe the idea is to say: "The purpose of these new files is orthogonal to the existing CHERRY_PICK_HEAD" with some explanation of that.

Show 17 quoted lines

> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -13,6 +13,7 @@
>  #include "rerere.h"
>  #include "merge-recursive.h"
>  #include "refs.h"
> +#include "dir.h"
>  
>  /*
>   * This implements the builtins revert and cherry-pick.
> @@ -25,6 +26,10 @@
>   * Copyright (c) 2005 Junio C Hamano
>   */
>  
> +#define SEQ_DIR		git_path("sequencer")
> +#define HEAD_FILE	git_path("sequencer/head")
> +#define TODO_FILE	git_path("sequencer/todo")

I've failed to convince you in the past that these fake constants are scary, but believe me, they really are. Consider the following code --- what would you expect it to print? What does it actually print? (Hint: there's not one right answer.)

	printf("%s %s %s %s %s\n", SEQ_DIR, HEAD_FILE, TODO_FILE,
					SEQ_DIR, HEAD_FILE);

Show 20 quoted lines

> +static void walk_revs_populate_todo(struct commit_list **todo_list,
> +				struct replay_opts *opts)
>  {
>  	struct rev_info revs;
>  	struct commit *commit;
> +	struct commit_list *new_item;
> +	struct commit_list *cur = NULL;
> +
> +	/* Insert into todo_list in the same order */
> +	prepare_revs(&revs, opts);
> +	while ((commit = get_revision(&revs))) {
> +		new_item = xmalloc(sizeof(struct commit_list));
> +		new_item->item = commit;
> +		if (cur)
> +			cur->next = new_item;
> +		else
> +			*todo_list = new_item;
> +		cur = new_item;
> +	}
> +	cur->next = NULL;

The naive reader, perhaps stupidly, wonders: "why not use commit_list_insert"? A comment or something to explain "NEEDSWORK: expose this as commit_list_append" could help him.

[...]

> -	return 0;
> +	/* Sequence of picks finished successfully; cleanup by
> +	   removing the .git/sequencer directory */
> +	return cleanup_sequencer_data();

GNU-style comment seems to have snuck in.

Thanks; this one was pretty pleasant.