Conversation
…ack to symbolic-ref
mislav
left a comment
There was a problem hiding this comment.
Thank you for tackling this!
samcoe
left a comment
There was a problem hiding this comment.
The code looks good to me. Thank you for adding tests 🙇
mislav
left a comment
There was a problem hiding this comment.
Looks great! Just a few tiny bits, please ✨
|
|
||
| func getBranchShortName(output []byte) string { | ||
| branch := firstLine(output) | ||
| return strings.TrimPrefix(branch, "refs/heads/") |
There was a problem hiding this comment.
looking at the code for git branch --show-current it looks like we maybe should also error if HEAD doesn't point to a branch (if the refs/heads/ prefix isn't present). I don't know when this could happen, but git seems to check for it: https://github.com/git/git/blob/master/builtin/branch.c#L456-L466
There was a problem hiding this comment.
the --shorten rules mention tags and remote-tracking branches https://github.com/git/git/blob/master/refs.c#L520-L528, but i can't get either of those to get reported for HEAD, so this may be a non-issue and just overly-defensive code.
$ git checkout v0.9.0
HEAD is now at bcc2c38 v0.9.0
$ git symbolic-ref HEAD
fatal: ref HEAD is not a symbolic ref
$ git checkout origin/master
Previous HEAD position was bcc2c38 v0.9.0
HEAD is now at 2822914 precompiled formik
$ git symbolic-ref HEAD
fatal: ref HEAD is not a symbolic ref
There was a problem hiding this comment.
@nightpool Thanks for the thorough review!
I'm not aware that a successful git symbolic-ref HEAD can return any values other than ones prefixed with refs/heads/, so I think the current approach is fine without adding any additional conditionals.
mislav
left a comment
There was a problem hiding this comment.
@mmontes11 Thanks for the updates! This looks great 👍
Closes #1897