Have git_branch_lookup accept GIT_BRANCH_ALL#5000
Have git_branch_lookup accept GIT_BRANCH_ALL#5000ethomson merged 2 commits intolibgit2:masterfrom augfab:branch_lookup_all
Conversation
ethomson
left a comment
There was a problem hiding this comment.
Cool! Thanks for tackling this. I have just a few minor issues to request that you address, if you don't mind.
| break; | ||
| case GIT_BRANCH_ALL: | ||
| error = retrieve_branch_reference(ref_out, repo, branch_name, false); | ||
| if (error < 0) |
There was a problem hiding this comment.
We should probably only retry when error == GIT_ENOTFOUND, otherwise we might hide potentially fatal errors by ignoring them and doing something else.
| error = retrieve_branch_reference(ref_out, repo, branch_name, true); | ||
| break; | ||
| default: | ||
| abort(); |
There was a problem hiding this comment.
abort is probably a bit harsh here. I think assert(0) is a reasonable compromise that we've used in the past.
| cl_git_pass(git_branch_lookup(&branch, repo, "br2", GIT_BRANCH_LOCAL)); | ||
| cl_git_pass(git_branch_lookup(&branch, repo, "br2", GIT_BRANCH_ALL)); | ||
| cl_git_fail(git_branch_lookup(&branch, repo, "br2", GIT_BRANCH_REMOTE)); | ||
| } |
There was a problem hiding this comment.
You're leaking memory here, namely the results of the prior git_branch_lookup that are hidden with the new assignment to branch. Probably best to just make each one of these calls their own test function
There was a problem hiding this comment.
I was scratching my head about that, thanks for the explanation 🙂.
|
Thanks a lot @ethomson for the review! |
|
Awesome, thanks @augfab for the PR! |
This PR is a proposed fix for issue #4938.