Skip to content

python3 specific: seems to wipe out entire .git/config upon #333

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

Open
yarikoptic opened this issue Jul 30, 2015 · 4 comments
Open

python3 specific: seems to wipe out entire .git/config upon #333

yarikoptic opened this issue Jul 30, 2015 · 4 comments

Comments

@yarikoptic
Copy link
Contributor

uff -- that is a nasty one ;) Our piece of code is

repo.config_writer().set_value("annex", "backends", backend)

where backend is a str (python3). And it seems to set the value properly and actual wipe out of the file doesn't happen until this writer (I guess) gets GCed -- I force that in our test now with gc.collect() and it results then in .git/config of that repo being wiped (0 byte size).
I have upgraded even to current master in that tox env, and the same result. Let me know if you need more details. python is 3.4.3, Debian amd64 sid/testing

yarikoptic added a commit to yarikoptic/datalad that referenced this issue Jul 30, 2015
@Byron Byron added this to the v1.0.2 - Fixes milestone Jul 31, 2015
@Byron
Copy link
Member

Byron commented Jul 31, 2015

Unfortunately, this issue cannot be fixed on the side of GitPython. Back in the days, I wrote the code as if there were deterministic destructors, which seemed to be the case in py2.7. However, since py3, this clearly is not the case at all anymore, and one simply shouldn't use __del__ at all.

Code involving the writer now has to be written like so:

writer = repo.config_writer()
writer.set_value('section', 'key', 'value')
writer.release()

If that is not done, bad things can happen, even though I wouldn't know why it can also lead to a wiped configuration file as you describe it.

As noted, you will have to change all code involving a config_writer() to release it after use - you will find this kind of code throughout the GitPython code-base as well.

Please let me know if this makes sense to you.

@yarikoptic
Copy link
Contributor Author

Great -- thanks for the quick get back!

  1. such construct helps. I also got burnt with some "new" behavior of __del__ on python3 -- now will be even more careful.
  2. it still feels that such solution is a workaround which will make others get burnt often -- it seems it needs more explicit/safe behavior which would not require looking through the docs to get a "correct use pattern". What if config_writer, by default, always opens/closes the file for any "set_value" call? then it should work safely for previous code without requiring explicit release(). Then, for batch changes, which can be explicitly and consciously enabled with config_writer(batch=True), it would require explicit .release(). Even better. config_writer should exhibit itself as a context manager, so I could do more pythonic
with repo.config_writer() as writer:
   writer.set_value('section', 'key', 'value')
   writer.set_value('section', 'key2', 'value2')

which would also release automatically whenever I leave the context

@Byron
Copy link
Member

Byron commented Aug 3, 2015

I love the idea stated at 2), and believe this should have been the solution from the get-go. Probably I decided against it back in the days due to the performance implications, as I most certainly didn't think of introducing something like config_writer(batch=True).

Currently I am totally unsure when I will find the state of mind to do this - you might be faster with a respective PR in case it's critical to you.

@yarikoptic
Copy link
Contributor Author

Currently I am totally unsure when I will find the state of mind to do
this - you might be faster with a respective PR in case it's critical to
you.

;-) in my case it was a single use-case and I could easily fix it up
with the explicit .release, so most probably I will not be useful here
with a PR :-/

Cheers!

@Byron Byron modified the milestones: v1.0.2 - Fixes, v1.0.3 - Fixes Feb 13, 2016
@Byron Byron modified the milestones: v2.0.0 - Features and Fixes, v2.0.1 - Bugfixes Apr 24, 2016
@Byron Byron modified the milestones: v2.0.9 - Bugfixes, v2.0.10 - Bugfixes, v2.1.0 - proper windows support, v2.1.0 - better windows support, v2.1.1 - Bugfixes Oct 16, 2016
@Byron Byron modified the milestones: v2.1.1 - Bugfixes, v2.1.2 - Bugfixes Dec 8, 2016
@Byron Byron modified the milestones: v2.1.2 - Bugfixes, v2.1.3 - Bugfixes Mar 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

3 participants