Re: [PATCH 0/2] [RFC] Revert/delay performance regression in 'git checkout -b'
- From
Derrick Stolee <stolee@gmail.com>
- Date
- Aug 22, 2019, 13:45 UTC
- Message-ID
- <37d35cf9-6387-2549-4b29-aadd40cb15ce@gmail.com>
- In-Reply-To
- <CABPp-BEN7TaMvtjoyqRa+_YxLDe8h8NYD9piu86-vWgwiKfbjQ@mail.gmail.com>
On 8/21/2019 11:16 PM, Elijah Newren wrote:
Show 58 quoted lines
> 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