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

[Jonathan Nieder <jrnieder@gmail.com>]

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?

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

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

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