Skip to content

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

Open
@yarikoptic

Description

@yarikoptic
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

Activity

added this to the v1.0.2 - Fixes milestone on Jul 31, 2015
Byron

Byron commented on Jul 31, 2015

@Byron
Member

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

yarikoptic commented on Jul 31, 2015

@yarikoptic
ContributorAuthor

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

Byron commented on Aug 3, 2015

@Byron
Member

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

yarikoptic commented on Aug 3, 2015

@yarikoptic
ContributorAuthor

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!

modified the milestones: v1.0.2 - Fixes, v1.0.3 - Fixes on Feb 13, 2016
removed this from the v2.0.0 - Features and Fixes milestone on Apr 24, 2016

35 remaining items

modified the milestones: v2.1.1 - Bugfixes, v2.1.2 - Bugfixes on Dec 8, 2016
modified the milestone: v2.1.2 - Bugfixes on Mar 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      No branches or pull requests

        Participants

        @yarikoptic@Byron@nvie

        Issue actions

          python3 specific: seems to wipe out entire .git/config upon · Issue #333 · gitpython-developers/GitPython