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

Hi,

Jonathan Nieder writes:

Ramkumar Ramachandra wrote:

quoted

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.

Thanks for digging that up for me.  I'll try to reword it in the next
iteration without introducing more jargon, if that's preferred.

quoted

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? :)).

I'll mention that in the commit message next time.

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

I don't know :p
I just added it because "rebase -i" uses it too, although I can't find
a definite usecase for it.

quoted

--- a/builtin/revert.c
+++ b/builtin/revert.c

@@ -25,6 +26,13 @@
  * Copyright (c) 2005 Junio C Hamano
  */
 
+#define SEQ_DIR "sequencer"
+
+#define SEQ_PATH	git_path(SEQ_DIR)
+#define HEAD_FILE	git_path(SEQ_DIR "/head")
+#define TODO_FILE	git_path(SEQ_DIR "/todo")
+#define DONE_FILE	git_path(SEQ_DIR "/done")

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

Uh, you'd prefer seeing it literally spelt out over and over again?

quoted

@@ -629,21 +637,118 @@ static int read_and_refresh_cache(struct replay_opts *opts)
 	return 0;
 }
 
+static int format_todo(struct strbuf *buf, struct commit_list *list,
+			struct replay_opts *opts)
+{
+	struct commit_list *cur = NULL;
+	struct commit_message msg = { NULL, NULL, NULL, NULL, NULL };
+	const char *sha1 = NULL;
+	const char *action;
+
+	action = (opts->action == REVERT ? "revert" : "pick");
+	for (cur = list; cur; cur = cur->next) {
+		sha1 = find_unique_abbrev(cur->item->object.sha1, DEFAULT_ABBREV);
+		if (get_message(cur->item, cur->item->buffer, &msg))
+			return error(_("Cannot get commit message for %s"), sha1);
+		strbuf_addf(buf, "%s %s %s\n", action, sha1, msg.subject);

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.

For the user.  This is the instruction sheet that the user will be
able to edit at a later stage, when we develop something like
"sequencer -i".

quoted

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

Ah, thanks for pointing these out -- I hadn't thought about them earlier.

quoted

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

Hm, error_errno seems that consistently returns -1 seems like a good
idea now.  I'll get it back into the series next time.

quoted

+	}
+
+	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) {

Ok.

quoted

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

Ok.

quoted

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

Why abbreviate?

quoted

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

What happens and should happen on error?

[...]

quoted

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

Exit status? I have to think back to figure out why I chose to pass it
around like this.

quoted

+{
+	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?

I'll use the lockfile API if I have time before the next iteration.

quoted

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

Right :)

[...]

quoted

 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.

fatal: You do not have a valid HEAD
I just tried it :)

quoted

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

Yes, although I'm not sure what to do with the done file now (you
pointed that out earlier).

quoted

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

Agreed, but I don't think this kind of optimization will be in the
scope of this series.  It's nice to think about though.

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

The latter, definitely.  I don't want to have to deal with malformed
instruction sheets.

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

Yes, yes! I'd love this feature.

Thanks.

-- Ram