Subject: Re: [PATCH v3 10/21] checkout: split part of it to new command 'switch'

From: Phillip Wood <[email-masked]>
Date: Fri, 29 Mar 2019 11:04:19 +0000

Hi Elijah

On 28/03/2019 17:39, Elijah Newren wrote:
> Hi Phillip,
> 
> On Thu, Mar 28, 2019 at 9:23 AM Phillip Wood <[email-masked]> wrote:
>> On 28/03/2019 11:04, Duy Nguyen wrote:
>>> On Wed, Mar 27, 2019 at 5:24 PM Phillip Wood <[email-masked]> wrote:
>>>> On 26/03/2019 15:48, Elijah Newren wrote:
>>>>> On Tue, Mar 26, 2019 at 8:24 AM Duy Nguyen <[email-masked]> wrote:
>>>>>> On Tue, Mar 26, 2019 at 10:01 PM Elijah Newren <[email-masked]> wrote:
> 
>>>>> Yes, and in the middle of a cherry-pick with a range you've added some
>>>>> commits to one branch and some to another.  In the middle of a revert
>>>>> you're doing similar.  It sounds like crazytown to me (and maybe we
>>>>> shouldn't provide the --ignore-in-process flag unless users clamor for
>>>>> it
>>>
>>> I missed this part in my last reading. I think if we could safely
>>> switch away and get back to resume, then --ignore-in-process could
>>> still be useful.
>>
>> If we can get back safely then that makes sense, I'm not sure about
>> switching while there are conflicts or staged changes though, it feels
>> like there's more potential for things to go wrong there.
> 
> I really like that way of putting it; I think that makes it much
> clearer.  Note, though that staged changes and conflicts could happen
> with any of rebase, merge, cherry-pick, or revert, so this problem is
> not limited to a subset of those operations.

Indeed

> 
>>> I sometimes switch to another commit to check out
>>> stuff then back. For interactive rebase with "edit" command for
>>> example, it's quite safe to do so. (yes the other option is "git
>>> worktree add", but that could be a heavy hammer sometimes)
>>
>>>> I think that could be the way to go for merges and cherry-picks, or
>>>
>>> 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 
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 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).

> (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.

> 
> 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.

>  BUT, make sure to also:
>    * Make '--ignore-in-progress' incompatible with both '-m' and
> '--discard-changes'; if folks try to use either of those additional
> options with --ignore-in-progress, tell people to use `<operation>
> --quit` first.

I think of --discard-changes like --abort. --quit only removes the state 
dir so would pair with -m, as it does not reset the index or working tree.

Overall I think we're more or less in agreement modulo the treatment of 
staged changes when there is no merge etc in progress.

Best Wishes

Phillip

>  Do NOT provide an override. (Alternatively, refer to
> `<operation> --quit` as the override, since it is).
> 
> 
> Elijah
>