From: phillip.wood123@gmail.com
To: Patrick Steinhardt <ps@pks.im>,
Phillip Wood via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Stefan Haller <lists@haller-berlin.de>,
Johannes Schindelin <Johannes.Schindelin@gmx.de>,
Phillip Wood <phillip.wood@dunelm.org.uk>
Subject: Re: [PATCH] rebase -i: improve error message when picking merge
Date: Thu, 4 Apr 2024 16:29:37 +0100 [thread overview]
Message-ID: <875639c6-1efc-4fb2-b80c-2205b67c263b@gmail.com> (raw)
In-Reply-To: <Zg5D3dXYFM2SONE-@tanuki>
Hi Patrick
On 04/04/2024 07:08, Patrick Steinhardt wrote:
> 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().
>> +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.
> 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.
>> + 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
> 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
>>
next prev parent reply other threads:[~2024-04-04 15:29 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-26 10:58 [PATCH] rebase -i: improve error message when picking merge Phillip Wood via GitGitGadget
2024-04-03 13:42 ` phillip.wood123
2024-04-04 6:08 ` Patrick Steinhardt
2024-04-04 6:08 ` Patrick Steinhardt
2024-04-04 15:29 ` phillip.wood123 [this message]
2024-04-04 19:44 ` Rubén Justo
2024-04-05 9:30 ` phillip.wood123
2024-04-06 14:24 ` Rubén Justo
2024-04-07 13:55 ` phillip.wood123
2024-04-08 14:16 ` [PATCH v2 0/2] " Phillip Wood via GitGitGadget
2024-04-08 14:16 ` [PATCH v2 1/2] rebase -i: pass struct replay_opts to parse_insn_line() Phillip Wood via GitGitGadget
2024-04-09 4:03 ` Patrick Steinhardt
2024-04-08 14:16 ` [PATCH v2 2/2] rebase -i: improve error message when picking merge Phillip Wood via GitGitGadget
2024-04-08 22:29 ` Junio C Hamano
2024-04-09 4:03 ` Patrick Steinhardt
2024-04-09 5:08 ` Junio C Hamano
2024-04-09 6:04 ` Patrick Steinhardt
2024-04-09 15:04 ` Phillip Wood
2024-04-09 19:56 ` Junio C Hamano
2024-04-12 13:24 ` Phillip Wood
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=875639c6-1efc-4fb2-b80c-2205b67c263b@gmail.com \
--to=phillip.wood123@gmail.com \
--cc=Johannes.Schindelin@gmx.de \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=lists@haller-berlin.de \
--cc=phillip.wood@dunelm.org.uk \
--cc=ps@pks.im \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).