[v2,0/2] rebase -i: improve error message when picking merge

Message

Philippe Blain via GitGitGadget April 8, 2024, 2:16 p.m. UTC

If the user tries to pick a merge commit error out when parsing the todo
list rather than complaining when trying to pick the commit.

Thanks to Patrick and Rubén for their comments on V1. Changes since V1:

 * Rebased to avoid a conflict with jk/core-comment-string
 * Patch 1 is a new preparatory change that plumbs 'struct replay_opts'
   through to parse_insn_line()
 * Patch 2 is updated to use is_rebase_i() rather than requiring the caller
   to pass a boolean indicating whether we're rebasing.

Phillip Wood (2):
  rebase -i: pass struct replay_opts to parse_insn_line()
  rebase -i: improve error message when picking merge

 builtin/rebase.c              | 17 +++++++----
 rebase-interactive.c          | 21 ++++++++-----
 rebase-interactive.h          |  9 ++++--
 sequencer.c                   | 57 ++++++++++++++++++++++++++++-------
 sequencer.h                   |  4 +--
 t/t3404-rebase-interactive.sh | 33 ++++++++++++++++++++
 6 files changed, 111 insertions(+), 30 deletions(-)


base-commit: 19981daefd7c147444462739375462b49412ce33
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1672%2Fphillipwood%2Frebase-reject-merges-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1672/phillipwood/rebase-reject-merges-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1672

Range-diff vs v1:

 -:  ----------- > 1:  1bcf92c6105 rebase -i: pass struct replay_opts to parse_insn_line()
 1:  7726c496385 ! 2:  fbc6746e018 rebase -i: improve error message when picking merge
     @@ Commit message
      
              exec git cherry-pick -m1 abc123
      
     -    The change is relatively straight forward but is complicated slightly as
     -    we now need to tell the parser if we're rebasing or not.
     -
          Reported-by: Stefan Haller <[e-mail]>
          Signed-off-by: Phillip Wood <[e-mail]>
      
     - ## builtin/rebase.c ##
     -@@ builtin/rebase.c: 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))
     - 			BUG("unusable todo list");
     - 
     - 		ret = complete_action(the_repository, &replay, flags,
     -
     - ## rebase-interactive.c ##
     -@@ rebase-interactive.c: int edit_todo_list(struct repository *r, struct todo_list *todo_list,
     - 	 * it.  If there is an error, we do not return, because the user
     - 	 * might want to fix it in the first place. */
     - 	if (!initial)
     --		incorrect = todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list) |
     -+		incorrect = todo_list_parse_insn_buffer(r, todo_list->buf.buf,
     -+							todo_list, 1) |
     - 			file_exists(rebase_path_dropped());
     - 
     - 	if (todo_list_write_to_file(r, todo_list, todo_file, shortrevisions, shortonto,
     -@@ rebase-interactive.c: int edit_todo_list(struct repository *r, struct todo_list *todo_list,
     - 	if (initial && new_todo->buf.len == 0)
     - 		return -3;
     - 
     --	if (todo_list_parse_insn_buffer(r, new_todo->buf.buf, new_todo)) {
     -+	if (todo_list_parse_insn_buffer(r, new_todo->buf.buf, new_todo, 1)) {
     - 		fprintf(stderr, _(edit_todo_list_advice));
     - 		return -4;
     - 	}
     -@@ rebase-interactive.c: int todo_list_check_against_backup(struct repository *r, struct todo_list *todo_
     - 	int res = 0;
     - 
     - 	if (strbuf_read_file(&backup.buf, rebase_path_todo_backup(), 0) > 0) {
     --		todo_list_parse_insn_buffer(r, backup.buf.buf, &backup);
     -+		todo_list_parse_insn_buffer(r, backup.buf.buf, &backup, 1);
     - 		res = todo_list_check(&backup, todo_list);
     - 	}
     - 
     -
       ## sequencer.c ##
      @@ sequencer.c: static int check_label_or_ref_arg(enum todo_command command, const char *arg)
       	return 0;
       }
       
     +-static int parse_insn_line(struct repository *r, struct replay_opts *opts UNUSED,
      +static int error_merge_commit(enum todo_command command)
      +{
      +	switch(command) {
     @@ sequencer.c: static int check_label_or_ref_arg(enum todo_command command, const
      +	}
      +}
      +
     - 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)
     ++static int parse_insn_line(struct repository *r, struct replay_opts *opts,
     + 			   struct todo_item *item, const char *buf,
     + 			   const char *bol, char *eol)
       {
     - 	struct object_id commit_oid;
     - 	char *end_of_object_name;
     -@@ sequencer.c: static int parse_insn_line(struct repository *r, struct todo_item *item,
     +@@ sequencer.c: static int parse_insn_line(struct repository *r, struct replay_opts *opts UNUSED
       		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 &&
     ++	if (is_rebase_i(opts) && 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)
     -@@ sequencer.c: 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;
     -@@ sequencer.c: 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;
     -@@ sequencer.c: 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 "
     -@@ sequencer.c: 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;
     -@@ sequencer.c: 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);
     - 
     -
     - ## sequencer.h ##
     -@@ sequencer.h: 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);
      
       ## t/t3404-rebase-interactive.sh ##
      @@ t/t3404-rebase-interactive.sh: test_expect_success 'bad labels and refs rejected when parsing todo list' '