From: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
To: phillip.wood@dunelm.org.uk
Cc: git@vger.kernel.org
Subject: Re: [PATCH 8/8] rebase: improve resumption from incorrect initial todo list
Date: Thu, 24 Aug 2023 18:46:16 +0200 [thread overview]
Message-ID: <ZOeJWODUB4QeLaNP@ugly> (raw)
In-Reply-To: <08c4c313-35c8-63e9-7d66-a35b24a449dd@gmail.com>
On Wed, May 17, 2023 at 01:13:28PM +0100, Phillip Wood wrote:
>On 26/04/2023 16:34, Oswald Buddenhagen wrote:
>> 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.
>
i'll go with "This seems reasonable, irrespective of the actual
presentation (which could be improved)".
>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.
>
that would require actually validating the todo instead of just printing
it. or maybe the presence of the backup file could be used to make
reliable inferences. have fun! ;)
>>> - 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.
>
i think that this is a completely meaningless distinction. a rebase is
"running" while the state directory exists. having multiple terminals
open is the norm, and when havoc ensues it doesn't matter to the user
whether one of the terminals had an editor launched by git open at the
time.
>> 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.
>
what measure of "practical" are you applying?
i'm assuming that no persistent locking infra exists currently. but i
don't see a reason why it _couldn't_ - having some functions to populate
and query .git/locked-refs/** in the right places doesn't seem like a
fundamentally hard problem.
>We could just check HEAD has not changed when the rebase continues
>after the user has fixed the todo list as you suggest below.
>
that's a good safeguard which i intend to implement, but when it
triggers, the user will have to deal with the conflict. it would be much
nicer to avoid it in the first place.
>> 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.
>
actually, yes, it would be. that's why i posted a patch that avoids it.
>> 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.
>
i have no clue how it happened (certainly something to do with many open
terminals), but i actually got into that situation shortly before
writing that mail, and i assure you that "abysmal" is absolutely not an
overstatement. i mean, what do you expect a user to think when presented
with two diverging heads when trying to finish 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?
>
yes
>>> > 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?
>
you're missing the point. the optimization isn't something anyone aimed
for.
regards
next prev parent reply other threads:[~2023-08-24 16:47 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
2023-08-24 16:46 ` Oswald Buddenhagen [this message]
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=ZOeJWODUB4QeLaNP@ugly \
--to=oswald.buddenhagen@gmx.de \
--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).