From: Elijah Newren <newren@gmail.com>
To: Tao Klerks <tao@klerks.biz>
Cc: Tao Klerks via GitGitGadget <gitgitgadget@gmail.com>,
git@vger.kernel.org
Subject: Re: [PATCH] RFC: switch: allow same-commit switch during merge if conflicts resolved
Date: Sat, 6 May 2023 19:48:05 -0700 [thread overview]
Message-ID: <CABPp-BGmPKyNcDa-wUh-oisTvvux+X=6BvGxSNQC2O7uodpFrA@mail.gmail.com> (raw)
In-Reply-To: <CAPMMpogiTVksUKgZ==n4d3xm4ZJqxm7ki2dOF8j8S5BaJvu1Ew@mail.gmail.com>
Hi Tao,
On Wed, May 3, 2023 at 10:01 PM Tao Klerks <tao@klerks.biz> wrote:
>
[...]
> > By the way, it was a problem that git-checkout wasn't updated to have
> > the same safety that git-switch has. We should fix that. (It's on my
> > todo list, along with adding other
> > prevent-erroneous-command-while-in-middle-of-other-operation cases.)
> >
>
> This surprises me because the difference in "safety" is very
> explicitly expressed and implemented in an option
> "can_switch_when_in_progress", which is driven purely by "checkout vs
> switch", and determines whether the validations in
> die_if_some_operation_in_progress() apply - dying on merge, am, rebase
> cherry-pick, or revert.
The implementation was done that way, not because _anyone_ thought
that was the right design, but because Duy wanted to introduce a new
command without touching the existing command, so that he only had to
deal with review comments about the new command. Two people commented
on it at the time, one of them me, saying that we should just change
checkout and fix it. But it was decided to leave it for later...
> If we are comfortable changing the behavior of branch checkout to be
> safe-and-limiting like switch, then that should be almost as simple as
> removing that condition.
I've never heard a dissenting vote against this, and I've brought it
up a few times. Junio elsewhere in this thread agrees we should just
change it.
Besides, this is typically the way backward incompatibilities are
handled in Git: First make something a warning, then make it an error,
then wait a while, then change the thing (e.g. git push defaults).
For simpler cases, you can jump straight to an error, especially when
not giving an error can hurt users and is unlikely to be what users
meant anyway. This is a case where it can really hurt.
> The wrinkle is that I believe "--force"
> should still be allowed at least in the cases where it is safe
> (whereas currently switch does *not* allow even --force if a merge is
> in progress, and this proposed patch accidentally "fixed" that for the
> same-commit case only).
I'd be okay with an override.
[...]
> >
> > More importantly, though...
> >
> > > Change the behavior of "git switch" and "git checkout" to no longer delete
> > > merge metadata, nor prohibit the switch, if a merge is in progress and the
> > > commit being switched to is the same commit the HEAD was previously set to.
> >
> > Even if there are conflicts? For rebases, cherry-picks, ams, and
> > reverts too? (Does allowing this during rebases and whatnot mean that
> > --abort becomes really funny? Does it mean that some commits are
> > applied to one branch, and all commits are applied to another? What
> > about autostashes? Does it interact weirdly with --update-refs?
> > etc.)
>
> I believe this question was resolved later in the thread. The proposal
> is to allow the simplest case of merge only, for resolved
> (unconflicted) indexes only. If the change were to make sense I could
> update this message to be clearer that none of those other operations
> or situations are impacted by this change.
As I mentioned to Junio, I understood fully that your implementation
limited the changes to this one case. That did not resolve my
concerns, it merely obviated some other bigger ones that I didn't
raise.
However, making it only available via a --force override (and then
perhaps also limiting it to just some operations), would resolve my
concerns.
> > My first gut guess is that switching with conflicts would be just as
> > safe as this is, and any users who likes your change is going to
> > complain if we don't allow it during conflicts.
>
> In principle I believe so too, I just haven't checked whether the
> tree-merge process attempts to do anything for a same-commit switch,
> and if it does, whether the presence of conflict data "bothers" it in
> any way / causes it to do the wrong thing, eg remove it.
>
> If verifying this and opening up the "pending conflicts" case meets
> the consistency itch, I'm happy to explore this area and (try to)
> expand the scope of the fix/exemption.
If this behavior is behind a `--force` flag rather than the default
behavior, then I think there's much more leniency for a partial
solution.
That said, I do still think it'd be nice to handle this case for
consistency, so if you're willing to take a look, that'd be great. If
you are interested, here's a pointer: Stolee's commit 313677627a8
("checkout: add simple check for 'git checkout -b'", 2019-08-29) might
be of interest here. Essentially, when switching to a same-commit
branch, you can short-circuit most of the work and basically just
update HEAD. (In his case, he was creating _and_ switching to a
branch, and he was essentially just trying to short-circuit the
reading and writing of the index since he knew there would be no
changes, but the same basic logic almost certainly applies to this
broader case -- no index changes are needed, so the existence of
conflicts shouldn't matter.)
If you don't want to handle that case, though, you should probably
think about what kind of message to give the user if they try to
`--force` the checkout and they have conflicts. They'd probably
deserve a longer explanation due to the inconsistency.
> > But I think it'd take
> > a fair amount of work to figure out if it's safe during
> > rebase/cherry-pick/am/revert (is it only okay on the very first patch
> > of a series? And only if non-interactive? And only without
> > --autostash and --update-refs? etc.), and whether the ending set of
> > rules feels horribly inconsistent or feels fine to support.
>
> I agree this gets complicated - I haven't thought or explored through
> most of these, but I have confirmed that switching branch in the
> middle of a *rebase* is very confusing: your rebase continues on the
> new HEAD, as you continue to commit, your rebased commits get
> committed to the branch you switched to, but at the end when you
> *complete* the rebase, the original ref you were rebasing still ends
> up being pointed to the new HEAD - so you end up with *both* the
> branch you were rebasing, and the branch you switched to along the
> way, pointing to the same head commit.
>
> I understand how that works in terms of git's internal logic, but as a
> user of rebase, if I tried to switch (to a new branch) in the middle,
> I would be intending to say "I got scared of the changes I'm making
> here, I want the that is ref pointed to the new commit graph at the
> end of the process to be this new ref, instead of the ref I originally
> started on".
>
> Supporting that usecase, for rebase, sounds to me like it should be
> done by something completely different to "git switch". The most
> helpful behavior I can think of here would be that a "git switch"
> attempt would say "cannot switch branch in the middle of a rebase. to
> continue your rebase and create a new branch, use 'git rebase
> --make-new-branch NEWBRANCHNAME" instead of 'git switch'"
That all sounds reasonable.
But you know someone is going to try it anyway during a
rebase/cherry-pick/revert. If we start letting `--force` override
during a merge, we should do something to address that inconsistency
for users. It doesn't need to be something big; we could likely
address it by just specifically checking for the `--force` case during
a rebase/cherry-pick/revert and providing an even more detailed error
message in that case that spells out why the operation cannot be
`--force`d.
> > > Also add a warning when the merge metadata is deleted (in case of a
> > > "git checkout" to another commit) to let the user know the merge state
> > > was lost, and that "git switch" would prevent this.
> >
> > If we're touching this area, we should employ the right fix rather
> > than a half measure. As I mentioned above, this should be an error
> > with the operation prevented -- just like switch behaves.
> >
>
> My understanding, given the code organization, was that we wanted to
> preserve current (funky) behavior for backwards-compatibility
> purposes.
I totally understand how you'd reach that conclusion. I would
probably come to the same one reading the code for the first time.
But, as it turns out, that's not how things happened.
> If we're comfortable changing behavior here, I am happy to
> change the patch (while keeping/allowing the --force exemption, which
> *should* still destroy the merge state).
Yaay!
> > > Also add a warning when the merge metadata is preserved (same commit),
> > > to let the user know the commit message prepared for the merge may still
> > > refer to the previous branch.
> >
> > So, it's not entirely safe even when the commit of the target branch
> > matches HEAD? Is that perhaps reason to just leave this for expert
> > users to use the update-refs workaround?
> >
>
> It is *safe*, it's just that one aspect of the outcome is *potentially
> confusing*. You really did do the merge on the original branch. The
> merge message is the same as it would be if you committed, created a
> new branch, and reset the original branch.
>
> (and just to note - the reasonable workaround is to commit the merge
> on the current "wrong" branch, create the other branch, and then reset
> the original branch, as Chris Torek shows on StackOverflow; not to
> teach people all about update-refs)
>
>
> Thanks so much for taking the time to go through all this!
>
> Please let me know whether you would be comfortable with a patch that:
> * Fixed checkout to be more restrictive
Absolutely.
> (except still allowing --force at least on a merging state)
That's fine too.
> * More explicitly noted that we are relaxing things for merge only,
> none of the other in-progress states that currently prevent switch
That wouldn't resolve any of my concerns; it was totally clear to me
the first time.
> * Also worked with outstanding conflicts in the index (verifying that
> this is safe)
In combination with `--force`, I think that would be very nice.
next prev parent reply other threads:[~2023-05-07 2:48 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-02 6:27 [PATCH] RFC: switch: allow same-commit switch during merge if conflicts resolved Tao Klerks via GitGitGadget
2023-05-02 15:55 ` Elijah Newren
2023-05-02 16:50 ` Junio C Hamano
2023-05-03 0:34 ` Elijah Newren
2023-05-04 5:01 ` Tao Klerks
2023-05-05 5:06 ` Tao Klerks
2023-05-07 2:57 ` Elijah Newren
2023-05-07 2:48 ` Elijah Newren [this message]
2023-05-07 22:01 ` Felipe Contreras
2023-05-08 8:30 ` Tao Klerks
2023-05-08 16:13 ` Felipe Contreras
2023-05-08 16:58 ` Tao Klerks
2023-05-08 19:18 ` Junio C Hamano
2023-05-09 1:55 ` Felipe Contreras
2023-05-08 10:44 ` Tao Klerks
2023-05-11 7:06 ` Elijah Newren
2023-05-21 20:08 ` Tao Klerks
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='CABPp-BGmPKyNcDa-wUh-oisTvvux+X=6BvGxSNQC2O7uodpFrA@mail.gmail.com' \
--to=newren@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=tao@klerks.biz \
/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).