Skip to content

Detect Git executable getting resolved via PATH

Patrick Steinhardt requested to merge pks-git-config into master

When configuring Gitaly setups, admins can choose which Git executable they want Gitaly to use for all Git invocations by specifing the config.git_path key. Because of this, Gitaly is never allowed to lookup Git via the PATH environment, but always needs to consult the configured value. While most of our code (except for tests which have been fixed with preceding commits) always honored this, there were two locations which didn't, which wen't silently under the radar waiting for trouble.

This commit introduces an improved test setup which will always catch such bugs: we simply set up a Git executable in a separate directory which writes an error message and immediately aborts with a strange error value and then adjust our PATH variable to have the executable's directory as first item. As a result, whenever Git is being executed with an unqualified path, we'll pick this binary instead of the real Git and produce an error. Tests would quickly catch this, e.g. like the following:

--- FAIL: TestSuccessfulIsAncestorRequestWithAltGitObjectDirs (0.04s)
    commit.go:80: stdout: , stderr: Git executable from $PATH was picked up. Please fix code to use `command.GitPath()` instead.

The real Git binary is injected via the GITALY_TESTING_GIT_BINARY environment variable, which is being picked up by our configuration code.

Most of this MR is just simple conversions of test code to use the correct Git executable. While mostly trivial, it's been quite a painful process to do the conversion due to some quite hidden dependencies, e.g. like the hook scripts which lived at random places containing unqualified references to Git. There were two "real" bugs, though:

  • git-linguist always executed Git via the PATH variable. This is probably less of a problem as it only used it to check if it's running in a Git repository.
  • Reading the index in our Git abstriction also used PATH to resolve Git. This can be a problem if the system-provided Git version is too old to understand certain extensions to the Git index while the configured Git version does understand and use them. This would then result in errors.

Anyway. I think the resulting test setup we have with this MR is quite nice as it'll neatly catch such issues from now on whenever you or CI executes make test.

This is a prerequisite for &3196 (closed) and #3026 (closed)

Edited by Patrick Steinhardt

Merge request reports