-
-
Notifications
You must be signed in to change notification settings - Fork 399
Allow setting git opt set sll cert locations option in libgit2 #879
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -28,6 +28,9 @@ | |||
# Import from the future | ||||
from __future__ import absolute_import | ||||
|
||||
# Import from core python modules | ||||
from ssl import get_default_verify_paths as default_ssl_verify_paths | ||||
|
||||
# Low level API | ||||
from _pygit2 import * | ||||
|
||||
|
@@ -267,3 +270,9 @@ def clone_repository( | |||
return Repository._from_c(crepo[0], owned=True) | ||||
|
||||
settings = Settings() | ||||
|
||||
try: | ||||
# try to set default ssl cert locations | ||||
settings._ssl_cert_locations = default_ssl_verify_paths() | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this belongs at |
||||
except: | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You shouldn't catch all exceptions. Silencing everything makes any related bugs nearly impossible to locate and is harmful therefore. It's important to always fail fast and loudly.
Suggested change
|
||||
pass |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -33,7 +33,7 @@ | |||||||||||||||||||||
from _pygit2 import GIT_OPT_GET_CACHED_MEMORY | ||||||||||||||||||||||
from _pygit2 import GIT_OPT_ENABLE_CACHING | ||||||||||||||||||||||
from _pygit2 import GIT_OPT_SET_CACHE_MAX_SIZE | ||||||||||||||||||||||
|
||||||||||||||||||||||
from _pygit2 import GIT_OPT_SET_SSL_CERT_LOCATIONS | ||||||||||||||||||||||
|
||||||||||||||||||||||
class SearchPathList(object): | ||||||||||||||||||||||
|
||||||||||||||||||||||
|
@@ -101,4 +101,30 @@ def cache_object_limit(self, object_type, value): | |||||||||||||||||||||
""" | ||||||||||||||||||||||
return option(GIT_OPT_SET_CACHE_OBJECT_LIMIT, object_type, value) | ||||||||||||||||||||||
|
||||||||||||||||||||||
def __set_ssl_cert_file(self, value): | ||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||||
"""Set the ssl cert file. The value cannot be read. | ||||||||||||||||||||||
""" | ||||||||||||||||||||||
return option(GIT_OPT_SET_SSL_CERT_LOCATIONS, value, None) | ||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Setters don't return values. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When setting one value we shouldn't reset another. If a user wants to do this they can update it to |
||||||||||||||||||||||
|
||||||||||||||||||||||
ssl_cert_file = property(fset=__set_ssl_cert_file) | ||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is an ancient way of creating properties. Always use it as a decorator instead. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, we should have a getter for this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is no get support for the ssl cert paths in You can not use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But you can track it on the Python side because it's needed to properly set the other value. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you can still use a decorator: @property
def attr(self):
"""A proper docstring."""
raise AttributeError
@attr.setter
def attr(self, value):
# set val There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So here's why we need to track the values we set earlier. Semantically, these setters are assigned with the values for what their names mean. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Digging around, it looks like ultimately
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice, that's what I thought... |
||||||||||||||||||||||
|
||||||||||||||||||||||
def __set_ssl_cert_dir(self, value): | ||||||||||||||||||||||
"""Set the ssl cert folder. The value cannot be read. | ||||||||||||||||||||||
""" | ||||||||||||||||||||||
return option(GIT_OPT_SET_SSL_CERT_LOCATIONS, None, value) | ||||||||||||||||||||||
|
||||||||||||||||||||||
ssl_cert_dir = property(fset=__set_ssl_cert_dir) | ||||||||||||||||||||||
|
||||||||||||||||||||||
def __set_ssl_cert_locations(self, locations): | ||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This one must be a method. Using property here is odd. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I went with properties because all other supported settings are implemented as property. It does not make sense to break consistency. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As I wrote earlier, we should go for Pythonic principles. And if they are not respected in some places it doesn't mean that you should follow the lead. When you write in C you don't follow Assembly code-style just because it's an underlying somewhere, do you? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll look at the issues this week-end. Though we don't have a project style guide. Well, will check this week-end. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The whole paragraph: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. By the way, thank you both for the awesome work you're doing! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This rule doesn't apply here. Since all other attributes correspond to one value. With two values it doesn't make sense. Setting two values by assigning a tuple to an attribute is ridiculous in Python. And also when writing Python it doesn't have to look like another language https://www.youtube.com/watch?v=wf-BqAjZb8M. It's fine in the low-level wrapper but not in a public interface. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And I wasn't referring to just PEP8. It's quite a common sense/knowledge. PEP8 is a code style guide but I'm talking about more generic approaches which are probably not formalized explicitly but every Pythonista knows that when they see code looking like Java in |
||||||||||||||||||||||
""" | ||||||||||||||||||||||
Passes both file_path and dir_path to libgit2. | ||||||||||||||||||||||
|
||||||||||||||||||||||
locations must be a list/tuple type with the the file path | ||||||||||||||||||||||
at index 0 and the directory path at index 1 | ||||||||||||||||||||||
|
||||||||||||||||||||||
The values set cannot be read. | ||||||||||||||||||||||
""" | ||||||||||||||||||||||
return option(GIT_OPT_SET_SSL_CERT_LOCATIONS, locations[0], locations[1]) | ||||||||||||||||||||||
|
||||||||||||||||||||||
_ssl_cert_locations = property(fset=__set_ssl_cert_locations) | ||||||||||||||||||||||
|
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.
Please don't rename this callable. It's an action, not a constant. So I must start with a verb.
get
. Renaming it to constant/var-style is harmful and misleading for readability.