Avoid brittle assumptions about preexisting temporary files in tests

[EliahKagan]

May fix #1744

Two tests had previously used tempfile.gettempdir() directly to get a directory to make assertions about, such as not being a valid git repository. One other test had created a repository at a hard-coded location relative to it, which in effect also assumed the availability of that path, a condition that may not hold, due either to GitPython's own previous test runs, unrelated use of those paths in a location like /tmp, or possibly other conditions. This uses newly created directories instead whose contents are known.

The tests this makes more robust include test_new_should_raise_on_invalid_repo_location, which is the one reported as failing on OpenIndiana in the build described in #1744. I believe this fully remedies that problem, but I do not believe it with high confidence. I had some trouble getting oi-userland set up in a way that would enable me to efficiently test this, and I would not be sure I had reproduced the precise conditions of the actual build.

I believe these improvements are worthwhile even separately from that, since the assumptions about tempfile.gettempdir() and its preexisting contents are minor test bugs in and of themselves. So I'm opening this PR now, but I want to make clear that while I believe this addresses #1744, I cannot be certain.

In part to make the tests more comprehensive, and in part because I think it may be relevant to the situation there, this also adds a related test, test_new_should_raise_on_invalid_repo_location_within_repo. This verifies that calling Repo on a directory that is not a git repository and that is known to reside inside a directory that is a git repository gives the same error as we expect when it is not (known to be) inside a repository.

A few more details are in commit messages, including on one of the tests that, while being made no longer to use a hard-coded path under tempfile.gettempdir(), now does its cleanup a little differently and cleanup now succeeds on it. So this turns out to be a small step toward #790.

[Byron]

Thanks so much for these improvements, and even for trying to reproduce the issue on OpenIndiana!

Having reacquainted myself to the respective issue, I noticed that to this time it's unclear why that happens in the first place. After all, we could determine that GIT_DIR isn't overriding the provided path, nor does it try to break out of the path (by default).

The new test also validates that this is the case, so it's still puzzling what's truly happening there.

CC @mtelka .

[Byron]

PS: It's interesting that the author and committer name has changed to Travis Runner, which makes me wonder if this is related to Travis CI in any way. I assume it's intentional, too, but it the similarity puzzled me.

[EliahKagan]

PS: It's interesting that the author and committer name has changed to Travis Runner, which makes me wonder if this is related to Travis CI in any way.

Sorry, where is this? (I possibly should know what that is a screenshot of, but I do not.)

Tests on CI set user.name to Travis Runner and user.email to travis@ci.com, but this has been the case for a while and I am not aware of a way that this PR should change anything related to that.

I assume it's intentional, too, but it the similarity puzzled me.

I may be misunderstanding you, but I have not intended any change related to commit authors in tests or otherwise, in this PR.

[Byron]

Apologies for the small screenshot, I realize how it's unclear about that it is really showing.

The new screenshot probably remedies that:

Somehow these commits don't have your name anymore, just as if something changed your global configuration.

[EliahKagan]

Thanks for pointing that out. My global user.name was indeed changed, on my Windows system, from Eliah Kagan to Travis Runner. I had not noticed that, and I have fairly little idea how it happened! It was very much not intentional.

But it doesn't seem to be related to this issue in any way, because the change goes all the way back to 39e2dff. Every once in a while I paste something into my terminal and I'm so sure it's what I want that I click yes on the message box that comes up to warn me that it is multiple lines. I recall pasting some YAML from a workflow by accident at some point. Probably it ran something from there. I'm not really sure about this, though, since I can't find it in my shell's history. But if that's what happened, then I wonder what else also got changed... 😦

[EliahKagan]

Update: I'm going to go with the accidental pasting explanation, even though it is incomplete, because it makes a lot of sense given the YAML code:

      run: |
        git config --global user.email "travis@ci.com"
        git config --global user.name "Travis Runner"

In a POSIX-compatible (Bourne style) shell, and I think most Unix-style shells, the | after run: would not suppress the command on the next line: it would be on the right side of a pipe, but it would never read from stdin (because git config is noninteractive) and it would perform its side effect. But in PowerShell that's not the case: if the command on the left side of | is not found, the right-side command does not run. Then the command to set user.email does not run, but the subsequent command to set user.name does.

Therefore, I do not believe this is a regression or other issue in GitPython. In investigating #1744 (if that turns out not to be fixed now), this Travis Runner business can probably be disregarded.

[Byron]

I am glad I brought this to your attention then, and will be looking forward to have Eliah Kagan back :).