Re: [PATCH 6/8] revert: Introduce head, todo, done files to persist state

Ramkumar Ramachandra wrote:

A cherry-pick/ revert operation consists of several smaller steps.
Later in the series, we would like to be able to resume a failed
operation.

When introducing jargon, it is hard to make the intent perfectly
clear.  I suppose what this means is:

 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
 allow "git cherry-pick --abort" to cancel and "git cherry-pick
 --continue" to resume the entire command, we will need to store some
 information about the state and the plan at the beginning.

Introduce a "head" file to make note of the HEAD when
the operation stated (so that the operation can be aborted), a "todo"
file to keep the list of the steps to be performed, and a "done" file
to keep a list of steps that have completed successfully.  The format
of these files is similar to the one used by the "rebase -i" process.

s/stated/started/ :)  Makes some sense, aside from that.

It would be more conventional to use all-caps symref-like names, like
MULTIPLE_CHERRY_PICK_ORIG_HEAD, CHERRY_PICK_TODO, and
CHERRY_PICK_DONE, or to put these files in a subdirectory (oh, they're
already in a subdirectory?  Why didn't you mention that? :)).

By the way, what is .git/sequencer/done used for?

These seeming constants that call a function are kind of scary.

Is this internal state or for the user?  If it is internal state, I'd
naïvely have expected a sequence of 40-character hexadecimal lines,
perhaps with human-readable names like "topic~3" for the sake of
error messages if git knows about them.

+static int persist_initialize(unsigned char *head)
+{
+	struct strbuf buf = STRBUF_INIT;
+	int fd;
+
+	if (!file_exists(SEQ_PATH) && mkdir(SEQ_PATH, 0777)) {

What if .git/sequencer exists and is a file?  How does this interact
with "[core] sharedrepository" configuration?  What happens if
.git/sequencer contains some stale files --- if the power fails while
git is writing new files in .git/sequencer/, will the state be
confusing?

+		int err = errno;
+		strbuf_release(&buf);
+		error(_("Could not create sequencer directory '%s': %s"),
+			SEQ_PATH, strerror(err));
+		return -err;

Why does the caller care about which errno, and what is it going to
do with that information?

+	}
+
+	if ((fd = open(HEAD_FILE, O_WRONLY | O_CREAT | O_TRUNC, 0666)) < 0) {

More idiomatic in the git codebase to write:

	fd = open(...);
	if (fd < 0) {

+		int err = errno;
+		strbuf_release(&buf);
+		error(_("Could not open '%s' for writing: %s"),
+			HEAD_FILE, strerror(err));
+		return -err;

As above.  Why does the caller care about errno?  If backing out after
an error, I suppose it might make sense to rmdir .git/sequencer while
at it.

+	}
+
+	strbuf_addf(&buf, "%s", find_unique_abbrev(head, DEFAULT_ABBREV));

Why abbreviate?

+	write_or_whine(fd, buf.buf, buf.len, HEAD_FILE);

What happens and should happen on error?

[...]

+static int persist_todo_done(int res, struct commit_list *todo_list,
+			struct commit_list *done_list, struct replay_opts *opts)

This is about recording what has been done and what remains to
be done?  What does the res argument represent?

+{
+	struct strbuf buf = STRBUF_INIT;
+	int fd, res2;
+
+	if (!res)
+		return 0;
+
+	/* TODO file */
+	if ((fd = open(TODO_FILE, O_WRONLY | O_CREAT | O_TRUNC, 0666)) < 0) {

What happens if we are interrupted in the middle of writing this?

+		int err = errno;
+		strbuf_release(&buf);
+		error(_("Could not open '%s' for writing: %s"),
+			TODO_FILE, strerror(err));
+		return -err;

I don't think the caller should care which errno. :)

[...]

 static int pick_commits(struct replay_opts *opts)
 {
+	struct commit_list *done_list = NULL;
 	struct rev_info revs;
 	struct commit *commit;
+	unsigned char head[20];
 	int res;
 
+	if (get_sha1("HEAD", head))
+		return error(_("You do not have a valid HEAD"));

What should happen if I try to cherry-pick onto an unborn branch?  I
haven't checked what happens.

+
 	if ((res = read_and_refresh_cache(opts)) ||
-		(res = prepare_revs(&revs, opts)))
+		(res = prepare_revs(&revs, opts)) ||
+		(res = persist_initialize(head)))
 		return res;
 
-	while ((commit = get_revision(&revs)) &&
-		!(res = do_pick_commit(commit, opts)))
-		;
-
-	return res;
+	while ((commit = get_revision(&revs))) {
+		if (!(res = do_pick_commit(commit, opts)))
+			commit_list_insert(commit, &done_list);

This puts done_list in the reverse order that the commits were
cherry-picked.  Is that the intent?

+		else {
+			commit_list_insert(commit, &revs.commits);
+			break;
+		}
+	}
+	return persist_todo_done(res, revs.commits, done_list, opts);

A few potential trade-offs:

 - should cherry-pick record the state after every commit?  This would
   be safe against stray die() calls or segfaults but requires hitting
   the filesystem which might not be wanted if doing a run of
   cherry-picks in memory (though git is far from supporting such a
   "many cherry picks in core followed by checkout and packed
   collection of objects written to disk all at once" optimization
   anyway).

 - should we use O_TRUNC or O_APPEND to modify the state in-place or
   use separate files and rename them into place?  The latter is
   safer against sudden exit.

 - should we (perhaps optionally) fsync the state when commiting to it?
   I think no, but someone performing a rebase and running a test suite
   with the potential to crash the system between commits might appreciate
   the effort.