-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Detect all registered VCS and choose inner-most #7593
Conversation
5a46eec
to
4796ea2
Compare
4796ea2
to
62ffc7b
Compare
except BadCommand: | ||
logger.debug("could not determine if %s is under git control " | ||
"because git is not available", location) | ||
return False | ||
return None | ||
except InstallationError: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems weird, I'd say call_subprocess
should not raise InstallationError
which is too high-level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is definitely not right, but I’d say this belongs to another (refactoring) PR. Note that the Mercurial implementation already catches InstallationError
right now.
Soft ping, I believe I have addressed all comments. There are a few follow-up improvements I am looking to do after this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very minor comments, otherwise this looks good to me.
Co-Authored-By: Christopher Hunt <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks all! |
Fix #3988.
The idea (as describe in #3988 (comment)) is to test all VCS, and choose the one that controls the inner-most directory as root. Git provides this value via
git rev-parse --show-toplevel
, and Mercurialhg root
.The implementation seems to work from my limited testing, but there are still much to do. A test, at least. And the VCS code modified in this PR probably could use some refactoring as well.