Repository navigation
git_repository_open_ext: fix handling of $GIT_NAMESPACE - #3992
pks-t merged 1 commit into
Conversation
Sorry, something went wrong.
pks-t
left a comment
There was a problem hiding this comment.
The fix looks good, but I think previously existing code could be refactored a bit to improve readability here
Sorry, something went wrong.
| if (error < 0) | ||
| goto error; | ||
| } | ||
| error = 0; |
There was a problem hiding this comment.
This looks a bit backwards here, as you are in fact not clearing the error of the leading block but instead the error from the previous git__getenv. It would be much clearer if you'd instead clear the error in the previous if (error == GIT_ENOTFOUND) condition, where we already call giterr_clear();.
Sorry, something went wrong.
There was a problem hiding this comment.
An additional note, which is not your fault but might be improved when we're already touching this. I think the code would be nicer if you moved the call to git__getenv and setting the namespace together. So you could just move down getting the environment variable and then set the namespace based on the function's return value.
Sorry, something went wrong.
The existing code would set a namespace of "" (empty string) with GIT_NAMESPACE unset. In a repository where refs/heads/namespaces/ exists, that can produce incorrect results. Detect that case and avoid setting the namespace at all. Since that makes the last assignment to error conditional, and the previous assignment can potentially get GIT_ENOTFOUND, set error to 0 explicitly to prevent the call from incorrectly failing with GIT_ENOTFOUND.
|
@pks-t Thanks for the review. Updated to move the error handling to a clearer location. |
Sorry, something went wrong.
|
Looks good now, thanks for this 👍 |
Sorry, something went wrong.
The existing code would set a namespace of "" (empty string) with
GIT_NAMESPACE unset. In a repository where refs/heads/namespaces/
exists, that can produce incorrect results. Detect that case and avoid
setting the namespace at all.
Since that makes the last assignment to error, and the previous
assignment can potentially get GIT_ENOTFOUND, set error to 0 explicitly
to prevent the call from incorrectly failing with GIT_ENOTFOUND.