From d84b960982b5bad0b3c78c4a680638824924004b Mon Sep 17 00:00:00 2001 From: Kostis Anagnostopoulos Date: Sat, 1 Oct 2016 03:50:12 +0200 Subject: cfg_TCs, #519: FIX config resource leaks + Modify lock/read-config-file code to ansure files closed + Use `with GitConfigarser()` more systematically in TCs. + Clear any locks left hanging from pev Tcs --- git/config.py | 60 +++++++++++++++++++++++------------------------------------ 1 file changed, 23 insertions(+), 37 deletions(-) (limited to 'git/config.py') diff --git a/git/config.py b/git/config.py index 5bd10975..ad6192ff 100644 --- a/git/config.py +++ b/git/config.py @@ -388,23 +388,18 @@ class GitConfigParser(with_metaclass(MetaParserBuilder, cp.RawConfigParser, obje while files_to_read: file_path = files_to_read.pop(0) fp = file_path - close_fp = False + file_ok = False - # assume a path if it is not a file-object - if not hasattr(fp, "seek"): + if hasattr(fp, "seek"): + self._read(fp, fp.name) + else: + # assume a path if it is not a file-object try: - fp = open(file_path, 'rb') - close_fp = True + with open(file_path, 'rb') as fp: + file_ok = True + self._read(fp, fp.name) except IOError: continue - # END fp handling - - try: - self._read(fp, fp.name) - finally: - if close_fp: - fp.close() - # END read-handling # Read includes and append those that we didn't handle yet # We expect all paths to be normalized and absolute (and will assure that is the case) @@ -413,7 +408,7 @@ class GitConfigParser(with_metaclass(MetaParserBuilder, cp.RawConfigParser, obje if include_path.startswith('~'): include_path = os.path.expanduser(include_path) if not os.path.isabs(include_path): - if not close_fp: + if not file_ok: continue # end ignore relative paths if we don't know the configuration file path assert os.path.isabs(file_path), "Need absolute paths to be sure our cycle checks will work" @@ -477,34 +472,25 @@ class GitConfigParser(with_metaclass(MetaParserBuilder, cp.RawConfigParser, obje # end fp = self._file_or_files - close_fp = False # we have a physical file on disk, so get a lock - if isinstance(fp, string_types + (FileType, )): + is_file_lock = isinstance(fp, string_types + (FileType, )) + if is_file_lock: self._lock._obtain_lock() - # END get lock for physical files - - if not hasattr(fp, "seek"): - fp = open(self._file_or_files, "wb") - close_fp = True - else: - fp.seek(0) - # make sure we do not overwrite into an existing file - if hasattr(fp, 'truncate'): - fp.truncate() - # END - # END handle stream or file - - # WRITE DATA try: - self._write(fp) + if not hasattr(fp, "seek"): + with open(self._file_or_files, "wb") as fp: + self._write(fp) + else: + fp.seek(0) + # make sure we do not overwrite into an existing file + if hasattr(fp, 'truncate'): + fp.truncate() + self._write(fp) finally: - if close_fp: - fp.close() - # END data writing - - # we do not release the lock - it will be done automatically once the - # instance vanishes + # we release the lock - it will not vanish automatically in PY3.5+ + if is_file_lock: + self._lock._release_lock() def _assure_writable(self, method_name): if self.read_only: -- cgit v1.2.1 From 51f4a1407ef12405e16f643f5f9d2002b4b52ab9 Mon Sep 17 00:00:00 2001 From: Yaroslav Halchenko Date: Sun, 2 Oct 2016 11:17:36 -0400 Subject: RF: use @functools.wraps within decorators instead of manual __name__ reassignment @wraps does more and does it right ;) --- git/config.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) (limited to 'git/config.py') diff --git a/git/config.py b/git/config.py index ad6192ff..b342410c 100644 --- a/git/config.py +++ b/git/config.py @@ -17,6 +17,8 @@ import logging import abc import os +from functools import wraps + from git.odict import OrderedDict from git.util import LockFile from git.compat import ( @@ -67,11 +69,11 @@ class MetaParserBuilder(abc.ABCMeta): def needs_values(func): """Returns method assuring we read values (on demand) before we try to access them""" + @wraps(func) def assure_data_present(self, *args, **kwargs): self.read() return func(self, *args, **kwargs) # END wrapper method - assure_data_present.__name__ = func.__name__ return assure_data_present -- cgit v1.2.1 From 8a01ec439e19df83a2ff17d198118bd5a31c488b Mon Sep 17 00:00:00 2001 From: Kostis Anagnostopoulos Date: Mon, 3 Oct 2016 00:37:37 +0200 Subject: FIX config-lock release early regression caused by #519 + Regression introduced in d84b960982b, by a wrong comment interpretation. --- git/config.py | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-) (limited to 'git/config.py') diff --git a/git/config.py b/git/config.py index b342410c..3c6a32eb 100644 --- a/git/config.py +++ b/git/config.py @@ -479,20 +479,15 @@ class GitConfigParser(with_metaclass(MetaParserBuilder, cp.RawConfigParser, obje is_file_lock = isinstance(fp, string_types + (FileType, )) if is_file_lock: self._lock._obtain_lock() - try: - if not hasattr(fp, "seek"): - with open(self._file_or_files, "wb") as fp: - self._write(fp) - else: - fp.seek(0) - # make sure we do not overwrite into an existing file - if hasattr(fp, 'truncate'): - fp.truncate() + if not hasattr(fp, "seek"): + with open(self._file_or_files, "wb") as fp: self._write(fp) - finally: - # we release the lock - it will not vanish automatically in PY3.5+ - if is_file_lock: - self._lock._release_lock() + else: + fp.seek(0) + # make sure we do not overwrite into an existing file + if hasattr(fp, 'truncate'): + fp.truncate() + self._write(fp) def _assure_writable(self, method_name): if self.read_only: -- cgit v1.2.1 From a469af892b3e929cbe9d29e414b6fcd59bec246e Mon Sep 17 00:00:00 2001 From: Kostis Anagnostopoulos Date: Mon, 3 Oct 2016 23:35:55 +0200 Subject: src: No PyDev warnings + Mark all unused vars and other non-pep8 (PyDev) warnings + test_utils: + enable & fix forgotten IterableList looped path. + unittestize all assertions. + remote: minor fix progress dispatching unknown err-lines --- git/config.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'git/config.py') diff --git a/git/config.py b/git/config.py index 3c6a32eb..eddfac15 100644 --- a/git/config.py +++ b/git/config.py @@ -40,7 +40,7 @@ log.addHandler(logging.NullHandler()) class MetaParserBuilder(abc.ABCMeta): """Utlity class wrapping base-class methods into decorators that assure read-only properties""" - def __new__(metacls, name, bases, clsdict): + def __new__(cls, name, bases, clsdict): """ Equip all base-class methods with a needs_values decorator, and all non-const methods with a set_dirty_and_flush_changes decorator in addition to that.""" @@ -62,7 +62,7 @@ class MetaParserBuilder(abc.ABCMeta): # END for each base # END if mutating methods configuration is set - new_type = super(MetaParserBuilder, metacls).__new__(metacls, name, bases, clsdict) + new_type = super(MetaParserBuilder, cls).__new__(cls, name, bases, clsdict) return new_type -- cgit v1.2.1