From: Phillip Wood <phillip.wood123@gmail.com>
To: phillip.wood@dunelm.org.uk, git@vger.kernel.org
Subject: Re: [PATCH 8/8] rebase: improve resumption from incorrect initial todo list
Date: Wed, 17 May 2023 13:13:28 +0100 [thread overview]
Message-ID: <08c4c313-35c8-63e9-7d66-a35b24a449dd@gmail.com> (raw)
In-Reply-To: <ZElEis+PLDYR+Jvr@ugly>
Hi Oswald
On 26/04/2023 16:34, Oswald Buddenhagen wrote:
> On Sun, Mar 26, 2023 at 03:28:01PM +0100, Phillip Wood wrote:
>> 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?
>>
> the failed state is identical to the "still editing the initial todo"
> state as far as "git status" and the shell prompt are concerned. this
> seems reasonable. i'll add it to the commit message.
When you do that please mention what "git status" and the shell prompt
actually print in this case. Ideally "git status" should mention that
the todo list needs to be edited if there are still errors in it, though
it would not surprise me if it is not that helpful at the moment.
>> - 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.
>>
> this is arguably a problem, but not much different from the pre-existing
> behavior of changes to HEAD done during the initial todo edit being lost.
I think there is a significant difference in that we're moving from a
situation where we lose commits that are created while rebase is running
to one where we're losing commits created while rebase is stopped. If a
user tries to create a commit while rebase is running then they're
asking for trouble. I don't think creating commits when rebase is
stopped is unreasonable in the same way.
> to avoid that, we'd need to lock HEAD while editing the todo. is that
> realistic at all?
I don't think it is practical to lock HEAD while git is not running. We
could just check HEAD has not changed when the rebase continues after
the user has fixed the todo list as you suggest below.
> on top of that, i should verify HEAD against orig-head in
> start_rebase(). though the only way for the user to get out of that
> situation is saving the todo contents and --abort'ing (and we must take
> care not the touch HEAD).
I think in that case it wouldn't be terrible to lose the edited todo
list as it is a bit of a corner case. The simplest thing to do would be
to print an error and remove .git/rebase-merge.
> this is somewhat similar to the abysmal situation of the final
> update-ref failing if the target ref has been modified while being
> rebased. we'd need to lock that ref for the entire duration of the
> rebase to avoid that.
"abysmal" is rather harsh - it would also be bad to overwrite the ref in
that case. I think it in relatively hard to get into that situation
though as "git checkout" wont checkout a branch that is being updated by
a rebase.
>> - 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 can be subsumed into the above case.
Meaning check and error out if HEAD has changed?
>> > 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.
>>
> this is all part of the "More or less as a side effect" paragraph, so
> this isn't a relevant objection.
I'm simply saying that we should not be optimizing for "rebase --abort"
in this case. Do you think we should?
>>> 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()?
>>
> that wouldn't help in this case, as these are dummy values which aren't
> going to be used.
If we only need to pass these when rebasing maybe we should have
separate wrappers for continuing a rebase and a cherry-pick/revert. If
we don't always need these parameters when continuing a rebase we could
have a separate function when these parameters are required and leave
the signature of sequencer_continue() unchanged.
> but more broadly, the whole state management is a total mess.
For historic reasons there are separate functions to write the state for
the "merge" backed and the "apply" backend. That is not ideal but it is
hardly a "total mess". The code for reading the state files is more
contrived than the code that writes them. I do have some patches to try
and reduce the duplication when reading the state files.
>>> -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()
>>
> i went for the "minimal churn" approach.
There is a balance to be had, but we don't want to build up a lot of
forward declarations over time just because it is easier than moving the
function in a preparatory patch. A simple patch to move a function is
easy to review with --color-moved.
>>> + git reflog > reflog &&
>>> + test $(grep -c fast-forward reflog) = 1 &&
>>
>> Using test_line_count would make test failures easier to debug.
>>
> that's calling for a new test_filtered_line_count function which would
> have quite some users.
> for the time being, both grep + test_line_count and grep -c are rather
> prevalent, in this file the latter in particular.
The style of our tests has evolved over time. When adding new tests it
is better to focus on making them easy to debug. I don't think you need
to add a new function here, just
grep fast-forward reflog >filtered-reflog
test_line_count = 1 filtered-reflog
>>> + 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.
>>
> fair enough, but the precedents again speak a different language.
Yes older tests tend to be written in a style that is harder to debug.
Best Wishes
Phillip
next prev parent reply other threads:[~2023-05-17 12:13 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
2023-04-26 15:34 ` Oswald Buddenhagen
2023-05-17 12:13 ` Phillip Wood [this message]
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=08c4c313-35c8-63e9-7d66-a35b24a449dd@gmail.com \
--to=phillip.wood123@gmail.com \
--cc=git@vger.kernel.org \
--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).