Improve tab expansion to handle params for push. #379

rkeithhill · 2017-01-28T07:05:36Z

Fix #234 BTW if we decide this is an OK fix (need another set of eyes on my $gitParams regex), I'd like to review the fact that all these switch regex case statements fallthrough to the next (observed while debugging tab expansion). I don't believe that is necessary. I'm thinking that most (all) of these should have a break statement.

Started a minimum set of Pester tests for tab completion. Would be good to flesh these out over time.

BTW the latest PowerShell extension for VS Code release supports "attach to PS host process" which makes debugging tab expansion a breeze. VS Code FTW! :-)

dahlbyk · 2017-01-31T14:25:30Z

src/GitTabExpansion.ps1

 $script:someCommands = @('add','am','annotate','archive','bisect','blame','branch','bundle','checkout','cherry',
                         'cherry-pick','citool','clean','clone','commit','config','describe','diff','difftool','fetch',
-                         'format-patch','gc','grep','gui','help','init','instaweb','log','merge','mergetool','mv',
-                         'notes','prune','pull','push','rebase','reflog','remote','rerere','reset','revert','rm',
+                         'format-patch','gc','grep','gui','help','init','instaweb','ls-files','log','merge','mergetool',


Out of curiosity, why add ls-files? You can get tab expansion for all commands (including plumbing like ls-files) by setting $Global:GitTabSettings.AllCommands = $true, but I'm not sure this belongs with the porcelain commands.

I'll remove this. Didn't know about AllCommands. Thanks.

dahlbyk

I think we're missing a few tests, otherwise this seems legit.

dahlbyk · 2017-01-31T14:37:56Z

test/TabExpansion.Tests.ps1

+            $result = & $module GitTabExpansionInternal 'git push  --follow-tags  -u   origin ma'
+            $result | Should BeExactly 'master'
+        }
+    }


Add tests to assert empty result when expanding a missing remote, branch, or remote/branch combination?

It wouldn't be a terrible idea to set a remote and/or branch name with a - in it, as a sanity check for the gitParams regex.

I can add some more tests. Can a branch name start with - or --?

No, that's one of the few non-system restrictions on branch/remote names.

dahlbyk · 2017-01-31T15:03:30Z

src/GitTabExpansion.ps1

            gitRemoteBranches $matches['remote'] $matches['ref'] $matches['branch'] -prefix $matches['force']
        }

        # Handles git push remote <ref>
        # Handles git push remote +<ref>
        # Handles git pull remote <ref>
-        "^(?:push|pull).* (?:\S+) (?<force>\+?)(?<ref>[^\s\:]*)$" {
+        "^(?:push|pull).* ${gitParams}(?<remote>[^\s-]\S*) (?<force>\+?)(?<ref>[^\s\:]*)$" {


Not sure how to keep this maintainable over time (other than a proper Git arg parser 🙄 ), but it's legal to have Git parameters between the remote and ref specs, and between individual ref specs (e.g. git push origin -f master -u my-branch). I'm fine with this incremental improvement for now, but food for thought.

Well, we could try "^(?:push|pull).* ${gitParams}(?<remote>[^\s-]\S*)${gitParams} (?<force>\+?)(?<ref>[^\s\:]*)$" and add some more tests? I'm OK with the incremental improvement unless we think it would be as simple as the above tweaks to handle this situation.

Something that like that seems like it should do the trick - give it a try?

rkeithhill · 2017-01-31T16:53:23Z

@dahlbyk I don't have a good test case for the remote refspec (with : in it). Can you provide me with such a refspec that returns results?

rkeithhill · 2017-02-01T05:00:48Z

OK I found some examples of refspec to add tests for. Found a regex bug in the process. Pester tests FTW!

dahlbyk

So far, so good. Stretch goal, since you're already up to your neck in this: git push supports multiple ref specs, so it's not unreasonable to expect tab to work at the end of git push -u origin HEAD:demo --follow-tags v0.1 +HEAD:ma to complete with +HEAD:master.

I took the liberty of simplifying your refs/heads/master:ma sample specs with a more real-world test case, but managed to ignore the tests on the line after. Feel free to rebase -i before a final merge.

dahlbyk · 2017-02-01T09:59:49Z

test/TabExpansion.Tests.ps1

+        It 'Tab completes all :branches' {
+            $result = & $module GitTabExpansionInternal 'git push origin :'
+            $result -contains ':master' | Should Be $true
+            # Is the following a valid branch name in a refspec?


No, it would be awesome if the {remote}/HEAD -> {remote}/default line from git branch -r were ignored. I never actually created an issue, but this does bug me periodically.

Thanks. I wasn't sure about that but it didn't look right (with spaces, ->). Thanks for updating the sample specs. I "think" the multiple ref specs should be easy to implement/test. I'll look at that tonight.

rkeithhill · 2017-02-02T04:12:55Z

Stretch goal, since you're already up to your neck in this

Oh sure, goad me on. :-) OK last commit addresses this I think.

dahlbyk · 2017-02-02T16:56:32Z

Stretch goal, since you're already up to your neck in this

Oh sure, goad me on. :-) OK last commit addresses this I think.

Not quite, since ref:branch can be mixed with other forms. I added/adjusted the tests and made the regex dumber between the remote and force/ref, so I think we're good.

rkeithhill · 2017-02-02T17:13:32Z

Yeah, getting into usages of git I'm not so familiar with. Well, you've fixed it. Want me to rebase or merge as is (didn't see any fixup)?

rkeithhill · 2017-02-02T18:05:21Z

Nevermind, there is a fixup.

Fix #234 BTW if we decide this is an OK fix (need another set of eyes on my $gitParams regex), I'd like to review the fact that all these switch regex case statements fallthrough to the next (observed while debugging tab expansion). I don't believe that is necessary. I'm thinking that most (all) of these should have a break statement. Started a minimum set of Pester tests for tab completion. Would be good to flesh these out over time. Add debug configuration for VSCode that is dedicated to debugging Pester tests.

Add Pester tests for this feature.

Make multiple push spec test more complete

rkeithhill · 2017-02-02T20:41:32Z

@dahlbyk FYI I rebased -i --autosquash this branch and pushed with --force-with-lease in case you want to make more changes. Otherwise, I'm feeling pretty good about merging this in.

rkeithhill requested a review from dahlbyk January 28, 2017 07:05

rkeithhill force-pushed the rkeithhill/is234-fix-remote-push-tab-exp branch from eb01328 to 66612c9 Compare January 31, 2017 03:35

rkeithhill self-assigned this Jan 31, 2017

rkeithhill added this to the v0.7 milestone Jan 31, 2017

dahlbyk reviewed Jan 31, 2017

View reviewed changes

dahlbyk requested changes Jan 31, 2017

View reviewed changes

rkeithhill force-pushed the rkeithhill/is234-fix-remote-push-tab-exp branch from 66612c9 to 87afebd Compare January 31, 2017 15:15

dahlbyk reviewed Feb 1, 2017

View reviewed changes

dahlbyk force-pushed the rkeithhill/is234-fix-remote-push-tab-exp branch from 82ef1ae to 013a00c Compare February 2, 2017 16:52

dahlbyk approved these changes Feb 2, 2017

View reviewed changes

rkeithhill and others added 5 commits February 2, 2017 13:35

Add more tab exp tests. Fix bug found by tests.

61d51d4

Simplify push source:destination test inputs

fc07ab6

Allow multiple refspecs in tab expansion.

e7b9312

Add Pester tests for this feature.

Fix remote expansion test for multiple remotes

dffe385

Make multiple push spec test more complete

rkeithhill force-pushed the rkeithhill/is234-fix-remote-push-tab-exp branch from 013a00c to dffe385 Compare February 2, 2017 20:40

dahlbyk merged commit 3a098c0 into master Feb 2, 2017

dahlbyk deleted the rkeithhill/is234-fix-remote-push-tab-exp branch February 2, 2017 20:50

rkeithhill mentioned this pull request Feb 2, 2017

Adds support for tab-completion of all parameters, long and short #395

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve tab expansion to handle params for push. #379

Improve tab expansion to handle params for push. #379

rkeithhill commented Jan 28, 2017

dahlbyk Jan 31, 2017

rkeithhill Jan 31, 2017

dahlbyk left a comment

dahlbyk Jan 31, 2017

rkeithhill Jan 31, 2017

dahlbyk Jan 31, 2017

dahlbyk Jan 31, 2017

rkeithhill Jan 31, 2017

dahlbyk Jan 31, 2017

rkeithhill commented Jan 31, 2017

rkeithhill commented Feb 1, 2017

dahlbyk left a comment

dahlbyk Feb 1, 2017

rkeithhill Feb 1, 2017

rkeithhill commented Feb 2, 2017

dahlbyk commented Feb 2, 2017

rkeithhill commented Feb 2, 2017

rkeithhill commented Feb 2, 2017

rkeithhill commented Feb 2, 2017

Improve tab expansion to handle params for push. #379

Improve tab expansion to handle params for push. #379

Conversation

rkeithhill commented Jan 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dahlbyk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkeithhill commented Jan 31, 2017

rkeithhill commented Feb 1, 2017

dahlbyk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkeithhill commented Feb 2, 2017

dahlbyk commented Feb 2, 2017

rkeithhill commented Feb 2, 2017

rkeithhill commented Feb 2, 2017

rkeithhill commented Feb 2, 2017