From: Tao Klerks <tao@klerks.biz>
To: Elijah Newren <newren@gmail.com>
Cc: Tao Klerks via GitGitGadget <gitgitgadget@gmail.com>,
git@vger.kernel.org,
Felipe Contreras <felipe.contreras@gmail.com>,
Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH] RFC: switch: allow same-commit switch during merge if conflicts resolved
Date: Sun, 21 May 2023 22:08:45 +0200 [thread overview]
Message-ID: <CAPMMpogFHnX2YPA4VmffmA0pku=43CQJ8iebCOkFm4ravBVTeg@mail.gmail.com> (raw)
In-Reply-To: <CAPMMpojDm8jHWFr8i5EC-oEKK8WBt1g3iyRvixfy1bhk8qck2g@mail.gmail.com>
FWIW, it looks like I bit off a lot more than I could chew when I
offered/proposed to fix "git checkout --force <ref>" across all the
in-progress operation types (and make it available in "git switch",
and then disallow non-force "git checkout" with in-progress
operations), especially given the 1-2 hours/week I'm managing to
spend.
I'm having enough troubles understanding all the ins & outs of the
current behavior that it will likely take me a few weeks to have any
proposed changes.
I will revive this thread at that time, if I manage to propose anything useful.
On Mon, May 8, 2023 at 12:44 PM Tao Klerks <tao@klerks.biz> wrote:
>
> On Sun, May 7, 2023 at 4:48 AM Elijah Newren <newren@gmail.com> wrote:
> >
> > On Wed, May 3, 2023 at 10:01 PM Tao Klerks <tao@klerks.biz> wrote:
> > >
> > > 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.
> >
>
> Hmm, I think there is confusion here.
>
> My proposal was (and now, again, is) to add support for "--force" to
> "git switch", and to keep and improve that existing support for "git
> checkout" (where it is in my opinion broken during a rebase), but that
> proposal was mostly-unrelated to my main goal and proposal for
> supporting same-commit switches in the first place:
>
> A same-commit switch (*without* --force) serves the use-case of
> *completing a merge on another branch*. This is, as far as I can tell
> only *useful* for merges:
> * during a rebase, switching in the middle (to the same commit,
> without --force) won't achieve anything useful; your rebase is still
> in progress, any previously rebased commits in the sequence are lost,
> and if you continue the rebase you'll end up with a very strange and
> likely-surprising partial rebase state)
> * during a cherry-pick, it's just "not very useful" - it's not bad
> like rebase, because in-progress cherry-pick metadata is destroyed
> * during am, and bisect I'm not sure, I haven't tested yet.
>
> The reason this in-progress is *valuable* for merges (in a way that it
> is not for those other states) is that the merge metadata not only
> says what you're in the middle of, but also contains additional useful
> information about what you've done so far, which you want to have be a
> part of what you commit in the end - the identity of the commit you
> were merging in.
>
> Supporting switch with --force, and having it implicitly destroy
> in-progress operation metadata, has value in that it makes it easier
> to break backwards compatibility of "git checkout" without impacting
> users' or tests' workflows; it helps make a change to make checkout
> safer; but it does not help with my other (/main?) objective of making
> it easy and intuitive to switch to another same-commit branch, to be
> able to commit your in-progress merge on another branch and avoid
> committing it where you started.
>
> Hence, if/when we add support for same-commit switching during merge
> (and potentially other operations, if that makes sense), it should
> *not* take "--force", which has a substantially different purpose and
> meaning.
>
> > > > 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.
>
> But if it were behind "--force", it wouldn't work :)
>
> >
> > 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.)
>
> Will look, thx
>
> >
> > 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.
> >
>
> --force implicitly and intentionally discards the conflicts.
>
> > > > 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.
>
> The behavior of "--force" is already clear - it resets your worktree
> to the state of the branch you are switching to. It also (or should
> but doesn't, in the case of rebase) destroys in-progress operation
> state/metadata.
>
> That said, I understand and agree that there should be a difference
> between a generic error "there is an operation in progress, you need
> to '--abort'" for the operation types that can and should not benefit
> from a same-commit exception, and the operation(s) that do get a
> same-commit exception when it doesn't apply (when you're trying to
> switch commit). If the same-commit does end up behind some parameter,
> there should be yet another message for a same-commit branch switch
> operation when the new needed parameter is not specified.
>
> >
> > > > > 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!
>
> As suggested in my recent response to Felipe, I will create a separate
> patch (series) for the git checkout safety enhancements and related
> --force support enhancements.
>
> >
> > > > > 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.
>
> I need this to work without --force, for the reasons noted above, *but
> I will split this into two patch series to avoid further confusion!*
>
> Thanks so much for your help!
prev parent reply other threads:[~2023-05-21 20:09 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
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 [this message]
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='CAPMMpogFHnX2YPA4VmffmA0pku=43CQJ8iebCOkFm4ravBVTeg@mail.gmail.com' \
--to=tao@klerks.biz \
--cc=felipe.contreras@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=gitster@pobox.com \
--cc=newren@gmail.com \
/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).