Re: [PATCH 0/2] [RFC] Revert/delay performance regression in 'git checkout -b'

From: Derrick Stolee <stolee@gmail.com>
To: Elijah Newren <newren@gmail.com>,
	Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>
Cc: "Git Mailing List" <git@vger.kernel.org>,
	"Nguyễn Thái Ngọc" <pclouds@gmail.com>,
	"Jeff Hostetler" <git@jeffhostetler.com>,
	"Junio C Hamano" <gitster@pobox.com>
Subject: Re: [PATCH 0/2] [RFC] Revert/delay performance regression in 'git checkout -b'
Date: Thu, 22 Aug 2019 09:45:03 -0400	[thread overview]
Message-ID: <37d35cf9-6387-2549-4b29-aadd40cb15ce@gmail.com> (raw)
In-Reply-To: <CABPp-BEN7TaMvtjoyqRa+_YxLDe8h8NYD9piu86-vWgwiKfbjQ@mail.gmail.com>

On 8/21/2019 11:16 PM, Elijah Newren wrote:
> Hi,
> 
> On Wed, Aug 21, 2019 at 12:21 PM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
>> As we were integrating Git 2.23.0 into VFS for Git, we discovered that "git
>> checkout -b new-branch" went from 0.3s to 10+s on the Windows OS repo. This
>> was an intentional change when writing the "git switch" builtin. Here is the
>> commit message for 65f099b ("switch: no worktree status unless real branch
>> switch happens" 2019-03-29):
>>
>> When we switch from one branch to another, it makes sense to show a
>> summary of local changes since there could be conflicts, or some files
>> left modified.... When switch is used solely for creating a new
>> branch (and "switch" to the same commit) or detaching, we don't really
>> need to show anything.
>>
>> "git checkout" does it anyway for historical reasons. But we can start
>> with a clean slate with switch and don't have to.
>>
>> This essentially reverts fa655d8411 (checkout: optimize "git checkout
>> -b <new_branch>" - 2018-08-16) and make it default for switch,
>> but also for -B and --detach. Users of big repos are encouraged to
>> move to switch.
>>
>> I was considering doing a full, long-term revert of this change to get the
>> performance back to normal, but I also saw this feedback on the list for
>> this patch:
>>
>> I like this last bit. The skip_merge_working_tree() function which
>> this removes was ugly, difficult to maintain, and difficult to get
>> just right (and easy to break -- even by changing parts of the system
>> which one might not expect to impact it).
> 
> Instead of restoring this easy-to-break code, could we instead
> simplify it and make it more robust?  As per the original commit
> message, the whole point of the patch is that when you have a huge
> index, operations take a while.  But in the special case of "git
> checkout -b <new_branch>", there's no need to even check the index.
> So, we could:
> 
>   * Check if the user is running "git checkout -b <new_branch>"
>   * If so, use the performance hack to skip touching the index at all.
> 
> This would be much better than what the patch currently does:
> 
>   * Check if the user has specified -m, if so they clearly didn't just
> specify "git checkout -b <new_branch>"
>   * Check if the user has specified -f, if so they clearly didn't just
> specify "git checkout -b <new_branch>"
>   * Check if the user has specified --detach, if so they clearly
> didn't just specify "git checkout -b <new_branch>"
>   * ...<lots of other similar steps>...
>   * If we got here, since we've checked all other cases (assuming
> other people who have touched checkout remembered to add the necessary
> checks for each and every new flag), then by deduction the user must
> have specified "git checkout -b <new_branch>", so...
>   * Use the performance hack to skip touching the index at all.

I can look into a simpler implementation of this direction. I'm
getting the feeling that this immediate recommendation of "git switch -c"
is too hasty, so the best thing to do is to create a simpler way
to detect "git checkout -b" and use the fast method.

Thanks,
-Stolee