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

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

From
Jonathan Nieder <jrnieder@gmail.com>
Date
Jul 6, 2011, 10:01 UTC
Message-ID
<20110706100119.GG15682@elie>
In-Reply-To
<1309938868-2028-11-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
Show 7 quoted lines
> 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
> implement features like "--continue" to continue the whole operation,
> we will need to store some information about the state and the plan at
> the beginning.  Introduce a ".git/sequencer/head" file to store this
> state, and ".git/sequencer/todo" file to store the plan.

I think I remember Junio being curious about which commit is stored in "head"; this might be a good place to put a reminder so future readers don't have to be confused.

> --- a/builtin/revert.c
> +++ b/builtin/revert.c
[...]
Show 7 quoted lines
> @@ -25,6 +26,10 @@
>   * Copyright (c) 2005 Junio C Hamano
>   */
>  
> +#define SEQ_DIR		"sequencer"
> +#define SEQ_HEAD_FILE	"sequencer/head"
> +#define SEQ_TODO_FILE	"sequencer/todo"
Yay. :)
Show 6 quoted lines
> @@ -417,7 +422,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
>  			return error(_("Your index file is unmerged."));
>  	} else {
>  		if (get_sha1("HEAD", head))
> -			return error(_("You do not have a valid HEAD"));
> +			return error(_("Can't %s on an unborn branch"), me);

Remember that "me" is an untranslated command name, and see also http://thread.gmane.org/gmane.comp.version-control.git/153026

Perhaps it would make sense to do something like
	if (get_sha1("HEAD", head)) {
		if (opts->action == REVERT)
			return error(_("can't revert as initial commit"));
		return error(_("cherry-pick into empty head not supported yet"));
	}

In a way they feel like different operations, anyway. On the other hand, there's no reason I can think of not to allow reverting a patch that only removes files as the initial commit other than not having implemented it.

Show 19 quoted lines
> @@ -578,10 +583,106 @@ static void read_and_refresh_cache(const char *me, struct replay_opts *opts)
>  	rollback_lock_file(&index_lock);
>  }
>  
> -static int pick_commits(struct replay_opts *opts)
> +static void format_todo(struct strbuf *buf, struct commit_list *todo_list,
> +			struct replay_opts *opts)
> +{
> +	struct commit_list *cur = NULL;
> +	struct commit_message msg = { NULL, NULL, NULL, NULL, NULL };
> +	const char *sha1_abbrev = NULL;
> +	const char *action;
> +
> +	action = (opts->action == REVERT ? "revert" : "pick");
> +	for (cur = todo_list; cur; cur = cur->next) {
> +		sha1_abbrev = find_unique_abbrev(cur->item->object.sha1, DEFAULT_ABBREV);
> +		if (get_message(cur->item, &msg))
> +			die(_("Cannot get commit message for %s"), sha1_abbrev);
> +		strbuf_addf(buf, "%s %s %s\n", action, sha1_abbrev, msg.subject);

Maybe some word like "command", "insn", or "keyword" would be more suggestive than "action". It also might be worth mentioning somewhere (in the commit message?) that this format is inspired by rebase--interactive's insn sheet.

Show 23 quoted lines
> +	}
> +}
> +
> +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;
> +	struct commit_list **next;
> +
> +	prepare_revs(&revs, opts);
> +
> +	/* Insert into todo_list in the same order */
> +	/* NEEDSWORK: Expose this as commit_list_append */
> +	next = todo_list;
> +	while ((commit = get_revision(&revs))) {
> +		new = xmalloc(sizeof(struct commit_list));
> +		new->item = commit;
> +		*next = new;
> +		next = &new->next;
> +	}
> +	*next = NULL;

The operation that could be exposed does not include get_revision, does it?

 /*
  * Example:
  *
  *	struct commit_list *list;
  *	struct commit_list **next = &list;
  *
  *	next = commit_list_append(c1, next);
  *	next = commit_list_append(c2, next);
  *	*next = NULL;
  *	assert(commit_list_count(list) == 2);
  *	return list;
  *
  * Don't forget to NULL-terminate!
  */
 struct commit_list **commit_list_append(struct commit *commit,
					struct commit_list **next)
 {
	struct commit_list *new = xmalloc(sizeof(*new_list));
	new->item = commit;
	*next = new;
	return &new->next;
 }
Show 8 quoted lines
> +static void create_seq_dir(void)
> +{
> +	if (file_exists(git_path(SEQ_DIR))) {
> +		if (!is_directory(git_path(SEQ_DIR)) && remove_path(git_path(SEQ_DIR)) < 0)
> +			die(_("Could not remove %s"), git_path(SEQ_DIR));
> +	} else if (mkdir(git_path(SEQ_DIR), 0777) < 0)
> +		die_errno(_("Could not create sequencer directory '%s'."), git_path(SEQ_DIR));
> +}

A local variable to cache the git_path result would make this much easier to read.

> diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
> new file mode 100644

Should "chmod +x" so the test can be run directly (I forget to do that all the time).

[...]
Show 5 quoted lines
> +test_expect_success 'cherry-pick cleans up sequencer directory upon success' '
> +	pristine_detach initial &&
> +	git cherry-pick initial..picked &&
> +	test_path_is_missing .git/sequencer
> +'

Thanks for thinking about these things. Maybe another test demonstrating that the .git/sequencer directory is left behind on failure would help put this in context.

Previous: Ramkumar RamachandraNext: Ramkumar Ramachandra
Message 29 of 55 in “[GSoC update] Sequencer: The insn sheet format”
  1. Ramkumar RamachandraJul 6, 2011
  2. 01/14 advice: Introduce error_resolve_conflictRamkumar Ramachandra, Jul 6, 2011
  3. Jonathan NiederJul 6, 2011
  4. Ramkumar RamachandraJul 6, 2011
  5. Jonathan NiederJul 6, 2011
  6. 02/14 revert: Inline add_message_to_msg functionRamkumar Ramachandra, Jul 6, 2011
  7. 03/14 revert: Don't check lone argument in get_encodingRamkumar Ramachandra, Jul 6, 2011
  8. 04/14 revert: Rename no_replay to record_originRamkumar Ramachandra, Jul 6, 2011
  9. 05/14 revert: Propogate errors upwards from do_pick_commitRamkumar Ramachandra, Jul 6, 2011
  10. Jonathan NiederJul 6, 2011
  11. Ramkumar RamachandraJul 6, 2011
  12. 06/14 revert: Eliminate global "commit" variableRamkumar Ramachandra, Jul 6, 2011
  13. Jonathan NiederJul 6, 2011
  14. Ramkumar RamachandraJul 6, 2011
  15. 07/14 revert: Introduce struct to keep command-line optionsRamkumar Ramachandra, Jul 6, 2011
  16. Jonathan NiederJul 6, 2011
  17. Ramkumar RamachandraJul 6, 2011
  18. Jonathan NiederJul 6, 2011
  19. Ramkumar RamachandraJul 12, 2011
  20. Junio C HamanoJul 6, 2011
  21. Ramkumar RamachandraJul 12, 2011
  22. Jonathan NiederJul 12, 2011
  23. 08/14 revert: Separate cmdline parsing from functional codeRamkumar Ramachandra, Jul 6, 2011
  24. Jonathan NiederJul 6, 2011
  25. Ramkumar RamachandraJul 6, 2011
  26. 09/14 revert: Don't create invalid replay_opts in parse_argsRamkumar Ramachandra, Jul 6, 2011
  27. Jonathan NiederJul 6, 2011
  28. 10/14 revert: Persist data for continuationRamkumar Ramachandra, Jul 6, 2011
  29. Jonathan NiederJul 6, 2011
  30. Ramkumar RamachandraJul 6, 2011
  31. Junio C HamanoJul 6, 2011
  32. 11/14 revert: Introduce a layer of indirection over pick_commitsRamkumar Ramachandra, Jul 6, 2011
  33. Jonathan NiederJul 6, 2011
  34. Ramkumar RamachandraJul 6, 2011
  35. Jonathan NiederJul 6, 2011
  36. Ramkumar RamachandraJul 6, 2011
  37. Jonathan NiederJul 6, 2011
  38. Junio C HamanoJul 6, 2011
  39. Ramkumar RamachandraJul 7, 2011
  40. 12/14 revert: Introduce --reset to cleanup sequencer dataRamkumar Ramachandra, Jul 6, 2011
  41. Jonathan NiederJul 6, 2011
  42. Ramkumar RamachandraJul 6, 2011
  43. Ramkumar RamachandraJul 6, 2011
  44. Jonathan NiederJul 6, 2011
  45. Jonathan NiederJul 6, 2011
  46. Ramkumar RamachandraJul 7, 2011
  47. 13/14 revert: Introduce --continue to continue the operationRamkumar Ramachandra, Jul 6, 2011
  48. Jonathan NiederJul 6, 2011
  49. Junio C HamanoJul 6, 2011
  50. Drew NorthupJul 6, 2011
  51. Ramkumar RamachandraJul 7, 2011
  52. 14/14 revert: Change insn sheet formatRamkumar Ramachandra, Jul 6, 2011
  53. Jonathan NiederJul 6, 2011
  54. Ramkumar RamachandraJul 6, 2011
  55. Jonathan NiederJul 6, 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.