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

Ramkumar Ramachandra [off-list ref] writes:

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

What does "Don't touch" here mean? Keep doing the same thing as before so
that people can rely on it? Or stop touching it? I presume the former, but
the description is suboptimal.

HEAD is to keep track of the original, or does it get updated during a
sequencer run?

+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;
+}

Yuck; you do not want if/else to do this (I am _not_ saying "you can do
that but here is a better way"). Have a variable that points at a pointer
of type "struct commit_list *", initialize it to where todo_list points
at, and then update to point at the next field of the new object.

Something like this:

	next = todo_list;
	while ((commit = ...) != NULL) {
		new = xcalloc(1, sizeof(struct commit_list));
                new->item = commit;
                *next = new;
                next = &new->next;
	}
	*next = NULL;

Doing it this way would avoid a segfault when get_revision() returned
nothing as an added bonus ;-).

+static void persist_head(const char *head)

s/persist/save/ perhaps?

+{
+	static struct lock_file head_lock;
+	struct strbuf buf = STRBUF_INIT;
+	int fd;
+

+	if (file_exists(SEQ_DIR)) {
+		if (!is_directory(SEQ_DIR) && remove_path(SEQ_DIR) < 0) {
+			strbuf_release(&buf);
+			die(_("Could not remove %s"), SEQ_DIR);
+		}
+	} else {
+		if (mkdir(SEQ_DIR, 0777) < 0) {
+			strbuf_release(&buf);
+			die_errno(_("Could not create sequencer directory '%s'."), SEQ_DIR);
+		}
+	}

Split this part into a separate helper.

+	fd = hold_lock_file_for_update(&head_lock, HEAD_FILE, LOCK_DIE_ON_ERROR);
+	strbuf_addf(&buf, "%s\n", head);
+	if (write_in_full(fd, buf.buf, buf.len) < 0)
+		die_errno(_("Could not write to %s."), HEAD_FILE);
+	if (commit_lock_file(&head_lock) < 0)
+		die(_("Error wrapping up %s"), HEAD_FILE);
+}

Is it sufficient to write the object name of the commit HEAD points at?
Would it be useful to also record the branch if the HEAD points at one?

+static void persist_todo(struct commit_list *todo_list, struct replay_opts *opts)

s/persist/save/ perhaps?

+{
+	static struct lock_file todo_lock;
+	struct strbuf buf = STRBUF_INIT;
+	int fd;
+	fd = hold_lock_file_for_update(&todo_lock, TODO_FILE, LOCK_DIE_ON_ERROR);

The current set of callers might always save head and then todo, but it
probably is a good idea to lift that restriction (which is why I suggested
to separate the initialization of seq-dir into a separate helper).

+static int pick_commits(struct replay_opts *opts)
+{
+	struct commit_list *todo_list = NULL;
+	unsigned char sha1[20];
+	struct commit_list *cur;
+	int res;
 
-	setenv(GIT_REFLOG_ACTION, me, 0);
 	read_and_refresh_cache(me, opts);
+	setenv(GIT_REFLOG_ACTION, me, 0);

Why?

-	prepare_revs(&revs, opts);
+	walk_revs_populate_todo(&todo_list, opts);
+	if (!get_sha1("HEAD", sha1))
+		persist_head(sha1_to_hex(sha1));
+	persist_todo(todo_list, opts);
 
-	while ((commit = get_revision(&revs))) {
-		int res = do_pick_commit(commit, opts);
+	for (cur = todo_list; cur; cur = cur->next) {
+		persist_todo(cur, opts);
+		res = do_pick_commit(cur->item, opts);
 		if (res)
 			return res;
 	}
 
-	return 0;
+	/* Sequence of picks finished successfully; cleanup by
+	   removing the .git/sequencer directory */

Just a style nit.

        /*
         * We write our multi-line comments
         * this way.
         */

+	return cleanup_sequencer_data();
 }