From: Phillip Wood <phillip.wood123@gmail.com>
To: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>, git@vger.kernel.org
Subject: Re: [PATCH 8/8] rebase: improve resumption from incorrect initial todo list
Date: Sun, 26 Mar 2023 15:28:01 +0100 [thread overview]
Message-ID: <8a188876-c456-7269-28de-9ff406204030@dunelm.org.uk> (raw)
In-Reply-To: <20230323162235.995574-9-oswald.buddenhagen@gmx.de>
Hi Oswald
On 23/03/2023 16:22, Oswald Buddenhagen wrote:
> When the user butchers the todo file during rebase -i setup, the
> --continue which would follow --edit-todo would have skipped the last
> steps of the setup. Notably, this would bypass the fast-forward over
> untouched picks (though the actual picking loop would still fast-forward
> the commits, one by one).
>
> Fix this by splitting off the tail of complete_action() to a new
> start_rebase() function and call that from sequencer_continue() when no
> commands have been executed yet.
>
> More or less as a side effect, we no longer checkout `onto` before exiting
> when the todo file is bad.
I think the implications of this change deserve to be discussed in the
commit message. Three things spring to mind but there may be others I
haven't thought of
- Previously when rebase stopped and handed control back to the user
HEAD would have already been detached. This patch changes that
meaning we can have an active rebase of a branch while that branch is
checked out. What does "git status" show in this case? What does the
shell prompt show? Will it confuse users?
- Previously if the user created a commit before running "rebase
--continue" we'd rebase on to that commit. Now that commit will be
silently dropped.
- Previously if the user checkout out another commit before running
"rebase --continue" we'd rebase on to that commit. Now we we rebase
on to the original "onto" commit.
> This makes aborting cheaper and will simplify
> things in a later change.
Given that we're stopping so the user can fix the problem and continue
the rebase I don't think optimizing for aborting is a convincing reason
for this change on its own.
> diff --git a/builtin/revert.c b/builtin/revert.c
> index 62986a7b1b..00d3e19c62 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -231,7 +231,8 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
> return ret;
> }
> if (cmd == 'c')
> - return sequencer_continue(the_repository, opts);
> + return sequencer_continue(the_repository, opts,
> + 0, NULL, NULL, NULL);
It's a bit unfortunate that we have to start passing all these extra
parameters, could the sequencer read them itself in read_populate_opts()?
> -int sequencer_continue(struct repository *r, struct replay_opts *opts)
> +static int start_rebase(struct repository *r, struct replay_opts *opts, unsigned flags,
> + const char *onto_name, const struct object_id *onto,
> + const struct object_id *orig_head, struct todo_list *todo_list);
It would be nice to avoid this forward declaration. I think you could do
that by adding a preparatory patch that moves either checkout_onto() or
sequencer_continue()
> @@ -6142,49 +6154,52 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
>
> return error(_("nothing to do"));
> } else if (res == EDIT_TODO_INCORRECT) {
> - checkout_onto(r, opts, onto_name, onto, orig_head);
> todo_list_release(&new_todo);
>
> return -1;
> }
>
> - /* Expand the commit IDs */
> - todo_list_to_strbuf(r, &new_todo, &buf2, -1, 0);
> - strbuf_swap(&new_todo.buf, &buf2);
> - strbuf_release(&buf2);
> - new_todo.total_nr -= new_todo.nr;
> - if (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo) < 0)
> - BUG("invalid todo list after expanding IDs:\n%s",
> - new_todo.buf.buf);
I don't think we need to move this code. If start_rebase() is called
from sequencer_continue() the initial edit of the todo list failed and
has been fixed by running "git rebase --edit-todo". In that case the
oids have already been expanded on disc.
> - if (opts->allow_ff && skip_unnecessary_picks(r, &new_todo, &onto)) {
> - todo_list_release(&new_todo);
> - return error(_("could not skip unnecessary pick commands"));
> - }
> -
> - if (todo_list_write_to_file(r, &new_todo, todo_file, NULL, NULL, -1,
> - flags & ~(TODO_LIST_SHORTEN_IDS), action)) {
> - todo_list_release(&new_todo);
> - return error_errno(_("could not write '%s'"), todo_file);
> - }
> -
> - res = -1;
> -
> - if (checkout_onto(r, opts, onto_name, onto, orig_head))
> - goto cleanup;
> -
> - if (require_clean_work_tree(r, "rebase", NULL, 1, 1))
> - goto cleanup;
> -
> - todo_list_write_total_nr(&new_todo);
> - res = pick_commits(r, &new_todo, opts);
> -
> -cleanup:
> + res = start_rebase(r, opts, flags, onto_name, onto, orig_head, &new_todo);
> todo_list_release(&new_todo);
>
> return res;
> }
>
> +test_expect_success 'continue after bad first command' '
> + test_when_finished "git rebase --abort ||:" &&
> + git checkout primary^0 &&
If you want a specific commit it's better to use a tag name as those are
fixed whereas the branches get rebased all over the place in this test file.
> + git reflog expire --expire=all HEAD &&
Is this really necessary, can you pass -n to "git reflog" below?
> + (
> + set_fake_editor &&
> + test_must_fail env FAKE_LINES="bad 1 pick 1 pick 2 reword 3" \
> + git rebase -i HEAD~3 &&
> + test_cmp_rev HEAD primary &&
> + FAKE_LINES="pick 2 pick 3 reword 4" git rebase --edit-todo &&
> + FAKE_COMMIT_MESSAGE="E_reworded" git rebase --continue
> + ) &&
> + git reflog > reflog &&
> + test $(grep -c fast-forward reflog) = 1 &&
Using test_line_count would make test failures easier to debug.
> + test_cmp_rev HEAD~1 primary~1 &&
> + test "$(git log -1 --format=%B)" = "E_reworded"
It is slightly more work, but please use test_cmp for things like this
as it makes it so much easier to debug test failures.
Best Wishes
Phillip
> +'
> +
> +test_expect_success 'abort after bad first command' '
> + test_when_finished "git rebase --abort ||:" &&
> + git checkout primary^0 &&
> + (
> + set_fake_editor &&
> + test_must_fail env FAKE_LINES="bad 1 pick 1 pick 2 reword 3" \
> + git rebase -i HEAD~3
> + ) &&
> + git rebase --abort &&
> + test_cmp_rev HEAD primary
> +'
> +
> test_expect_success 'tabs and spaces are accepted in the todolist' '
> rebase_setup_and_clean indented-comment &&
> write_script add-indent.sh <<-\EOF &&
next prev parent reply other threads:[~2023-03-26 14:28 UTC|newest]
Thread overview: 49+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-23 16:22 [PATCH 0/8] sequencer refactoring Oswald Buddenhagen
2023-03-23 16:22 ` [PATCH 1/8] rebase: simplify code related to imply_merge() Oswald Buddenhagen
2023-03-23 19:40 ` Phillip Wood
2023-03-23 20:00 ` Junio C Hamano
2023-03-23 21:08 ` Felipe Contreras
2023-08-09 17:15 ` [PATCH v2 0/3] rebase refactoring Oswald Buddenhagen
2023-08-09 17:15 ` [PATCH v2 1/3] rebase: simplify code related to imply_merge() Oswald Buddenhagen
2023-08-09 17:15 ` [PATCH v2 2/3] rebase: handle --strategy via imply_merge() as well Oswald Buddenhagen
2023-08-09 17:15 ` [PATCH v2 3/3] rebase: move parse_opt_keep_empty() down Oswald Buddenhagen
2023-08-15 14:01 ` Phillip Wood
2023-10-20 9:36 ` [PATCH v3 0/3] rebase refactoring Oswald Buddenhagen
2023-10-20 9:36 ` [PATCH v3 1/3] rebase: simplify code related to imply_merge() Oswald Buddenhagen
2023-10-20 9:36 ` [PATCH v3 2/3] rebase: handle --strategy via imply_merge() as well Oswald Buddenhagen
2023-10-20 21:51 ` Junio C Hamano
2023-10-20 9:36 ` [PATCH v3 3/3] rebase: move parse_opt_keep_empty() down Oswald Buddenhagen
2023-10-20 22:07 ` [PATCH v3 0/3] rebase refactoring Junio C Hamano
2023-10-23 15:43 ` Phillip Wood
2023-10-23 19:02 ` Junio C Hamano
2023-03-23 16:22 ` [PATCH 2/8] rebase: move parse_opt_keep_empty() down Oswald Buddenhagen
2023-03-23 19:39 ` Phillip Wood
2023-03-23 16:22 ` [PATCH 3/8] sequencer: pass around rebase action explicitly Oswald Buddenhagen
2023-03-23 19:27 ` Phillip Wood
2023-03-23 21:27 ` Oswald Buddenhagen
2023-03-23 16:22 ` [PATCH 4/8] sequencer: create enum for edit_todo_list() return value Oswald Buddenhagen
2023-03-23 19:27 ` Phillip Wood
2023-03-23 16:22 ` [PATCH 5/8] rebase: preserve interactive todo file on checkout failure Oswald Buddenhagen
2023-03-23 19:31 ` Phillip Wood
2023-03-23 22:38 ` Oswald Buddenhagen
2023-03-24 14:15 ` Phillip Wood
2023-03-24 14:42 ` Oswald Buddenhagen
2023-03-23 20:16 ` Junio C Hamano
2023-03-23 23:23 ` Oswald Buddenhagen
2023-03-24 4:31 ` Junio C Hamano
2023-03-23 16:22 ` [PATCH 6/8] sequencer: simplify allocation of result array in todo_list_rearrange_squash() Oswald Buddenhagen
2023-03-23 19:46 ` Phillip Wood
2023-03-23 22:13 ` Oswald Buddenhagen
2023-03-23 16:22 ` [PATCH 7/8] sequencer: pass `onto` to complete_action() as object-id Oswald Buddenhagen
2023-03-23 19:34 ` Phillip Wood
2023-03-23 21:36 ` Oswald Buddenhagen
2023-03-24 14:18 ` Phillip Wood
2023-03-23 16:22 ` [PATCH 8/8] rebase: improve resumption from incorrect initial todo list Oswald Buddenhagen
2023-03-26 14:28 ` Phillip Wood [this message]
2023-04-26 15:34 ` Oswald Buddenhagen
2023-05-17 12:13 ` Phillip Wood
2023-08-24 16:46 ` Oswald Buddenhagen
2023-03-23 19:38 ` [PATCH 0/8] sequencer refactoring Phillip Wood
2023-03-25 11:08 ` Phillip Wood
2023-04-06 12:09 ` Phillip Wood
2023-05-17 13:10 ` 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=8a188876-c456-7269-28de-9ff406204030@dunelm.org.uk \
--to=phillip.wood123@gmail.com \
--cc=git@vger.kernel.org \
--cc=oswald.buddenhagen@gmx.de \
--cc=phillip.wood@dunelm.org.uk \
/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).