Hi Phillip,
On Fri, Mar 29, 2019 at 4:04 AM Phillip Wood <phillip.wood123@xxxxxxxxx> wrote:
> On 28/03/2019 17:39, Elijah Newren wrote:
> > On Thu, Mar 28, 2019 at 9:23 AM Phillip Wood <phillip.wood123@xxxxxxxxx> wrote:
> >> On 28/03/2019 11:04, Duy Nguyen wrote:
> >>> Just so we're clear, what is your "the way" to go? to remove
> >>> CHERRY_HEAD_PICK and MERGE_HEAD (and other MERGE_* as well) if
> >>> --ignore-in-process is specified? Or to leave MERGE_* and
> >>> CHERRY_PICK_HEAD alone and delete other stuff?
> >>
> >> I was agreeing with Elijah about dropping --ignore-in-progress unless
> >> there's a demand for it or at least restricting it so that it requires
> >> --discard-changes and aborts in-progress merges and single in-progress
> >> cherry-picks/reverts. (I'm worried about people switching branches when
> >> cherry-picking more than one commit, though as you say it can make sense
> >> during a rebase.)
> >
> > I understand the desire to prevent mis-uses, and I agree that if there
> > are staged changes or conflicts it's really likely things will go
> > sideways. But I think we should instead check for those situations
> > rather than use e.g. rebase vs. merge as a proxy for whether those
> > problems could be present.
>
> When cherry-picking multiple commits if the user commits the conflict
> resolution with 'git commit' then the presence of .git/sequencer is the
> only sign that a cherry-pick is in progress (wt-status.c fails to detect
> this, I've got a fix but no tests yet). rebase can also stop without
> having conflicts or staged changes so I think we need to check for in
> progress commands as well as conflicts (what do we want to do if someone
This whole discussion is about "--ignore-in-progress" which implicitly
implies we are checking for in progress commands and choosing whether
to override it. So I don't understand what you mean by saying we need
to check for it; isn't that a given?
> tries to switch in the middle of a bisect? - I don't have a strong
> opinion). I agree switch should fail if there are conflicts, but I think
> it is fine to switch with staged or unstaged changes if there isn't a
> merge etc in progress (I quite often start working on something and
> then realize I haven't started a new branch just before I commit). I
> could possibly be convinced that silently switching with staged changes
> is always a bad idea though.
I think we might be missing the big picture by trying to discuss
things in terms of in-progress operations or conflicts or staged
changes or unstaged changes. Allow me to attempt to reframe the
discussion: We have identified at least one case where allowing the
--ignore-in-progress flag would be unsafe, and we've identified one
where we think it would be safe and useful, thus we need a rule of
thumb for when it is safe to use and when it isn't. Here's my
attempt:
--ignore-in-progress is safe enough for usage if we can switch to
another branch and back with no net overall changes to either the
index or the working tree after the two switches.
This rule could allow for the presence of both staged and unstaged
changes (or maybe even conflicts in some alternate world where
checkout/switch didn't necessarily error out on those), depending on
if switch/checkout can operate without touching those particular files
as part of switching.
> > I am especially concerned with the idea of
> > having something like "git switch --ignore-in-progress
> > --discard-changes" being used to quit merges or cherry-picks or
> > reverts or even rebases. In my opinion, doing so is creating flags to > combine uncommon pairs of git commands (git <operation> --quit + git
> > switch) in a way that is far less clear. I think that's a bad route
> > to go down, and we should keep the commands orthogonal
>
> keeping commands orthogonal is certainly clearer, if less convenient -
> lets do it (assuming Duy agrees).
Yaay!
> > (if I could
> > start all over, I'd also make reset and checkout and everything else
> > stop modifying any in-progress state).
> >
> > Instead, I would either:
> >
> > * Drop `--ignore-in-progress` for now. (Although Duy had a
> > meaningful usecase)
>
> I think it could be useful during a rebase, I'm not sure about any of
> the other operations though.
I think it could be useful during some rebases, but it should not be
allowed if the user can't switch back to the current commit with no
net overall changes to the index or working tree.
Also, I don't see how rebase is unique here. Rebase, cherry-pick,
merge, and revert can all stop with conflicts, staged changes, and
unstaged changes. All of them can also stop without any one of those
(e.g. cherry-pick'ing a commit which has been piecemeal applied
already, merging a branch whose individual changes have already been
cherry-picked and when the user has specified --no-commit, or
reverting a commit whose changes have already been unapplied). Thus,
I continue to believe that which operation is in progress is
irrelevant. Either we shouldn't allow switching during any
in-progress operation, or we should determine some other criteria for
when it is safe to allow --ignore-in-progress. Basing it on the
operation would sometimes allow --ignore-in-progress to be used when
it shouldn't be, and disallow it sometimes when it shouldn't. I'm a
fan of the rule I mentioned up above ("if we can switch and switch
back with no net changes then it's safe enough to allow")
> >
> > OR
> >
> > * Make `git switch --ignore-in-progress <branch>` leave all process
> > state in place and switch branches, if we would otherwise be able to
> > switch branches (i.e. there isn't dirty or conflicted changes in the
> > way).
>
> I thought we allowed branch switches when there are staged or unstaged
> changes, I don't think that is a problem unless we're in the middle of a
> merge etc. I'm still not sure it's a good idea to switch branches in the
> middle of a multiple cherry-pick, maybe we should print a warning.
I didn't say to disallow it if there were dirty or conflicted changes,
I said to disallow it if there were dirty or conflicted changes *in
the way*. We don't allow branch switches when dirty changes would be
overwritten or need to be merged, as that can't easily be reversed. I
think --ignore-in-progress should only be allowed when it can be
easily reversed to get the user back to the right branch/commit.
This "no net changes" rule also reinforces (or is reinforced by) the
other suggestion I made of having --ignore-in-progress be made
incompatible with both -m and --discard-changes.
But I totally agree that switching branches during the middle of some
operation should print a warning -- not just for cherry-pick, but for
merge or rebase or revert too. In all cases it'll be important to
tell the user both that they could really mess things up if they try
to resume the operation without switching back, and telling the user
how to get back to where they used to be (in rebase's case, that'd be
"git switch --ignore-in-progress <previous-commit>" while for the
other three it'd be "git switch --ignore-in-progress
<previous-branch>").
