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

Fix duplicated branch completion for git checkout issue 505 #506

Conversation

bergmeister
Copy link
Contributor

Fixes #505 by outputting only unique branches.
Instead of the proposed Sort-Object -Unique, I used Select-Object -Unique because:

  • the 3 arrays for local branches, remote branches and tags are already sorted by themselves
  • in order to not break existing behaviour and only fix the bug
  • most user are less interested in tags and remote branches when using git checkout, therefore keeping the order as it was before makes sense
  • better performance

gitBranches $matches['ref'] $true
gitRemoteUniqueBranches $matches['ref']
gitTags $matches['ref']
$script:gitBranches = @(gitBranches $matches['ref'] $true)
Copy link
Collaborator

Choose a reason for hiding this comment

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

You aren't trying to cache the branches, so you probably shouldn't save them in a script scope variable.
I think using the pipeline like this might be a bit more natural and avoids creating multiple arrays:

& {
    gitBranches $matches['ref'] $true
    gitRemoteUniqueBranches $matches['ref']
    gitTags $matches['ref']
} | Select-Object -Unique

You could use a function instead of the script block like I used here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion, you're right that this is the more efficient way of doing it.

& {
@(gitBranches $matches['ref'] $true)
@(gitRemoteUniqueBranches $matches['ref'])
@(gitTags $matches['ref'])
Copy link
Owner

Choose a reason for hiding this comment

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

Shouldn't need the surrounding @(...) on these.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well spotted, this was a left over from the previous version when I was using arrays.

@dahlbyk dahlbyk merged commit d0a9421 into dahlbyk:develop Dec 14, 2017
@dahlbyk
Copy link
Owner

dahlbyk commented Dec 14, 2017

I just noticed this is targeting develop; I've opened #512 against master.

Still wish GitHub supported retargeting PRs...

@bergmeister
Copy link
Contributor Author

Hmm, isn't that what gitflow would be for (except that it is from the command line)

@dahlbyk
Copy link
Owner

dahlbyk commented Dec 18, 2017

Despite the alignment of branch names, we don't use Git Flow. master is still targeting v0.x, develop is code with breaking changes working toward v1.0.

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.

3 participants