stop test runner if working tree becomes dirty

[hoffbrinkle]

A command may leave the working tree dirty (e.g. the command is applying
code formatters to the tree). In such a case, we should stop running
tests and allow the user to deal with it at the change that caused the
code to change.

[mhagger]

This is a great idea? 👈 typo, sorry!

This is a great idea! 👈 corrected

Wouldn't we want some kind of custom error message explaining that the reason for the failure was that the test modified the working tree, as opposed to the test returning an error result? It seems like it could be confusing otherwise. Or does git's output already make it quite clear why the commit was considered a failure?

What would happen in this case if the --keep-going option is in use? In that situation, git-test would still want to check out the subsequent commit and test it. Will that attempted checkout result in a second round of error messages, possibly obscuring the first one? (Presumably in this situation, git-test should not keep going, at least not without giving the user the chance to clean up the working directory first.)

Also, I'm not sure that the commit should necessarily be considered to have "failed" in this case. Sure, if one of the "tests" is to run a code formatter over the code, then it seems like a modified source tree should be considered a failure. OTOH if you want a test like that, the test itself should ideally include its own check that the source tree was not modified.

But there are other situations where the source tree can appear "dirty". For example, suppose that a build target is removed, and at the same time a corresponding .gitignore entry is removed. That could make the tree look dirty because of a copy of the build target left over from testing the previous commit. Marking that revision broken would seem incorrect. Though what should git-test do in that situation? It's not clear. One could imagine giving the user the chance to decide how to proceed (and a chance to manually clean up the working tree). But that would require some kind of new UI, which would be a lot more complex than this PR. Maybe it should just stop without marking the commit good or bad.

Summary: I love the idea of having a clear error for the "dirty tree" situation. I think this PR is already an improvement on the status quo, though I'm wondering whether it could give the user better information/guidance to help them figure out how to proceed.

[hoffbrinkle]

Thanks for the feedback! Obviously, this was a quick change that solved my immediate need. However, I agree that it could certainly be made more robust and feature complete.

OTOH if you want a test like that, the test itself should ideally include its own check that the source tree was not modified.

While I could incorporate the workspace checking in to the command, the same could be said for many features. Seems like anything that is integrating with git is probably best done by this infrastructure. I think it'd be a common enough use case that there'd be value add in handling this with this test command.

I'm going to be busy for at least the next week, possibly three weeks, but I think we may as well come up with at least come up with a strategy for many of the cases you outline above, and then I can go back and improve this PR. I may just create an separate issue for it and do it there. However, it's going to be a week or three before I can devote some thought to it.