Skip to content

git: Fix racy test creating refs in Gitaly repo

Patrick Steinhardt requested to merge pks-fix-test-modifying-gitaly-ref into master

When executing the test suite, sometimes it would happen that the Gitaly repo now has a new reference .git/master. It would be hard to notice if not for the fact that Git now starts complaining about an ambiguous master reference. What's interesting is that this reference contains the tree which is in internal/gitaly/service/wiki.

The root cause for this is a test in internal/git. What it does is to spawn the Git command git update-ref master 0000. This is wrong in two ways:

  1. git-update-ref(1) expects a fully qualified reference, but we're passing in "master" only. This explains why ".git/master" is being created.

  2. The second argument is supposed to be the OID the reference is being set to. "0000" may be mistaken to be the zero OID, but in the case of Gitaly it isn't. In fact, it currently gets resolved to the tree in internal/gitaly/service/wiki, whose OID by coincidence has four leading zeroes.

While this misuse explains how such a reference would be created, it doesn't explain the why. The thing though is that one of the testcases would simply run that command with git.SafeBareCmd, which doesn't take any repo. As a result, it'll just run in the current working directory without any indication in which repo Git should perform its work. Which during test execution is Gitaly's repo.

The spuriousness of the symptom can be explained by the fact that there's a race going on: there's four test cases which each spawn a command without waiting for it to exit. If we're fast enough with spawning those executables, then we'll cancel the context and thus also cancel the commands, without them yet having performed the reference update. If we're too slow, then it would've updated the Gitaly repo.

Fix the testcase by properly using git-update-ref(1) and by waiting for commands to exit before starting the next testcase. As we cannot safely make the git.SafeBareCmd work without modifying the process's state, this commit just drops that testcase.

Merge request reports