fix regression in support for detached HEAD state#1155
Conversation
for gh pr status
vilmibm
left a comment
There was a problem hiding this comment.
thanks, this looks great! I'd like to see an additional test added; otherwise 👍
context/context.go
Outdated
|
|
||
| currentBranch, err := git.CurrentBranch() | ||
| if err != nil { | ||
| switch err { |
There was a problem hiding this comment.
I looked through git history and we already had a commit trying to fix this; we regressed because there's no test. I'd like a test added that invokes gh pr status in a no-branch situation to cover this case moving forward.
There was a problem hiding this comment.
I think I've addressed this concern to a degree.
- update the "blank context" mock to return an
ErrNotOnAnyBranchif no branch has been set. - add a test to
pr_test.gothat checks that the command runs to completion if there is no branch.
I don't think this is completely foolproof, since it relies on the mock's behavior staying synchronized with the regular filesystem context, so I'm not 100% happy with it.
But I didn't see any existing tests that set up a real Git repository (on a real filesystem, using real git), and I'm assuming that you don't want to add that with this change.
There was a problem hiding this comment.
I was imagining using our CmdStubber's StubError to have the git branch call result in the expected error. Sorry for the back and forth but I'd rather see that approach (unless there's a reason why it wouldn't work that I'm forgetting about).
mislav
left a comment
There was a problem hiding this comment.
Thanks for taking care of this!
for
gh pr statusfixes #1132