Replace xfail with gc.collect in TestSubmodule.test_rename

[EliahKagan]

In various places in the code, a conditional gc.collect() is used to allow a test to finish its work (and pass). This adds one more such place. It accordingly removes an xfail marking. TestSubmodule.test_rename now passes on all combinations of platform and Python version.

Like the xfail condition, running gc.collect() here is conditional, being done only in the specific situation the PermissionError occurs. Besides that it does not always run even on Windows (only in 3.12 and later), this resembles various other conditional and non-conditional gc.collect calls.

The situation is a bit odd, but I think that's--albeit by a fairly slight margin--more of a reason to convert the xfail into a gc.collect() than to refrain from doing so.

Context

#1745 (under "Which versions should we test?"), and also 82c361e, 0b7ee17, and various other less interesting commits, provide further context about this test and the PermissionError on Windows that does not occur prior to 3.12.

Because PermissionError error message is about another process having the file open, #790 seems relevant. But this does not happen in a call to git.util.rmtree.

One call is enough, now

It had previously appeared to me that two calls to gc.collect were required, but I am unable to reproduce that. It may have been specific to how I was running it on my system at that time. The need for only one call may have been brought about by changes to the code in the mean time, but I have tested that only one call appears required even without the changes in #1765. (Perhaps #1753 contributed? I think most likely not, though.)

[Byron]

That's a great catch! Who would have thought that gc.collect() is also applicable here.

This probably also hints at the need to eventually fix the way GitPython handles systems resources. After all, it really seems like one would rather want to use…

with git.open(path) as repo:
   pass

…to control the lifetime of all opened resources precisely. Alternatively, GitPython would not be allowed to keep anything open past the lifetime of a method call, but that's not feasible at all. Maybe there is alternatives that I am not seeing.

It's interesting how it's obvious that GitPython was designed in a time when there was no GC, but rather, reference counting with deterministic and immediate calls of destructors.

[EliahKagan]

This sounds like it might be part of making the classes that currently have __del__ methods also be context managers, as discussed in comments in #1669. Though I suppose this usage need not wait for that, with Repo having been a context manager since #555. Should the tests be modified to make greater use of that? I'm not sure it would address the sort of thing that happened here and it's not quite what your code example shows.

It's interesting how it's obvious that GitPython was designed in a time when there was no GC, but rather, reference counting with deterministic and immediate calls of destructors.

I have some thoughts about how the effects of this might be mitigated, to some extent, even for existing code that would not use newly added context managers (or perhaps even before, or separately from, making more classes context managers). But right now these are half-formed ideas that might turn out to be meritless and (as with #1762) there's some other GitPython-related stuff I'd probably want to finish dealing with first. I mention this mainly so I don't forget or, if I do, so that it is more likely to come up again.

[Byron]

This sounds like it might be part of making the classes that currently have __del__ methods also be context managers, as discussed in comments in #1669. Though I suppose this usage need not wait for that, with Repo having been a context manager since #555. Should the tests be modified to make greater use of that? I'm not sure it would address the sort of thing that happened here and it's not quite what your code example shows.

I left some more ideas in this comment, and based on it I'd say the tests should be kept as is. Should a full-on context-manager approach happen, the tests can be a great indicator to show how hard the migration would be for existing users, and also guide towards making it as painless as possible.