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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 12/03/2019 16:43, Elijah Newren wrote:
> On Tue, Mar 12, 2019 at 4:06 AM Phillip Wood <phillip.wood123@xxxxxxxxx> wrote:
>>
>> Hi Elijah
>>
>> On 11/03/2019 17:54, Elijah Newren wrote:
>>> A few other comments that I thought of while responding elsewhere in
>>> the thread that didn't make sense to include elsewhere...
>>>
>>> On Fri, Mar 8, 2019 at 2:00 AM Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx> wrote:
>>>>
>>>> +-m::
>>>> +--merge::
>>>> +       If you have local modifications to one or more files that are
>>>> +       different between the current branch and the branch to which
>>>> +       you are switching, the command refuses to switch branches in
>>>> +       order to preserve your modifications in context.  However,
>>>> +       with this option, a three-way merge between the current
>>>> +       branch, your working tree contents, and the new branch is
>>>> +       done, and you will be on the new branch.
>>>> ++
>>>> +When a merge conflict happens, the index entries for conflicting
>>>> +paths are left unmerged, and you need to resolve the conflicts
>>>> +and mark the resolved paths with `git add` (or `git rm` if the merge
>>>> +should result in deletion of the path).
>>>
>>> Now that Phillip highlighted issues with -m and -f, it's hard not to
>>> wonder about other corner cases.  For example, what if the user made
>>> some changes, staged them, then made more changes, then tried to 'git
>>> checkout -m <other branch>'?  That's no longer a three-way merge, but
>>> four way.  How does that work?  Does it just rely on merge-recursive's
>>> (poorly defined) choice of when to bail out and when to permit such
>>> craziness?
>>
>> If the two-way merge fails then it does 'git add -u' before calling
>> merge_recursive(), then any merged paths are reset to the new HEAD
>> (which throws away newly added files, it should keep anything that is
>> not in HEAD or HEAD@{1}). So any staged changes are lost.
> 
> Ah, so roughly
>   * git add -u
>   * uncommitted_tree=$(git write-tree)
>   * git reset --hard
>   * git checkout $other_branch
>   * git merge-recursive $old_branch -- $other_branch $uncommitted_tree
>   * git reset --mixed HEAD

Something like that (I think it skips the reset and checkout and does
git merge-recursive $old_branch -- $uncommitted_tree $other_branch
and then updating HEAD)

> This at least gives well defined behavior, even if somewhat suboptimal
> in relation to losing staged changes (especially when those staged
> changes were new files).
> 
> I wonder if it'd be nicer, after I get my don't-touch-the-working-tree
> merge rewrite done, to instead do something like:
>   * Write the beginning index to a tree; call it $tree_0
>   * Note whether any working tree files differ from the index, add
> these all to a temporary index and write to to a tree; call it
> $tree_1.
>   * Do a three way in-memory merge of $old_branch with $other_branch
> and $tree_0; call it $merged_tree if there are no conflicts
>   * If $tree_0 == $tree_1, checkout the new branch and update the
> index and working tree to reflect the merge result.
>   * If $tree_0 != $tree_1 and there were any conflicts, abort telling
> the user they need to either unstage or stage changes first (we don't
> want to confuse users with a merge of a merge).
>   * Switch to the new branch, and update the index to match $merged_tree
>   * Do a three way in-memory merge of $old_branch with $merged_tree
> and $tree_1, writing the results (including any conflicts) to the
> working tree afterward.

As much as it annoys me to have to clear conflicts from the index after
a `checkout` or `stash pop` I'm wary of updating the working tree with
conflicts without marking those paths as unmerged in the index. Marking
them prevents the user from accidentally committing files with
unresolved conflicts. It is also easier for the user to find the
conflicts if they're marked as unmerged in the index, they can use `diff
--cc` and recreate them if they need to start over with the resolution.

> 
> Pros of this method:
>   * We don't lose newly staged files
>   * We don't lose user's carefully staged entries for existing files either
> Cons of this method:
>   * It may abort with an error if the user has a mix of both staged
> and unstaged changes (in particular, it will do so if the user's
> staged changes conflict with some difference in the new branch)

I think it's a good way of preserving any unstaged changes, it is
probably good that the user is warned ahead of time that their staged
changes would be lost.

> Thoughts?
> 
>>
>>>> +--orphan <new-branch>::
>>>> +       Create a new 'orphan' branch, named `<new-branch>`, started from
>>>> +       `<start-point>` and switch to it. See explanation of the same
>>>> +       option in linkgit:git-checkout[1] for details.
>>>
>>> Sigh...does this mean --orphan will remain broken?  It has always
>>> driven me crazy that it leaves you with a fully populated rather than
>>> an empty index.
>>
>> I've always thought that was weird.
>>
>>> It seemed broken to me before I figured out the
>>> special usecase,
>>
>> I haven't figured it out yet - what is it?
> 
> It's a presumption that despite the fact that you want a new branch,
> and one with no history to boot, that for some reason you want all the
> previous branch's current contents.  In particular, you can think of
> it as a way to squash all the history of an existing branch into a
> single commit in a new branch.
> 
> Knowing of this usecase doesn't make it bother me any less when I want
> to create a new unrelated empty branch; it seems like it took the
> esoteric usecase over the common one to me, but I'm biased.  It makes
> me feel better than neither you nor Eric could understand this
> behavior of --orphan either.

Thanks for the explanation, I agree it seems like a more esoteric use case.

Best Wishes

Phillip




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux