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

Re: [PATCH 3/5] revert: Allow mixed pick and revert instructions

From
Jonathan Nieder <jrnieder@gmail.com>
Date
Aug 10, 2011, 15:15 UTC
Message-ID
<20110810151527.GC31315@elie.gateway.2wire.net>
In-Reply-To
<1312970151-18906-4-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
Show 8 quoted lines
> Change the way the instruction parser works, allowing arbitrary
> (action, operand) pairs to be parsed.  So now, you can do:
>
>   pick fdc0b12 picked
>   revert 965fed4 anotherpick
>
> For cherry-pick and revert, this means that a 'git cherry-pick
> --continue' can continue an ongoing revert operation and viceversa.
Nice. :)
> This patch lays the foundation for extending the parser to support
> more actions.

And why would I want to do that? I think there's a missing "so git rebase -i can reuse this machinery some day" at the end of the sentence.

Show 14 quoted lines
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -639,89 +639,84 @@ static void read_and_refresh_cache(struct replay_opts *opts)
>   *     assert(commit_list_count(list) == 2);
>   *     return list;
>   */
> -struct commit_list **commit_list_append(struct commit *commit,
> -					struct commit_list **next)
> +struct replay_insn_list **replay_insn_list_append(enum replay_action action,
> +						struct commit *operand,
> +						struct replay_insn_list **next)
>  {
> -	struct commit_list *new = xmalloc(sizeof(struct commit_list));
> +	struct replay_insn_list *new = xmalloc(sizeof(struct replay_insn_list));
Tip: we can save the readers some reading and prepare for future
renaming of the structure (not that that's something to fear) using
the idiom
	struct replay_insn_list *new = xmalloc(sizeof(*new));
[...]
Show 10 quoted lines
> -static int format_todo(struct strbuf *buf, struct commit_list *todo_list,
> -		struct replay_opts *opts)
> +static int format_todo(struct strbuf *buf, struct replay_insn_list *todo_list)
>  {
> -	struct commit_list *cur = NULL;
> +	struct replay_insn_list *cur = NULL;
>  	struct commit_message msg = { NULL, NULL, NULL, NULL, NULL };
>  	const char *sha1_abbrev = NULL;
> -	const char *action_str = opts->action == REPLAY_REVERT ? "revert" : "pick";
> +	const char *action_str;
Might be clearer with narrower scope:
	struct replay_insn_list *cur;
	for (cur = todo_list; cur; cur = cur->next) {
		struct commit_message msg = COMMIT_MESSAGE_INIT;
		const char *sha1_abbrev, *action_str;
		sha1_abbrev = ...;
		action_str = ...;
		if (get_message(cur->operand, &msg))
			return error(...);
		strbuf_addf(buf, "%s %s %s\n", ...);
	}
	return 0;

By the way, shouldn't there a free_message() call to balance out the get_message()?

[...]
> -static struct commit *parse_insn_line(char *start, struct replay_opts *opts)
> +static int parse_insn_line(char *start, enum replay_action *action,
> +			struct commit **operand)
Hm, why not
 static int parse_insn_line(const char *start, struct replay_insn_list *item);
Show 6 quoted lines
>  {
>  	unsigned char commit_sha1[20];
>  	char sha1_abbrev[40];
> -	enum replay_action action;
> -	int insn_len = 0;
> +	int keyword_len;

What is this renaming about? Maybe "action_len" would work to clarify that this is about the length of the action string at the start, not the entire line.

By the way, what are the semantics of that variable? It doesn't seem to be used, so couldn't we just eliminate it?

Show 9 quoted lines
>  	char *p, *q;
>  
>  	if (!prefixcmp(start, "pick ")) {
> -		action = REPLAY_PICK;
> -		insn_len = strlen("pick");
> -		p = start + insn_len + 1;
> +		*action = REPLAY_PICK;
> +		keyword_len = strlen("pick");
> +		p = start + keyword_len + 1;
In such a scenario, this would say
	if (!prefixcmp(start, "pick ")) {
		item->action = REPLAY_PICK;
		p += strlen("pick ");
	}
[...]
>  	} else
> -		return NULL;
> +		return -1;
Unrelated to this patch: maybe
 	return error("unrecognized action in sequencer file: %s", start);
to be easier to debug.
>  	q = strchr(p, ' ');
>  	if (!q)
> -		return NULL;
> +		return -1;
So we reject "pick a87c8989"?  That's a shame.
>  	q++;
>  
>  	strlcpy(sha1_abbrev, p, q - p);
memcpy would be clearer.  Can't this overflow the sha1_abbrev buffer?
[...]
>  	if (get_sha1(sha1_abbrev, commit_sha1) < 0)
> -		return NULL;
> +		return -1;
get_sha1 doesn't print a message, so maybe:
	return error("malformed object name in sequencer file: %s",
							sha1_abbrev);
> +
> +	*operand = lookup_commit_reference(commit_sha1);
> +	if (!*operand)
> +		return -1;
Perhaps something like
	return error("operand %s in sequencer file is not a commit",
							sha1_abbrev);
[...]
Show 11 quoted lines
> @@ -797,18 +791,18 @@ static void read_populate_opts(struct replay_opts **opts_ptr)
>  		die(_("Malformed options sheet: %s"), opts_file);
>  }
>  
> -static void walk_revs_populate_todo(struct commit_list **todo_list,
> +static void walk_revs_populate_todo(struct replay_insn_list **todo_list,
>  				struct replay_opts *opts)
>  {
>  	struct rev_info revs;
> -	struct commit *commit;
> +	struct commit *operand;
Can avoid some churn by keeping the old name here.
Show 10 quoted lines
> -	struct commit_list **next;
> +	struct replay_insn_list **next;
>  
>  	prepare_revs(&revs, opts);
>  
>  	next = todo_list;
> -	while ((commit = get_revision(&revs)))
> -		next = commit_list_append(commit, next);
> +	while ((operand = get_revision(&revs)))
> +		next = replay_insn_list_append(opts->action, operand, next);
[...]
Show 9 quoted lines
> @@ -901,8 +896,9 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
>  	read_and_refresh_cache(opts);
>  
>  	for (cur = todo_list; cur; cur = cur->next) {
> -		save_todo(cur, opts);
> -		res = do_pick_commit(cur->item, opts);
> +		save_todo(cur);
> +		opts->action = cur->action;
> +		res = do_pick_commit(cur->operand, opts);

If do_pick_commit took an "action" operand, this would be less scary. :)

[...]
Show 7 quoted lines
> --- a/t/t3510-cherry-pick-sequence.sh
> +++ b/t/t3510-cherry-pick-sequence.sh
> @@ -211,4 +211,62 @@ test_expect_success 'malformed instruction sheet 2' '
>  	test_must_fail git cherry-pick --continue
>  '
>  
> +test_expect_success 'revert --continue continues after cherry-pick' '
I haven't looked at the tests yet.  FWIW, with whatever changes above
seem suitable,
Acked-by: Jonathan Nieder <jrnieder@gmail.com>
Thanks.
Previous: Ramkumar RamachandraNext: Ramkumar Ramachandra
Message 5 of 16 in “Some preliminary work based on sequencer-stable”
  1. 0/5 Some preliminary work based on sequencer-stableRamkumar Ramachandra, Aug 10, 2011
  2. 1/5 revert: Don't remove the sequencer state on errorRamkumar Ramachandra, Aug 10, 2011
  3. 2/5 sequencer.h: Move data structuresRamkumar Ramachandra, Aug 10, 2011
  4. 3/5 revert: Allow mixed pick and revert instructionsRamkumar Ramachandra, Aug 10, 2011
  5. Jonathan NiederAug 10, 2011
  6. Ramkumar RamachandraAug 11, 2011
  7. Ramkumar RamachandraAug 11, 2011
  8. Jonathan NiederAug 11, 2011
  9. 4/5 sequencer: Expose code that handles files in .git/sequencerRamkumar Ramachandra, Aug 10, 2011
  10. Jonathan NiederAug 10, 2011
  11. Ramkumar RamachandraAug 10, 2011
  12. Jonathan NiederAug 10, 2011
  13. Ramkumar RamachandraAug 11, 2011
  14. Jonathan NiederAug 11, 2011
  15. Ramkumar RamachandraAug 11, 2011
  16. 5/5 sequencer: Remove sequencer state after final commitRamkumar Ramachandra, Aug 10, 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.