Can we be a bit more precise here? As written, this says "if anything goes wrong at all while running the git subprocess, assume we're not inside a git repo"; if the git subprocess exits without error, but doesn't produce any output, also assume we're not inside a git repo. I think this is too crude - at the very least, I would distinguish CalledProcessError from other exceptions, and make the "if subprocess output is empty" check more explicit than casting to bool.
The messaging is a bit awkward here, because our "are we in a git repo" tests don't distinguish between "this actually is not a git repo", "you don't have git installed", and other failures (such as "you don't have read permissions on the CWD" etc.).
consider reordering the logic here into something like:
if isGitRepo: reason = 'the previous...' else: reason = 'this is not a git repo...'
Easier to follow than overwriting reason in place.
I already wrote this in the ticket 5 days ago, but I have a problem with this idea. It doesn't make sense to me that we're skipping an entire set of tests because we're not inside a git repo. After all the tests have nothing to do with git (it's not like we're testing something about git or the GHC git repo). It seems to me that this is implementing hack to avoid a problem that should be actually fixed somehwere else (namely, the problem of trying to add perf test results in git notes).
Why is it so hard to avoid adding git notes when not inside a git repo instead?
I'll request changes because of the idea (haven't even looked at the code yet).
@osa1 We use git notes to get the baseline performance numbers and to save the new numbers. Here we only skip performance tests if we both cannot get a baseline (and hence can't tell if tests pass or fail) nor have a way to save the results.
I didn't manage to find a git command that would reliably say if the current directory is a git repo. You'd expect "git rev-parse --is-inside-work-tree" to print false if not in a work tree, but it actually exits with a non-zero exit code. Perhaps this is cleaner now: check that "git status" returns a 0 exit code, rename to can_git_status() and adjust warning messages/variable names to be a little more specific to what went wrong.