git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Jonathan Nieder <jrnieder@gmail.com>
Date
May 11, 2011, 12:47 UTC
Message-ID
<20110511124657.GG2676@elie>
In-Reply-To
<1305100822-20470-7-git-send-email-artagnon@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.
Show 5 quoted lines
> 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?
Show 12 quoted lines
> --- 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.
Show 18 quoted lines
> @@ -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.

Show 6 quoted lines
> +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?

Show 5 quoted lines
> +		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) {
Show 5 quoted lines
> +		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?

Show 9 quoted lines
> +{
> +	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?
Show 5 quoted lines
> +		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. :)
[...]
Show 10 quoted lines
>  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.

Show 15 quoted lines
> +
>  	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?

Show 6 quoted lines
> +		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.
Previous: Ramkumar RamachandraNext: Ramkumar Ramachandra
Message 24 of 39 in “Sequencer Foundations”
  1. 0/8 Sequencer FoundationsRamkumar Ramachandra, May 11, 2011
  2. 1/8 revert: Improve error handling by cascading errors upwardsRamkumar Ramachandra, May 11, 2011
  3. Jonathan NiederMay 11, 2011
  4. Ramkumar RamachandraMay 13, 2011
  5. Ramkumar RamachandraMay 19, 2011
  6. 2/8 revert: Make "commit" and "me" local variablesRamkumar Ramachandra, May 11, 2011
  7. Jonathan NiederMay 11, 2011
  8. Ramkumar RamachandraMay 13, 2011
  9. Daniel BarkalowMay 13, 2011
  10. 3/8 revert: Introduce a struct to parse command-line options intoRamkumar Ramachandra, May 11, 2011
  11. Jonathan NiederMay 11, 2011
  12. Ramkumar RamachandraMay 13, 2011
  13. Jonathan NiederMay 13, 2011
  14. Ramkumar RamachandraMay 13, 2011
  15. 4/8 revert: Separate cmdline argument handling from the functional codeRamkumar Ramachandra, May 11, 2011
  16. Jonathan NiederMay 11, 2011
  17. Ramkumar RamachandraMay 13, 2011
  18. Ramkumar RamachandraMay 13, 2011
  19. Jonathan NiederMay 13, 2011
  20. 5/8 revert: Catch incompatible command-line options earlyRamkumar Ramachandra, May 11, 2011
  21. Jonathan NiederMay 11, 2011
  22. Ramkumar RamachandraMay 13, 2011
  23. 6/8 revert: Introduce head, todo, done files to persist stateRamkumar Ramachandra, May 11, 2011
  24. Jonathan NiederMay 11, 2011
  25. Ramkumar RamachandraMay 13, 2011
  26. 7/8 revert: Implement parsing --continue, --abort and --skipRamkumar Ramachandra, May 11, 2011
  27. Jonathan NiederMay 11, 2011
  28. Ramkumar RamachandraMay 13, 2011
  29. Jonathan NiederMay 13, 2011
  30. 8/8 revert: Implement --abort processingRamkumar Ramachandra, May 11, 2011
  31. Jonathan NiederMay 11, 2011
  32. Christian CouderMay 12, 2011
  33. Jonathan NiederMay 12, 2011
  34. Jonathan NiederMay 12, 2011
  35. Christian CouderMay 13, 2011
  36. Jonathan NiederMay 13, 2011
  37. Christian CouderMay 16, 2011
  38. Jonathan NiederMay 19, 2011
  39. Ramkumar RamachandraMay 20, 2011

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.