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.

Sign up for GitHub

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

Jump to bottom

Use git branch --show-current because it works when there are no commits #786

Merged
merged 3 commits into from
Mar 19, 2021
Merged

Use git branch --show-current because it works when there are no commits #786

merged 3 commits into from
Mar 19, 2021

Conversation

rhoboat
Copy link
Contributor

@rhoboat rhoboat commented Feb 16, 2021 •
edited
Loading

@rhoboat rhoboat requested a review from bwhaley February 16, 2021 18:49
@rhoboat rhoboat requested a review from yorinasub17 February 19, 2021 18:07
yorinasub17
yorinasub17 reviewed Feb 22, 2021
View reviewed changes
Copy link
Contributor

@yorinasub17 yorinasub17 left a comment

Choose a reason for hiding this comment

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

Had one minor nit for code style, but other than that LGTM!

modules/git/git.go
name := strings.TrimSpace(string(bytes))
if name == "HEAD" {
return "", nil
}
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: DRY with the logic above by extracting the git subprocess call into its own function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course!

@rhoboat
Copy link
Contributor Author

rhoboat commented Mar 18, 2021

I just force-pushed a rebase. Will take care of the nit now.

zackproser
zackproser approved these changes Mar 19, 2021
View reviewed changes
Copy link
Contributor

@zackproser zackproser left a comment

Choose a reason for hiding this comment

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

LGTM

@rhoboat rhoboat merged commit 393f87d into master Mar 19, 2021
@rhoboat rhoboat deleted the rho/branch-issue branch March 19, 2021 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yorinasub17 yorinasub17 yorinasub17 left review comments

@zackproser zackproser zackproser approved these changes

@brikis98 brikis98 Awaiting requested review from brikis98

@bwhaley bwhaley Awaiting requested review from bwhaley

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rhoboat @yorinasub17 @zackproser