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

Git: move current_branch() to core #4420

Merged
merged 1 commit into from Dec 15, 2015

Conversation

apjanke
Copy link
Contributor

@apjanke apjanke commented Sep 28, 2015

Moves the current_branch() function from the git plugin to the core lib/git.zsh, since there are other functions in core OMZ or in themes that depend on it, and we don't want any core -> plugin dependencies.

Fixes #4085
Fixes #4059
Fixes #331
Fixes #4581

@robbyrussell robbyrussell added git Status: testers needed Pull Requests that are waiting for testers to merge Area: core Issue or PR related to core parts of the project labels Oct 3, 2015
# it's not a symbolic ref, but in a Git repo.
function git_current_branch() {
local ref
ref=$(git symbolic-ref --quiet HEAD 2> /dev/null)
Copy link
Contributor

Choose a reason for hiding this comment

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

  • That should be command git if you do not mind ;-)
  • Same on line 71.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't mind at all. Amended.

@ncanceill
Copy link
Contributor

👍

This is sooo much needed!

Tested OK all is well. Just the small change I noted inline to apply. Also please state in your description message that this fixes #4059.

Excellent job @apjanke, you got them all current_branch calls!

@apjanke apjanke force-pushed the git-move-current_branch-to-core branch from ffff1fd to e809c59 Compare October 16, 2015 22:40
@apjanke
Copy link
Contributor Author

apjanke commented Oct 16, 2015

Added the commands and "Fixes #4059".

@ncanceill
Copy link
Contributor

Weird, it seems you re-pushed the exact same commit... maybe a bug with GitHub?

@apjanke apjanke force-pushed the git-move-current_branch-to-core branch from e809c59 to 4dc89aa Compare October 17, 2015 21:30
@apjanke
Copy link
Contributor Author

apjanke commented Oct 17, 2015

No, I must have screwed up somehow: the change to add the commands was missing in my local copy too. I bet I lost it while trying to rebase off of master. Re-commited and re-poshed; looks like it's there now.

@ncanceill
Copy link
Contributor

Then I vote for a merge ASAP!

@apjanke
Copy link
Contributor Author

apjanke commented Nov 6, 2015

Ping... can we merge this? @mcornella

@mcornella
Copy link
Member

Hi @apjanke, sorry for missing this one. I was going to merge but the commit message is leaking over multiple lines. Please edit that with a line break of separation between the subject and description of the commit.

Fixes ohmyzsh#4085: core -> plugin dependency issue.
Rename it to git_current_branch for clarity that it's git-specific.
Update all plugins that were calling it to use new name.
Fix variable leaks by making more variables in lib/git.zsh local.
Have lib/git.zsh use [[ ]] instead of [ ] everywhere.
@apjanke apjanke force-pushed the git-move-current_branch-to-core branch from 4dc89aa to 9f55213 Compare December 15, 2015 01:29
mcornella added a commit that referenced this pull request Dec 15, 2015
@mcornella mcornella merged commit e344f4c into ohmyzsh:master Dec 15, 2015
@apjanke apjanke deleted the git-move-current_branch-to-core branch December 15, 2015 03:18
@ncanceill
Copy link
Contributor

w00t

nunogt pushed a commit to nunogt/oh-my-zsh that referenced this pull request Jan 25, 2016
…to-core

Git: move current_branch() to core
@mcornella mcornella removed the Status: testers needed Pull Requests that are waiting for testers to merge label Oct 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: core Issue or PR related to core parts of the project
Projects
None yet
5 participants