git_repository_open_ext: fix handling of $GIT_NAMESPACE#3992
Conversation
pks-t
left a comment
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
| 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();.
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.
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.
joshtriplett
force-pushed
the
env-namespace
branch
from
1a036af to
c9e967a
Compare
|
@pks-t Thanks for the review. Updated to move the error handling to a clearer location. |
|
Looks good now, thanks for this 👍 |
2 participants
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.