FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

cfg_TCs, #519: FIX config resource leaks · gitpython-developers/GitPython@d84b960 · GitHub

  1. gitpython-developers
  2. GitPython

Repository navigation

Commit d84b960

Browse files
ankostis
committed
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
1 parent b114f3b commit d84b960

2 files changed

Lines changed: 116 additions & 115 deletions

File tree

‎git/config.py‎

Lines changed: 23 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -388,23 +388,18 @@ def read(self):
388388
while files_to_read:
389389
file_path = files_to_read.pop(0)
390390
fp = file_path
391-
close_fp = False
391+
file_ok = False
392392

393-
# assume a path if it is not a file-object
394-
if not hasattr(fp, "seek"):
393+
if hasattr(fp, "seek"):
394+
self._read(fp, fp.name)
395+
else:
396+
# assume a path if it is not a file-object
395397
try:
396-
fp = open(file_path, 'rb')
397-
close_fp = True
398+
with open(file_path, 'rb') as fp:
399+
file_ok = True
400+
self._read(fp, fp.name)
398401
except IOError:
399402
continue
400-
# END fp handling
401-
402-
try:
403-
self._read(fp, fp.name)
404-
finally:
405-
if close_fp:
406-
fp.close()
407-
# END read-handling
408403

409404
# Read includes and append those that we didn't handle yet
410405
# We expect all paths to be normalized and absolute (and will assure that is the case)
@@ -413,7 +408,7 @@ def read(self):
413408
if include_path.startswith('~'):
414409
include_path = os.path.expanduser(include_path)
415410
if not os.path.isabs(include_path):
416-
if not close_fp:
411+
if not file_ok:
417412
continue
418413
# end ignore relative paths if we don't know the configuration file path
419414
assert os.path.isabs(file_path), "Need absolute paths to be sure our cycle checks will work"
@@ -477,34 +472,25 @@ def write(self):
477472
# end
478473

479474
fp = self._file_or_files
480-
close_fp = False
481475

482476
# we have a physical file on disk, so get a lock
483-
if isinstance(fp, string_types + (FileType, )):
477+
is_file_lock = isinstance(fp, string_types + (FileType, ))
478+
if is_file_lock:
484479
self._lock._obtain_lock()
485-
# END get lock for physical files
486-
487-
if not hasattr(fp, "seek"):
488-
fp = open(self._file_or_files, "wb")
489-
close_fp = True
490-
else:
491-
fp.seek(0)
492-
# make sure we do not overwrite into an existing file
493-
if hasattr(fp, 'truncate'):
494-
fp.truncate()
495-
# END
496-
# END handle stream or file
497-
498-
# WRITE DATA
499480
try:
500-
self._write(fp)
481+
if not hasattr(fp, "seek"):
482+
with open(self._file_or_files, "wb") as fp:
483+
self._write(fp)
484+
else:
485+
fp.seek(0)
486+
# make sure we do not overwrite into an existing file
487+
if hasattr(fp, 'truncate'):
488+
fp.truncate()
489+
self._write(fp)
501490
finally:
502-
if close_fp:
503-
fp.close()
504-
# END data writing
505-
506-
# we do not release the lock - it will be done automatically once the
507-
# instance vanishes
491+
# we release the lock - it will not vanish automatically in PY3.5+
492+
if is_file_lock:
493+
self._lock._release_lock()
508494

509495
def _assure_writable(self, method_name):
510496
if self.read_only:

‎git/test/test_config.py‎

Lines changed: 93 additions & 78 deletions
Original file line numberDiff line numberDiff line change
@@ -4,80 +4,98 @@
44
# This module is part of GitPython and is released under
55
# the BSD License: http://www.opensource.org/licenses/bsd-license.php
66

7-
from git.test.lib import (
8-
TestCase,
9-
fixture_path,
10-
assert_equal,
11-
)
12-
from git.test.lib import with_rw_directory
7+
import glob
8+
import io
9+
import os
10+
1311
from git import (
1412
GitConfigParser
1513
)
1614
from git.compat import (
1715
string_types,
18-
)
19-
import io
20-
import os
16+
is_win,)
2117
from git.config import cp
18+
from git.test.lib import (
19+
TestCase,
20+
fixture_path,
21+
)
22+
from git.test.lib import with_rw_directory
23+
24+
import os.path as osp
25+
26+
27+
_tc_lock_fpaths = osp.join(osp.dirname(__file__), 'fixtures/*.lock')
28+
29+
30+
def _rm_lock_files():
31+
for lfp in glob.glob(_tc_lock_fpaths):
32+
if is_win and osp.isfile(lfp):
33+
os.chmod(lfp, 0o777)
34+
os.remove(lfp)
2235

2336

2437
class TestBase(TestCase):
38+
def setUp(self):
39+
_rm_lock_files()
40+
41+
def tearDown(self):
42+
for lfp in glob.glob(_tc_lock_fpaths):
43+
if osp.isfile(lfp):
44+
raise AssertionError('Previous TC left hanging git-lock file: %s', lfp)
2545

2646
def _to_memcache(self, file_path):
27-
fp = open(file_path, "rb")
28-
sio = io.BytesIO(fp.read())
47+
with open(file_path, "rb") as fp:
48+
sio = io.BytesIO(fp.read())
2949
sio.name = file_path
3050
return sio
3151

3252
def test_read_write(self):
3353
# writer must create the exact same file as the one read before
3454
for filename in ("git_config", "git_config_global"):
3555
file_obj = self._to_memcache(fixture_path(filename))
36-
w_config = GitConfigParser(file_obj, read_only=False)
37-
w_config.read() # enforce reading
38-
assert w_config._sections
39-
w_config.write() # enforce writing
40-
41-
# we stripped lines when reading, so the results differ
42-
assert file_obj.getvalue()
43-
self.assertEqual(file_obj.getvalue(), self._to_memcache(fixture_path(filename)).getvalue())
44-
45-
# creating an additional config writer must fail due to exclusive access
46-
self.failUnlessRaises(IOError, GitConfigParser, file_obj, read_only=False)
47-
48-
# should still have a lock and be able to make changes
49-
assert w_config._lock._has_lock()
50-
51-
# changes should be written right away
52-
sname = "my_section"
53-
oname = "mykey"
54-
val = "myvalue"
55-
w_config.add_section(sname)
56-
assert w_config.has_section(sname)
57-
w_config.set(sname, oname, val)
58-
assert w_config.has_option(sname, oname)
59-
assert w_config.get(sname, oname) == val
60-
61-
sname_new = "new_section"
62-
oname_new = "new_key"
63-
ival = 10
64-
w_config.set_value(sname_new, oname_new, ival)
65-
assert w_config.get_value(sname_new, oname_new) == ival
66-
67-
file_obj.seek(0)
68-
r_config = GitConfigParser(file_obj, read_only=True)
69-
assert r_config.has_section(sname)
70-
assert r_config.has_option(sname, oname)
71-
assert r_config.get(sname, oname) == val
72-
w_config.release()
56+
with GitConfigParser(file_obj, read_only=False) as w_config:
57+
w_config.read() # enforce reading
58+
assert w_config._sections
59+
w_config.write() # enforce writing
60+
61+
# we stripped lines when reading, so the results differ
62+
assert file_obj.getvalue()
63+
self.assertEqual(file_obj.getvalue(), self._to_memcache(fixture_path(filename)).getvalue())
64+
65+
# creating an additional config writer must fail due to exclusive access
66+
self.failUnlessRaises(IOError, GitConfigParser, file_obj, read_only=False)
67+
68+
# should still have a lock and be able to make changes
69+
assert w_config._lock._has_lock()
70+
71+
# changes should be written right away
72+
sname = "my_section"
73+
oname = "mykey"
74+
val = "myvalue"
75+
w_config.add_section(sname)
76+
assert w_config.has_section(sname)
77+
w_config.set(sname, oname, val)
78+
assert w_config.has_option(sname, oname)
79+
assert w_config.get(sname, oname) == val
80+
81+
sname_new = "new_section"
82+
oname_new = "new_key"
83+
ival = 10
84+
w_config.set_value(sname_new, oname_new, ival)
85+
assert w_config.get_value(sname_new, oname_new) == ival
86+
87+
file_obj.seek(0)
88+
r_config = GitConfigParser(file_obj, read_only=True)
89+
assert r_config.has_section(sname)
90+
assert r_config.has_option(sname, oname)
91+
assert r_config.get(sname, oname) == val
7392
# END for each filename
7493

7594
@with_rw_directory
7695
def test_lock_reentry(self, rw_dir):
7796
fpl = os.path.join(rw_dir, 'l')
78-
gcp = GitConfigParser(fpl, read_only=False)
79-
with gcp as cw:
80-
cw.set_value('include', 'some_value', 'a')
97+
with GitConfigParser(fpl, read_only=False) as gcp:
98+
gcp.set_value('include', 'some_value', 'a')
8199
# entering again locks the file again...
82100
with gcp as cw:
83101
cw.set_value('include', 'some_other_value', 'b')
@@ -91,21 +109,21 @@ def test_lock_reentry(self, rw_dir):
91109

92110
def test_multi_line_config(self):
93111
file_obj = self._to_memcache(fixture_path("git_config_with_comments"))
94-
config = GitConfigParser(file_obj, read_only=False)
95-
ev = "ruby -e '\n"
96-
ev += " system %(git), %(merge-file), %(--marker-size=%L), %(%A), %(%O), %(%B)\n"
97-
ev += " b = File.read(%(%A))\n"
98-
ev += " b.sub!(/^<+ .*\\nActiveRecord::Schema\\.define.:version => (\\d+). do\\n=+\\nActiveRecord::Schema\\."
99-
ev += "define.:version => (\\d+). do\\n>+ .*/) do\n"
100-
ev += " %(ActiveRecord::Schema.define(:version => #{[$1, $2].max}) do)\n"
101-
ev += " end\n"
102-
ev += " File.open(%(%A), %(w)) {|f| f.write(b)}\n"
103-
ev += " exit 1 if b.include?(%(<)*%L)'"
104-
assert_equal(config.get('merge "railsschema"', 'driver'), ev)
105-
assert_equal(config.get('alias', 'lg'),
106-
"log --graph --pretty=format:'%Cred%h%Creset -%C(yellow)%d%Creset %s %Cgreen(%cr)%Creset'"
107-
" --abbrev-commit --date=relative")
108-
assert len(config.sections()) == 23
112+
with GitConfigParser(file_obj, read_only=False) as config:
113+
ev = "ruby -e '\n"
114+
ev += " system %(git), %(merge-file), %(--marker-size=%L), %(%A), %(%O), %(%B)\n"
115+
ev += " b = File.read(%(%A))\n"
116+
ev += " b.sub!(/^<+ .*\\nActiveRecord::Schema\\.define.:version => (\\d+). do\\n=+\\nActiveRecord::Schema\\." # noqa E501
117+
ev += "define.:version => (\\d+). do\\n>+ .*/) do\n"
118+
ev += " %(ActiveRecord::Schema.define(:version => #{[$1, $2].max}) do)\n"
119+
ev += " end\n"
120+
ev += " File.open(%(%A), %(w)) {|f| f.write(b)}\n"
121+
ev += " exit 1 if b.include?(%(<)*%L)'"
122+
self.assertEqual(config.get('merge "railsschema"', 'driver'), ev)
123+
self.assertEqual(config.get('alias', 'lg'),
124+
"log --graph --pretty=format:'%Cred%h%Creset -%C(yellow)%d%Creset %s %Cgreen(%cr)%Creset'"
125+
" --abbrev-commit --date=relative")
126+
self.assertEqual(len(config.sections()), 23)
109127

110128
def test_base(self):
111129
path_repo = fixture_path("git_config")
@@ -202,22 +220,19 @@ def check_test_value(cr, value):
202220

203221
def test_rename(self):
204222
file_obj = self._to_memcache(fixture_path('git_config'))
205-
cw = GitConfigParser(file_obj, read_only=False, merge_includes=False)
206-
207-
self.failUnlessRaises(ValueError, cw.rename_section, "doesntexist", "foo")
208-
self.failUnlessRaises(ValueError, cw.rename_section, "core", "include")
223+
with GitConfigParser(file_obj, read_only=False, merge_includes=False) as cw:
224+
self.failUnlessRaises(ValueError, cw.rename_section, "doesntexist", "foo")
225+
self.failUnlessRaises(ValueError, cw.rename_section, "core", "include")
209226

210-
nn = "bee"
211-
assert cw.rename_section('core', nn) is cw
212-
assert not cw.has_section('core')
213-
assert len(cw.items(nn)) == 4
214-
cw.release()
227+
nn = "bee"
228+
assert cw.rename_section('core', nn) is cw
229+
assert not cw.has_section('core')
230+
assert len(cw.items(nn)) == 4
215231

216232
def test_complex_aliases(self):
217233
file_obj = self._to_memcache(fixture_path('.gitconfig'))
218-
w_config = GitConfigParser(file_obj, read_only=False)
219-
self.assertEqual(w_config.get('alias', 'rbi'), '"!g() { git rebase -i origin/${1:-master} ; } ; g"')
220-
w_config.release()
234+
with GitConfigParser(file_obj, read_only=False) as w_config:
235+
self.assertEqual(w_config.get('alias', 'rbi'), '"!g() { git rebase -i origin/${1:-master} ; } ; g"')
221236
self.assertEqual(file_obj.getvalue(), self._to_memcache(fixture_path('.gitconfig')).getvalue())
222237

223238
def test_empty_config_value(self):

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL