Issues with AutoInterrupt in conjunction with sys.exit on python 3.5

[Heiko-san]

The destructor of the cmd class (actually AutoInterrupt) seems to throw some errors using python 3.5 on Arclinux:

Exception ignored in: <bound method Git.AutoInterrupt.__del__ of <git.cmd.Git.AutoInterrupt object at 0x7f7c2b101d68>>
Traceback (most recent call last):
  File "/usr/lib/python3.5/site-packages/git/cmd.py", line 294, in __del__
TypeError: 'NoneType' object is not callable
Exception ignored in: <bound method Git.AutoInterrupt.__del__ of <git.cmd.Git.AutoInterrupt object at 0x7f7c2b101cc0>>
Traceback (most recent call last):
  File "/usr/lib/python3.5/site-packages/git/cmd.py", line 294, in __del__
TypeError: 'NoneType' object is not callable

It seems this error is triggered by a call to sys.exit(0) at the end of my script, since it doesn't appear if I comment it out.
However I couldn't reproduce this behavior with python 2.7, here sys.exit(0) seems to be working with the module correctly.

What I do is simply clone a repo, add some files commit and push them to a remote.

...
repo = Repo.clone_from(target_git_path, target_temp_dir)
...
repo.index.add([file])
...
repo.index.commit('Initial commit')
...
refspec='refs/heads/{0}:refs/heads/{0}'.format(repo.active_branch) # this will fix error if user has set "push.default = matching"
repo.remotes.origin.push(refspec=refspec)

[barry-scott]

I recall from reading another isssue that the problem is that the process object is not being deleted
quickly under python3. This is because of the change in garbage collection between py2 and py3.

Looking at the code it looks like the fix is to explicitly del the process object once it is no longer needed.

This bug I suspect may happen on py2 depending on garbage collector config.

for example from cmd.py:965:

def __get_object_header(self, cmd, ref):
    cmd.stdin.write(self._prepare_ref(ref))
    cmd.stdin.flush()
    return self._parse_object_header(cmd.stdout.readline())

should be something along these lines:

def __get_object_header(self, cmd, ref):
    cmd.stdin.write(self._prepare_ref(ref))
    cmd.stdin.flush()
    result = self._parse_object_header(cmd.stdout.readline())
    del cmd
    return cmd

[Byron]

Which version of GitPython are you using ? It doesn't appear to be the latest one.
The line in question has recently be fixed to check for all the eventualities.
Thanks for the clarification.

[Byron]

Closed due to inactivity. Please comment to get the issue reopened.

[barry-scott]

Byron,

Unless you fixed the code this bug is still present in the code base.

Barry

[Byron]

@barry-scott Can you please answer the question in my last comment, or maybe even provide a test that triggers the issue to allow it to be fixed ? Otherwise this one would be open forever, and I try to avoid that. Thank you.

[Byron]

And I just noticed that you have not been the OP, yet it seemed you have information about the issue that might help resolving it.

[barry-scott]

What I recall is that I noticed is the with python2 as soon as the object owning a process resource goes out of scope its is deleted. But in python3 the delete is delayed because of changes in garbage collection.
To force the clean up all objects that control a process resource must be delete explicitly using del proc

For example line 635 in cmd..py returns an instance of AutoInterrupt. All caller will have to be changed to del the returned object after use.

Unless this code change has been made this bug remains unfixed.

[Byron]

Thanks @barry-scott, I understand. It seems a context manager for the Cmd object would be an excellent addition to its API.

[barry-scott]

I'm pondering if the cmd code should be ripped out and replaced en-mass.
Given the set of requirements that the cmd code has the current implementation is looking far to complex and hard to modify.

We now know that it needs to:

• run a git command
• allow all stdout to be processed in real time
• allow all strerr to be processed in real time
• allow prompts for input to be processed (for credentials)
• use debug logging to verify operation
• support Windows/macOS/Unix

I think that the above features for cmd can be implemented in a smaller and more obvious way
given all that has been learned with the current implementation.

(I wish you would setup a mailing list for GItPython to allow discussions)

[Byron]

@barry-scott I believe you are right - the code has been growing just to accommodate all the needs as they arose. This will eventually erode clarity unless some major refactoring or even a rewrite is done.

A good starting point would be to clearly separate the orthogonal parts. Something that comes to mind immediately is the part that streams objects through the git process (a hash goes in, an object comes out, the process is held alive permanently). Another obvious step could be to remove all reliance on the destructors and use context-managers or something like an idempotent release() method instead.

On the bright side, one can stake small steps in this endeavour, knowing that the massive reliance on the implementation will show any issues right away when running the tests, some of which might have to be adjusted at some point.

In any case, I find it most important to at most deprecate old parts of the API, so that existing clients have time to adjust. Ideally, they won't even have to, as many will just use it as a simple call wrapper.

When reading through the list of bullet points, one seems to be new in particular: allow prompts for input to be processed (for credentials). I wonder whether this actually has (or should ever) work through git itself. AFAIK, there is the GIT_ASKPASS environment variable just for that purpose.

This project had a mailing list before, and I closed it just because there was nothing happening. By now I do prefer using issues on github for all public communication. If you wish to go ahead with certain refactorings or improvements, you could open an issue for them and discuss the details there. Even though the process is not well defined, we will figure something out as we go, there isn't too many people active on this project after all. I hope that will work for you, as your contributions are greatly appreciated.

[barry-scott]

On windows git credentials wincred prompts using the terminal stdin/stdout.
That has to be parsed out of the stream as in my case the console window if hidden.

I am working on a prototype of the process control and filtering the work is in:
https://github.com/barry-scott/scm-workbench/blob/master/Source/Git/Experiments/git_cmd.py

Its currently code is python3 only and the windows version is not sane.
Turns out I cannot get away with using half use subprocess Popen.
I will need to do the full CreateProcess Pattern, which will fix the python2
support I think.

The macos/linux version is showing promise.

python3 git_cmd.py git status

Barry

On 3 Aug 2016, at 04:27, Sebastian Thiel notifications@github.com wrote:

@barry-scott I believe you are right - the code has been growing just to accommodate all the needs as they arose. This will eventually erode clarity unless some major refactoring or even a rewrite is done.

A good starting point would be to clearly separate the orthogonal parts. Something that comes to mind immediately is the part that streams objects through the git process (a hash goes in, an object comes out, the process is held alive permanently). Another obvious step could be to remove all reliance on the destructors and use context-managers or something like an idempotent release() method instead.

On the bright side, one can stake small steps in this endeavour, knowing that the massive reliance on the implementation will show any issues right away when running the tests, some of which might have to be adjusted at some point.

In any case, I find it most important to at most deprecate old parts of the API, so that existing clients have time to adjust. Ideally, they won't even have to, as many will just use it as a simple call wrapper.

When reading through the list of bullet points, one seems to be new in particular: allow prompts for input to be processed (for credentials). I wonder whether this actually has (or should ever) work through git itself. AFAIK, there is the GIT_ASKPASS environment variable just for that purpose.

This project had a mailing list before, and I closed it just because there was nothing happening. By now I do prefer using issues on github for all public communication. If you wish to go ahead with certain refactorings or improvements, you could open an issue for them and discuss the details there. Even though the process is not well defined, we will figure something out as we go, there isn't too many people active on this project after all. I hope that will work for you, as your contributions are greatly appreciated.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

[Byron]

Thanks for the heads-up. Looking at the code, I didn't see the usage of GIT_ASKPASS anywhere. It's just that I think it's the solution to the 'need to ask for credentials' problem, at least it is what the linked manual suggests.

[barry-scott]

GIT_ASKPASS is half a solution. I could write a stub that using a named pipe to ask the main GUI for the answers. But that means the GUI must implement the credential storage. I guess I can figure out what git-credentials-wincred.exe does and fake being it.

[Byron]

I strongly believe GIT_ASKPASS works like SSH_ASKPASS, which really is nothing but a way to enter the password. It will still be managed by the respective credentials system, so there is nothing to be duplicated here.

[barry-scott]

Yes you are right the credentials are stored after askpass returns them.
It is unfortunent that you get called twice once for username thne for password.
But nothing that a little state machine cannot solve.

I should have this askpass stuff integrated into scm woekbench soon.

Barry

On 5 Aug 2016, at 07:02, Sebastian Thiel notifications@github.com wrote:

I strongly believe GIT_ASKPASS works like SSH_ASKPASS, which really is nothing but a way to enter the password. It will still be managed by the respective credentials system, so there is nothing to be duplicated here.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

[ankostis]

@barry-scott you may take a fresh look at code after some major Windows issues have been resolved in #519 and remaining ones are marked for #525.

[barry-scott]

@ankostis thanks for the heads up. I will test against trunk and check for regressions.

[Byron]

I am closing this issue as it had no interaction for more than 3 months. Please feel free to comment in case you need it to be reopened.