-
Notifications
You must be signed in to change notification settings - Fork 203
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
Add mercurial repository support. #979
Conversation
Automatic reply from Jenkins: Can I test this? |
try: | ||
HgCommandError | ||
except NameError, err: | ||
self.log.exception("It seems like python-hglib is not available: %s" % err) |
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.
Can you not just use HAVE_HG
for 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.
+1, what's the use of HAVE_HG
if it's not being used?
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.
If you're up to it, fix this in the svnrepo.py
and gitrepo.py
modules too.
I don't know why we used this approach, using the HAVE_X
constants makes more sense.
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.
also, it should use self.log.error
:
if not HAVE_HG:
self.log.error("The python-hglib Python module is not available, which is required for Mercurial support.")
ok to test |
retest this please |
@author: Cedric Clerget (University of Franche-Comte) | ||
""" | ||
import getpass | ||
import os |
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.
not used? so remove please
@cclerget: mostly remarks w.r.t. style, some also apply to the existing Python modules in |
@cclerget: if you've tackled the remarks, please issue a comment to notify us (there are no notifications for pushed commits) |
except NameError, err: | ||
self.log.exception("It seems like GitPython is not available: %s" % err) | ||
if not HAVE_GIT: | ||
self.log.error("It seems like GitPython is not available, which is required for Git support.") |
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.
Can this point ever be reached? Will the USABLE
properties not prevent from ever getting here?
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.
the USABLE
is only used by EB, but what if someone creates their own GitRepository
instance and calls setup_repo
? You want to make sure things don't go down hard...
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.
true, then again, the uses of logger makes it hard to use this outside EB (as I noticed in the clean up script).
Test PASSed. |
git.GitCommandError | ||
except NameError, err: | ||
self.log.exception("It seems like GitPython is not available: %s" % err) | ||
if not HAVE_GIT: |
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.
I thought the catching exception was used to make sure git/svn binary was present on system and I was wrong. What you think about an additional check on binary presence ?
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.
I'd say that's the job of the respective Python packages being used?
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.
Yes and only git and hglib module are concerned. Just apply a minor fix to catch OSError exception for cloning step.
Test PASSed. |
conflicts with current |
tagging for v2.3... |
merged via #1446 |
Unit tests are not coded for the moment, I would like to discuss if a tiny mercurial repo should be embedded in source tree or remotely on bitbucket or others.