gitCheckout() validates branch, plus unit tests

[pda]

When constructing the git checkout command, the sometimes-user-specified branch name should never be considered flags/options by git. It's not possible to terminate arg parsing with -- because that has special meaning to git checkout — it indicates the following names are files, not refs. So, the ref must be validated. There's a git check-ref-format subcommand which roughly does that. Rather than shelling out to git, this patch introduces a simple gitCheckRefFormat(ref) implementation to verify the ref doesn't start with a hyphen. This could be replaced with shell-out or a more thorough implementation if that feels worth doing.

With branch set to --foo…

• Before: git checkout -f --foo → error: unknown option 'foo'.
• After: git checkout -f -- --foo → error: pathspec '--foo' did not match any file(s) known to git

Related: #1314

---

This pull request also introduces unit tests (with a mockShellRunner) for gitCheckout(), gitClone(), gitClean(), gitCleanSubmodules() and gitFetch().

$ go test ./bootstrap -run TestGit -v
=== RUN   TestGitCheckRefFormat
=== RUN   TestGitCheckRefFormat/hello
=== RUN   TestGitCheckRefFormat/--world
--- PASS: TestGitCheckRefFormat (0.00s)
    --- PASS: TestGitCheckRefFormat/hello (0.00s)
    --- PASS: TestGitCheckRefFormat/--world (0.00s)
=== RUN   TestGitCheckoutValidatesRef
--- PASS: TestGitCheckoutValidatesRef (0.00s)
=== RUN   TestGitCheckoutSomething
--- PASS: TestGitCheckoutSomething (0.00s)
=== RUN   TestGitClone
--- PASS: TestGitClone (0.00s)
=== RUN   TestGitClean
--- PASS: TestGitClean (0.00s)
=== RUN   TestGitCleanSubmodules
--- PASS: TestGitCleanSubmodules (0.00s)
=== RUN   TestGitFetch
--- PASS: TestGitFetch (0.00s)
PASS

[pda]

This should be the only functional change, the rest is testability refactoring.

[sj26]

Seems legit 👍

[sj26]

This seems like a convoluted way to do strings.HasPrefix(ref, "-")? Or ref[0] == "-"?

[pda]

Yeah… I went through various implementations as I got lured by unit testing into implementing more and more of the spec. I was hoping to implement the whole function as a single pass over ref, rather than lots of strings.Whatever calls that each process the whole string. Especially as I was thinking there would be various disallowed prefix/suffix characters… turns out at this point there's only one of each. I should probably simplify it back down.

[pda]

I should probably just rewrite the whole thing as one regexp.

[pda]

b3cb5f9

[sj26]

Nice