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

Re: [PATCH] rebase -i: improve error message when picking merge

From
Pphillip.wood123@gmail.com <phillip.wood123@gmail.com>
Date
Apr 4, 2024, 15:29 UTC
Message-ID
<875639c6-1efc-4fb2-b80c-2205b67c263b@gmail.com>
In-Reply-To
<Zg5D3dXYFM2SONE-@tanuki>
Hi Patrick
On 04/04/2024 07:08, Patrick Steinhardt wrote:
Show 17 quoted lines
> On Mon, Feb 26, 2024 at 10:58:07AM +0000, Phillip Wood via GitGitGadget wrote:
> [...]
>> diff --git a/builtin/rebase.c b/builtin/rebase.c
>> index 5b086f651a6..a33e41c44da 100644
>> --- a/builtin/rebase.c
>> +++ b/builtin/rebase.c
>> @@ -297,7 +297,7 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
>>   	else {
>>   		discard_index(&the_index);
>>   		if (todo_list_parse_insn_buffer(the_repository, todo_list.buf.buf,
>> -						&todo_list))
>> +						&todo_list, 1))
> 
> I think these booleans are somewhat hard to read. I may be
> overengineering this, so please feel free to push back, but would it
> make more sense to introduce a `unsigned flags` field and then have a
> `PARSE_INSN_IS_REBASING` flag?

I agree the boolean is a bit opaque, I don't think they are that unusual in the code base but that doesn't mean they're readable. I had hoped to pass an instance of 'struct replay_opts' to parse_insn_line() and then call is_rebase_i() which there which would have been nicer. Unfortunately "git rebase --edit-todo" does not instantiate an instance of that struct as it is not needed for editing the todo list so I went with a boolean instead. There are one or two places where we need to use is_rebase_i() for this when calling parse_insn_line() which makes using an unsigned flags argument a bit messy. I'll have a look and see how hard it would be to ensure we always have a valid 'struct replay_opts' when calling parse_insn_line().

Show 10 quoted lines
>> +static int error_merge_commit(enum todo_command command)
>> +{
>> +	switch(command) {
>> +	case TODO_PICK:
>> +		return error(_("'%s' does not accept merge commits, "
>> +			       "please use '%s'"),
>> +			     todo_command_info[command].str, "merge -C");
> 
> I wonder how actionable these commands are. Can we give the full command
> that the user can use instead, including the commit ID?

The calling function also prints the offending line so the user can see the commit details there.

Show 5 quoted lines
> That raises another question though: how exactly is the user supposed to
> perform the merge? Should they merge the merge commit, resulting in two
> merge commits? Should they pick one of the sides, and if so, which one?
> I guess the answer is "it depends", which makes it harder for us to come
> up with actionable advice here.

I agree it would be nice to be more precise but I'm not sure we can tell what the user is actually trying to do so I think the best we can do is point them in the direction of the 'merge' command.

Show 8 quoted lines
>> +	case TODO_REWORD:
>> +		return error(_("'%s' does not accept merge commits, "
>> +			       "please use '%s'"),
>> +			     todo_command_info[command].str, "merge -c");
> 
> I was about to suggest that the above two cases should be merged. But
> then I realized that it's "merge -c" here, but "merge -C" in the first
> case.
Yes, it is a subtle difference.
Thanks very much for taking a look at this
Phillip
Show 146 quoted lines
> Patrick
> 
>> +	case TODO_EDIT:
>> +		return error(_("'%s' does not accept merge commits, "
>> +			       "please use '%s' followed by '%s'"),
>> +			     todo_command_info[command].str,
>> +			     "merge -C", "break");
>> +
>> +	case TODO_FIXUP:
>> +	case TODO_SQUASH:
>> +		return error(_("cannot squash merge commit into another commit"));
>> +
>> +	default:
>> +		BUG("unexpected todo_command");
>> +	}
>> +}
>> +
>>   static int parse_insn_line(struct repository *r, struct todo_item *item,
>> -			   const char *buf, const char *bol, char *eol)
>> +			   const char *buf, const char *bol, char *eol,
>> +			   int rebasing)
>>   {
>>   	struct object_id commit_oid;
>>   	char *end_of_object_name;
>> @@ -2655,7 +2684,12 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,
>>   		return status;
>>   
>>   	item->commit = lookup_commit_reference(r, &commit_oid);
>> -	return item->commit ? 0 : -1;
>> +	if (!item->commit)
>> +		return -1;
>> +	if (rebasing && item->command != TODO_MERGE &&
>> +	    item->commit->parents && item->commit->parents->next)
>> +		return error_merge_commit(item->command);
>> +	return 0;
>>   }
>>   
>>   int sequencer_get_last_command(struct repository *r UNUSED, enum replay_action *action)
>> @@ -2686,7 +2720,7 @@ int sequencer_get_last_command(struct repository *r UNUSED, enum replay_action *
>>   }
>>   
>>   int todo_list_parse_insn_buffer(struct repository *r, char *buf,
>> -				struct todo_list *todo_list)
>> +				struct todo_list *todo_list, int rebasing)
>>   {
>>   	struct todo_item *item;
>>   	char *p = buf, *next_p;
>> @@ -2704,7 +2738,7 @@ int todo_list_parse_insn_buffer(struct repository *r, char *buf,
>>   
>>   		item = append_new_todo(todo_list);
>>   		item->offset_in_buf = p - todo_list->buf.buf;
>> -		if (parse_insn_line(r, item, buf, p, eol)) {
>> +		if (parse_insn_line(r, item, buf, p, eol, rebasing)) {
>>   			res = error(_("invalid line %d: %.*s"),
>>   				i, (int)(eol - p), p);
>>   			item->command = TODO_COMMENT + 1;
>> @@ -2852,7 +2886,8 @@ static int read_populate_todo(struct repository *r,
>>   	if (strbuf_read_file_or_whine(&todo_list->buf, todo_file) < 0)
>>   		return -1;
>>   
>> -	res = todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list);
>> +	res = todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list,
>> +					  is_rebase_i(opts));
>>   	if (res) {
>>   		if (is_rebase_i(opts))
>>   			return error(_("please fix this using "
>> @@ -2882,7 +2917,7 @@ static int read_populate_todo(struct repository *r,
>>   		struct todo_list done = TODO_LIST_INIT;
>>   
>>   		if (strbuf_read_file(&done.buf, rebase_path_done(), 0) > 0 &&
>> -		    !todo_list_parse_insn_buffer(r, done.buf.buf, &done))
>> +		    !todo_list_parse_insn_buffer(r, done.buf.buf, &done, 1))
>>   			todo_list->done_nr = count_commands(&done);
>>   		else
>>   			todo_list->done_nr = 0;
>> @@ -6286,7 +6321,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
>>   	strbuf_release(&buf2);
>>   	/* Nothing is done yet, and we're reparsing, so let's reset the count */
>>   	new_todo.total_nr = 0;
>> -	if (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo) < 0)
>> +	if (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo, 1) < 0)
>>   		BUG("invalid todo list after expanding IDs:\n%s",
>>   		    new_todo.buf.buf);
>>   
>> diff --git a/sequencer.h b/sequencer.h
>> index dcef7bb99c0..ed2c4b38514 100644
>> --- a/sequencer.h
>> +++ b/sequencer.h
>> @@ -136,7 +136,7 @@ struct todo_list {
>>   }
>>   
>>   int todo_list_parse_insn_buffer(struct repository *r, char *buf,
>> -				struct todo_list *todo_list);
>> +				struct todo_list *todo_list, int rebasing);
>>   int todo_list_write_to_file(struct repository *r, struct todo_list *todo_list,
>>   			    const char *file, const char *shortrevisions,
>>   			    const char *shortonto, int num, unsigned flags);
>> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
>> index 64b641002e4..20b8589ad07 100755
>> --- a/t/t3404-rebase-interactive.sh
>> +++ b/t/t3404-rebase-interactive.sh
>> @@ -2203,6 +2203,39 @@ test_expect_success 'bad labels and refs rejected when parsing todo list' '
>>   	test_path_is_missing execed
>>   '
>>   
>> +test_expect_success 'non-merge commands reject merge commits' '
>> +	test_when_finished "test_might_fail git rebase --abort" &&
>> +	git checkout E &&
>> +	git merge I &&
>> +	oid=$(git rev-parse HEAD) &&
>> +	cat >todo <<-EOF &&
>> +	pick $oid
>> +	reword $oid
>> +	edit $oid
>> +	fixup $oid
>> +	squash $oid
>> +	EOF
>> +	(
>> +		set_replace_editor todo &&
>> +		test_must_fail git rebase -i HEAD 2>actual
>> +	) &&
>> +	cat >expect <<-EOF &&
>> +	error: ${SQ}pick${SQ} does not accept merge commits, please use ${SQ}merge -C${SQ}
>> +	error: invalid line 1: pick $oid
>> +	error: ${SQ}reword${SQ} does not accept merge commits, please use ${SQ}merge -c${SQ}
>> +	error: invalid line 2: reword $oid
>> +	error: ${SQ}edit${SQ} does not accept merge commits, please use ${SQ}merge -C${SQ} followed by ${SQ}break${SQ}
>> +	error: invalid line 3: edit $oid
>> +	error: cannot squash merge commit into another commit
>> +	error: invalid line 4: fixup $oid
>> +	error: cannot squash merge commit into another commit
>> +	error: invalid line 5: squash $oid
>> +	You can fix this with ${SQ}git rebase --edit-todo${SQ} and then run ${SQ}git rebase --continue${SQ}.
>> +	Or you can abort the rebase with ${SQ}git rebase --abort${SQ}.
>> +	EOF
>> +	test_cmp expect actual
>> +'
>> +
>>   # This must be the last test in this file
>>   test_expect_success '$EDITOR and friends are unchanged' '
>>   	test_editor_unchanged
>>
>> base-commit: c875e0b8e036c12cfbf6531962108a063c7a821c
>> -- 
>> gitgitgadget
>>
Previous: Patrick SteinhardtNext: Rubén Justo
Message 5 of 26 in “rebase -i: improve error message when picking merge”
  1. rebase -i: improve error message when picking mergePhillip Wood via GitGitGadget, Feb 26, 2024
  2. phillip.wood123@gmail.comApr 3, 2024
  3. Patrick SteinhardtApr 4, 2024
  4. Patrick SteinhardtApr 4, 2024
  5. phillip.wood123@gmail.comApr 4, 2024
  6. Rubén JustoApr 4, 2024
  7. phillip.wood123@gmail.comApr 5, 2024
  8. Rubén JustoApr 6, 2024
  9. phillip.wood123@gmail.comApr 7, 2024
  10. 0/2 rebase -i: improve error message when picking mergePhillip Wood via GitGitGadget, Apr 8, 2024
  11. 1/2 rebase -i: pass struct replay_opts to parse_insn_line()Phillip Wood via GitGitGadget, Apr 8, 2024
  12. Patrick SteinhardtApr 9, 2024
  13. 2/2 rebase -i: improve error message when picking mergePhillip Wood via GitGitGadget, Apr 8, 2024
  14. Junio C HamanoApr 8, 2024
  15. Patrick SteinhardtApr 9, 2024
  16. Junio C HamanoApr 9, 2024
  17. Patrick SteinhardtApr 9, 2024
  18. Phillip WoodApr 9, 2024
  19. Junio C HamanoApr 9, 2024
  20. Phillip WoodApr 12, 2024
  21. 0/2 rebase -i: improve error message when picking mergePhillip Wood via GitGitGadget, May 30, 2024
  22. 1/2 rebase -i: pass struct replay_opts to parse_insn_line()Phillip Wood via GitGitGadget, May 30, 2024
  23. 2/2 rebase -i: improve error message when picking mergePhillip Wood via GitGitGadget, May 30, 2024
  24. Junio C HamanoMay 30, 2024
  25. Phillip WoodJun 3, 2024
  26. Junio C HamanoJun 3, 2024

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.