Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use --list for branch and tag tab expansion #864

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

AndreasHassing
Copy link

@AndreasHassing AndreasHassing commented Jul 6, 2021

To improve performance of those tab expansions, which are currently sub-optimal in git repos with a lot of branches.

This is a major performance improvement I've been using locally. Seeing that posh-git has moved to require git v2 or higher, this feature will be in all git versions using posh-git and I thought to share it.

Often times in monolithic repositories at work I used to wait for multiple seconds for branch and tag name completion to succeed, this offloads most of the work to the git engine.

@AndreasHassing
Copy link
Author

AndreasHassing commented Jul 7, 2021

Test failures with errors like:

    Expected: 'git checkout -b newbranch'
    But was:  'Invoke-GitFunction -b newbranch'

Come from PR #855 and are not related to this change.

Edit: actually, I am not sure. The tests pass in the master build, but in my PR and also #862, the builds fail for the same test failures. Odd.

@dahlbyk
Copy link
Owner

dahlbyk commented Jul 7, 2021

I'll take a look. Thanks for the contribution!

@dscho
Copy link
Contributor

dscho commented Dec 10, 2021

Test failures with errors like:

    Expected: 'git checkout -b newbranch'
    But was:  'Invoke-GitFunction -b newbranch'

I saw the same and debugged a little bit. Turns out that if I replace the script scope with global, like so:

diff --git a/test/GitProxyFunctionExpansion.Tests.ps1 b/test/GitProxyFunctionExpansion.Tests.ps1
index 9ef606d..9b253b1 100644
--- a/test/GitProxyFunctionExpansion.Tests.ps1
+++ b/test/GitProxyFunctionExpansion.Tests.ps1
@@ -28,34 +28,34 @@ Describe 'Proxy Function Expansion Tests' {
             }
         }
         It 'Expands a proxy function with parameters' {
-            function script:Invoke-GitFunction { git checkout $args }
+            function global:Invoke-GitFunction { git checkout $args }
             $result = & $module Expand-GitProxyFunction 'Invoke-GitFunction -b newbranch'
             $result | Should -Be 'git checkout -b newbranch'
             $result | Should -Be (& $module Expand-GitProxyFunction 'igf -b newbranch')
         }
         It 'Expands a multiline proxy function' {
-            function script:Invoke-GitFunction { git checkout $args }
+            function global:Invoke-GitFunction { git checkout $args }
             $result = & $module Expand-GitProxyFunction "Invoke-GitFunction ```r`n-b ```r`nnewbranch"
             $result | Should -Be 'git checkout -b newbranch'
             $result | Should -Be (& $module Expand-GitProxyFunction "igf ```r`n-b ```r`nnewbranch")
         }
         It 'Does not expand the proxy function name if there is no preceding whitespace before backtick newlines' {
-            function script:Invoke-GitFunction { git checkout $args }
+            function global:Invoke-GitFunction { git checkout $args }
             & $module Expand-GitProxyFunction "Invoke-GitFunction```r`n-b```r`nnewbranch" | Should -Be "Invoke-GitFunction```r`n-b```r`nnewbranch"
             & $module Expand-GitProxyFunction "igf```r`n-b```r`nnewbranch" | Should -Be "igf```r`n-b```r`nnewbranch"
         }
         It 'Does not expand the proxy function name if there is no preceding non-newline whitespace before any backtick newlines' {
-            function script:Invoke-GitFunction { git checkout $args }
+            function global:Invoke-GitFunction { git checkout $args }
             & $module Expand-GitProxyFunction "Invoke-GitFunction ```r`n-b```r`nnewbranch" | Should -Be "Invoke-GitFunction ```r`n-b```r`nnewbranch"
             & $module Expand-GitProxyFunction "igf ```r`n-b```r`nnewbranch" | Should -Be "igf ```r`n-b```r`nnewbranch"
         }
         It 'Does not expand the proxy function name if the preceding whitespace before backtick newlines are newlines' {
-            function script:Invoke-GitFunction { git checkout $args }
+            function global:Invoke-GitFunction { git checkout $args }
             & $module Expand-GitProxyFunction "Invoke-GitFunction`r`n```r`n-b`r`n```r`nnewbranch" | Should -Be "Invoke-GitFunction`r`n```r`n-b`r`n```r`nnewbranch"
             & $module Expand-GitProxyFunction "igf`r`n```r`n-b`r`n```r`nnewbranch" | Should -Be "igf`r`n```r`n-b`r`n```r`nnewbranch"
         }
         It 'Does not expand the proxy function if there is no trailing space' {
-            function script:Invoke-GitFunction { git checkout $args }
+            function global:Invoke-GitFunction { git checkout $args }
             & $module Expand-GitProxyFunction 'Invoke-GitFunction' | Should -Be 'Invoke-GitFunction'
             & $module Expand-GitProxyFunction 'igf' | Should -Be 'igf'
         }
@@ -85,7 +85,7 @@ Describe 'Proxy Function Expansion Tests' {
             }
         }
         It 'Expands a single line function' {
-            function script:Invoke-GitFunction {
+            function global:Invoke-GitFunction {
                 git checkout $args
             }
             $result = & $module Expand-GitProxyFunction 'Invoke-GitFunction '
@@ -93,7 +93,7 @@ Describe 'Proxy Function Expansion Tests' {
             $result | Should -Be (& $module Expand-GitProxyFunction 'igf ' )
         }
         It 'Expands a single line function with short parameter' {
-            function script:Invoke-GitFunction {
+            function global:Invoke-GitFunction {
                 git checkout -b $args
             }
             $result = & $module Expand-GitProxyFunction 'Invoke-GitFunction '
@@ -101,7 +101,7 @@ Describe 'Proxy Function Expansion Tests' {
             $result | Should -Be (& $module Expand-GitProxyFunction 'igf ' )
         }
         It 'Expands a single line function with long parameter' {
-            function script:Invoke-GitFunction {
+            function global:Invoke-GitFunction {
                 git checkout --detach $args
             }
             $result = & $module Expand-GitProxyFunction 'Invoke-GitFunction '
@@ -109,7 +109,7 @@ Describe 'Proxy Function Expansion Tests' {
             $result | Should -Be (& $module Expand-GitProxyFunction 'igf ' )
         }
         It 'Expands a single line with piped function suffix' {
-            function script:Invoke-GitFunction {
+            function global:Invoke-GitFunction {
                 git checkout --detach $args | Write-Host
             }
             $result = & $module Expand-GitProxyFunction 'Invoke-GitFunction '
@@ -117,7 +117,7 @@ Describe 'Proxy Function Expansion Tests' {
             $result | Should -Be (& $module Expand-GitProxyFunction 'igf ' )
         }
         It 'Expands the first line in function' {
-            function script:Invoke-GitFunction {
+            function global:Invoke-GitFunction {
                 git checkout $args
                 $a = 5
                 Write-Host $null
@@ -127,7 +127,7 @@ Describe 'Proxy Function Expansion Tests' {
             $result | Should -Be (& $module Expand-GitProxyFunction 'igf ' )
         }
         It 'Expands the middle line in function' {
-            function script:Invoke-GitFunction {
+            function global:Invoke-GitFunction {
                 $a = 5
                 git checkout $args
                 Write-Host $null
@@ -137,7 +137,7 @@ Describe 'Proxy Function Expansion Tests' {
             $result | Should -Be (& $module Expand-GitProxyFunction 'igf ' )
         }
         It 'Expands the last line in function' {
-            function script:Invoke-GitFunction {
+            function global:Invoke-GitFunction {
                 $a = 5
                 Write-Host $null
                 git checkout $args
@@ -147,7 +147,7 @@ Describe 'Proxy Function Expansion Tests' {
             $result | Should -Be (& $module Expand-GitProxyFunction 'igf ' )
         }
         It 'Expands semicolon delimited functions' {
-            function script:Invoke-GitFunction {
+            function global:Invoke-GitFunction {
                 $a = 5; git checkout $args; Write-Host $null;
             }
             $result = & $module Expand-GitProxyFunction 'Invoke-GitFunction '
@@ -155,7 +155,7 @@ Describe 'Proxy Function Expansion Tests' {
             $result | Should -Be (& $module Expand-GitProxyFunction 'igf ' )
         }
         It 'Expands mixed semicolon delimited and newline functions' {
-            function script:Invoke-GitFunction {
+            function global:Invoke-GitFunction {
                 $a = 5; Write-Host $null
                 git checkout $args; Write-Host $null;
             }
@@ -164,7 +164,7 @@ Describe 'Proxy Function Expansion Tests' {
             $result | Should -Be (& $module Expand-GitProxyFunction 'igf ' )
         }
         It 'Expands mixed semicolon delimited and newline multiline functions' {
-            function script:Invoke-GitFunction {
+            function global:Invoke-GitFunction {
                 $a = 5; Write-Host $null
                 git `
                 checkout `
@@ -175,7 +175,7 @@ Describe 'Proxy Function Expansion Tests' {
             $result | Should -Be (& $module Expand-GitProxyFunction 'igf ' )
         }
         It 'Expands simultaneously semicolon delimited and newline functions' {
-            function script:Invoke-GitFunction {
+            function global:Invoke-GitFunction {
                 $a = 5;
                 Write-Host $null;
                 git checkout $args;
@@ -186,7 +186,7 @@ Describe 'Proxy Function Expansion Tests' {
             $result | Should -Be (& $module Expand-GitProxyFunction 'igf ' )
         }
         It 'Expands multiline function' {
-            function script:Invoke-GitFunction {
+            function global:Invoke-GitFunction {
                 git `
                 checkout `
                 $args
@@ -196,7 +196,7 @@ Describe 'Proxy Function Expansion Tests' {
             $result | Should -Be (& $module Expand-GitProxyFunction 'igf ' )
         }
         It 'Expands multiline function that terminates with semicolon on new line' {
-            function script:Invoke-GitFunction {
+            function global:Invoke-GitFunction {
                 git `
                 checkout `
                 $args `
@@ -207,7 +207,7 @@ Describe 'Proxy Function Expansion Tests' {
             $result | Should -Be (& $module Expand-GitProxyFunction 'igf ' )
         }
         It 'Expands multiline function with short parameter' {
-            function script:Invoke-GitFunction {
+            function global:Invoke-GitFunction {
                 git `
                 checkout `
                 -b `
@@ -218,7 +218,7 @@ Describe 'Proxy Function Expansion Tests' {
             $result | Should -Be (& $module Expand-GitProxyFunction 'igf ' )
         }
         It 'Expands multiline function with long parameter' {
-            function script:Invoke-GitFunction {
+            function global:Invoke-GitFunction {
                 git `
                 checkout `
                 --detach `
@@ -229,21 +229,21 @@ Describe 'Proxy Function Expansion Tests' {
             $result | Should -Be (& $module Expand-GitProxyFunction 'igf ' )
         }
         It 'Does not expand a single line with piped function prefix' {
-            function script:Invoke-GitFunction {
+            function global:Invoke-GitFunction {
                 "master" | git checkout --detach $args
             }
             & $module Expand-GitProxyFunction 'Invoke-GitFunction ' | Should -Be 'Invoke-GitFunction '
             & $module Expand-GitProxyFunction 'igf ' | Should -Be 'igf '
         }
         It 'Does not expand function if $args is not present' {
-            function script:Invoke-GitFunction {
+            function global:Invoke-GitFunction {
                 git checkout
             }
             & $module Expand-GitProxyFunction 'Invoke-GitFunction ' | Should -Be 'Invoke-GitFunction '
             & $module Expand-GitProxyFunction 'igf ' | Should -Be 'igf '
         }
         It 'Does not expand function if $args is not attached to the git function' {
-            function script:Invoke-GitFunction {
+            function global:Invoke-GitFunction {
                 $a = 5
                 git checkout
                 Write-Host $args
@@ -252,7 +252,7 @@ Describe 'Proxy Function Expansion Tests' {
             & $module Expand-GitProxyFunction 'igf ' | Should -Be 'igf '
         }
         It 'Does not expand multiline function if $args is not attached to the git function' {
-            function script:Invoke-GitFunction {
+            function global:Invoke-GitFunction {
                 $a = 5
                 git `
                 checkout
@@ -262,7 +262,7 @@ Describe 'Proxy Function Expansion Tests' {
             & $module Expand-GitProxyFunction 'igf ' | Should -Be 'igf '
         }
         It 'Does not expand multiline function backtick newlines are not preceded with whitespace' {
-            function script:Invoke-GitFunction {
+            function global:Invoke-GitFunction {
                 $a = 5
                 git`
                 checkout`
@@ -278,7 +278,7 @@ Describe 'Proxy Function Expansion Tests' {
             if(Test-Path -Path Function:\Invoke-GitFunction) {
                 Rename-Item -Path Function:\Invoke-GitFunction -NewName Invoke-GitFunctionBackup
             }
-            function script:Invoke-GitFunction { git checkout $args }
+            function global:Invoke-GitFunction { git checkout $args }
         }
         AfterEach {
             if(Test-Path -Path Function:\Invoke-GitFunction) {
@@ -320,14 +320,14 @@ Describe 'Proxy Function Expansion Tests' {
             }
         }
         It 'Tab completes without subcommands' {
-            function script:Invoke-GitFunction { git whatever $args }
+            function global:Invoke-GitFunction { git whatever $args }
             $functionText = & $module Expand-GitProxyFunction 'Invoke-GitFunction '
             $result = & $module GitTabExpansionInternal $functionText
 
             $result | Should -Be @()
         }
         It 'Tab completes bisect subcommands' {
-            function script:Invoke-GitFunction { git bisect $args }
+            function global:Invoke-GitFunction { git bisect $args }
             $functionText = & $module Expand-GitProxyFunction 'Invoke-GitFunction '
             $result = & $module GitTabExpansionInternal $functionText
 
@@ -342,7 +342,7 @@ Describe 'Proxy Function Expansion Tests' {
             $result2 -contains 'skip' | Should -Be $true
         }
         It 'Tab completes remote subcommands' {
-            function script:Invoke-GitFunction { git remote $args }
+            function global:Invoke-GitFunction { git remote $args }
             $functionText = & $module Expand-GitProxyFunction 'Invoke-GitFunction '
             $result = & $module GitTabExpansionInternal $functionText
 

it kind of works, I only get errors regarding igf:

[-] Proxy Function Expansion Tests.Proxy Function Name TabExpansion Tests.Expands a proxy function with parameters 58ms (56ms|2ms)
 at $result | Should -Be (& $module Expand-GitProxyFunction 'igf -b newbranch'), C:\git-sdk-64\usr\src\posh-git\test\GitProxyFunctionExpansion.Tests.ps1:34
 at <ScriptBlock>, C:\git-sdk-64\usr\src\posh-git\test\GitProxyFunctionExpansion.Tests.ps1:34
 Expected strings to be the same, but they were different.
 Expected length: 16
 Actual length:   25
 Strings differ at index 0.
 Expected: 'igf -b newbranch'
 But was:  'git checkout -b newbranch'

[...]

@AndreasHassing
Copy link
Author

Hi @dahlbyk, any chance to get this in? Is there something I need to do to prepare it better for merge? 😊

Thank you in advance.

@dahlbyk
Copy link
Owner

dahlbyk commented Mar 27, 2022

Hi @dahlbyk, any chance to get this in? Is there something I need to do to prepare it better for merge? 😊

I've finally managed to come out of my virtual coma. You should find all tests passing if you rebase onto master.

I did just merge #868, which seems to conflict with your change.

@AndreasHassing
Copy link
Author

Hi @dahlbyk, any chance to get this in? Is there something I need to do to prepare it better for merge? 😊

I've finally managed to come out of my virtual coma. You should find all tests passing if you rebase onto master.

I did just merge #868, which seems to conflict with your change.

Fixed the merge conflict, thanks :).

@dahlbyk
Copy link
Owner

dahlbyk commented Mar 27, 2022

Fixed the merge conflict

Great! Anything immediately obvious to you about the linux/macos failures?

@AndreasHassing
Copy link
Author

Fixed the merge conflict

Great! Anything immediately obvious to you about the linux/macos failures?

No, but I feel like it is caused by what I did since I touch branch filtering in my changes.

I'll take a look tomorrow to try and fix the issues 😊.

Copy link
Owner

@dahlbyk dahlbyk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Failures will probably be easier to troubleshoot if you adjust the asserts:

-$result -contains 'master' | Should -Be $true
+$result | Should -Contain 'master'

src/GitTabExpansion.ps1 Outdated Show resolved Hide resolved
@rkeithhill
Copy link
Collaborator

Failures will probably be easier to troubleshoot if you adjust the asserts:

-$result -contains 'master' | Should -Be $true
+$result | Should -Contain 'master'

Yup,
image

To improve performance of those tab expansions, which are currently
sub-optimal in git repos with a lot of branches.

* for `branch`, `--list` has been available since 1.7.8
  * https://github.com/git/git/blob/ecbdaf0899161c067986e9d9d564586d4b045d62/Documentation/RelNotes/1.7.8.txt#L25
* for `tag`, `--list` has been available since 1.7.10
  * https://github.com/git/git/blob/142430338477d9d1bb25be66267225fb58498d92/Documentation/RelNotes/1.7.10.txt#L128
@AndreasHassing AndreasHassing force-pushed the improve-branch-autocompletion-time branch from c52bdcc to 42aa80d Compare April 4, 2022 12:55
@hmleal
Copy link

hmleal commented Nov 22, 2022

There's anything missing before merging this PR? This PR is going to speed up things for all users, it would be good have it merged

@AndreasHassing
Copy link
Author

AndreasHassing commented Mar 10, 2023

There's anything missing before merging this PR? This PR is going to speed up things for all users, it would be good have it merged

@hmleal From my side; just need tests to pass 😊. They pass on Windows but MacOS and Ubuntu are not behaving the same way.
Please hold while I go buy a Mac Mini 😉 /s, I'll see if this is a POSIX thing and attempt to repro in a tiny Azure Ubuntu VM 😄.

Edit: It's definitely something to do with POSIX vs Windows:

Linux Windows
image image

On Windows, git branch --no-color --list "*" returns all branches. On Linux, it returns nothing.

@AndreasHassing
Copy link
Author

AndreasHassing commented Mar 10, 2023

@dahlbyk & @rkeithhill: this is green now 😊. If you have a minute at some point, a review and potentially merge would be appreciated.

Thank you for your good work on this project!

@AndreasHassing AndreasHassing requested a review from dahlbyk March 10, 2023 23:28
@AndreasHassing
Copy link
Author

@dahlbyk do you have a couple of minutes to take another peek at this? 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants