René's URL Explorer Experiment

Title: Fix mkdir race condition in LooseObjectDB.store by EliahKagan · Pull Request #91 · gitpython-developers/gitdb · GitHub

Open Graph Title: Fix mkdir race condition in LooseObjectDB.store by EliahKagan · Pull Request #91 · gitpython-developers/gitdb

X Title: Fix mkdir race condition in LooseObjectDB.store by EliahKagan · Pull Request #91 · gitpython-developers/gitdb

Description: Fixes #85 This replaces the conditional call to os.mkdir that raises an unintended FileExistsError if the directory is created between the check and the os.mkdir call, using a single os.makedirs call instead, with exist_ok=True. This way, we attempt creation in a way that produces no error if the directory is already present, while still raising FileExistsError if a non-directory filesystem entry (such as a regular file) is present where we want the directory to be. This is the advantage of this approach over the approach of swallowing FileExistError as suggested in #85. Note, however, that os.makedirs behaves like mkdir -p: it attempts to create parent directories (and their parents, etc.) if they do not already exist. So it should only be used if that is acceptable in this case. I am not aware of a reason it wouldn't be, but I am not very familiar with gitdb. So that aspect of the situation deserves special consideration in reviewing this PR. I'd be pleased to change the approach if os.makdirs is judged not suitable here. I think the approach suggested in #85 is reasonable, and it can be made more robust by checking that the directory exists after the creation attempt (or in other ways). The code was under test: that line is exercised in TestExamples.test_base, TestGitDB.test_writing, TestLooseDB.test_basics, and TestObjDBPerformance.test_large_data_streaming. However, no test catches the race condition this fixes, and I have not added one. Testing that the race condition does not occur in the specific way as before by accessing and calling the same functions as before in the same order would be easy, but it would be more of an illusion of a regression test than a useful test. Testing by trying to brute-force a race condition, without modifying the operation of the code for the test, would work but the tests would take a very long time to run. Testing it in a way that is fairly robust against new ways of reintroducing the race condition and that is not too slow should be possible, but I don't know of a good way to do it; everything I've thought of would be complicated, and possibly make running the test in a debugger like pdb infeasible. So I have not added a regression test for this bug. However, if it is considered important to have one, then I can consider the matter further.

Open Graph Description: Fixes #85 This replaces the conditional call to os.mkdir that raises an unintended FileExistsError if the directory is created between the check and the os.mkdir call, using a single os.makedirs ca...

X Description: Fixes #85 This replaces the conditional call to os.mkdir that raises an unintended FileExistsError if the directory is created between the check and the os.mkdir call, using a single os.makedirs ca...

Opengraph URL: https://github.com/gitpython-developers/gitdb/pull/91

X: @github

direct link

Domain: github.com

Links:

Viewport: width=device-width