Tests don't readily pass in forks

[EliahKagan]

Getting tests to pass in forks requires steps that are not documented, and that might preferably not be required. This is due to the tests' dependence on the presence of particular version tags, but I believe it can be fixed without (or before) changing how the tests work.

By default, a fork is created with only the default branch--here, main--of the upstream repository, and none of its tags. The GitHub interface for forking has a checkbox that, if unchecked, copies all branches and tags, but it is checked by default. It is also possible to add the upstream when cloning, or afterwards, or even to fetch the tags from a remote that is not configured, by passing a URL to git fetch.

Two pull requests that were mainly about other things also attempted to improve this, but made limited impact on it. The recent addition of git fetch --tags to the instructions in #1647 (fafb4f6), extended in #1654 (72e48aa), only address narrow cases where...

• the fork gains the tags between being cloned and the tests being run, or
• the upstream remote was added but never yet fetched from (this happens in GitHub Codespaces).

#1654 (72e48aa) also adds instructions for cloning a fork with gh, which adds the upstream automatically and fetches from it, getting tags even if the fork doesn't have them. But this does not address CI, which seems to me the bigger issue. The readme currently says:

The same linting and testing will also be performed against different supported python versions upon submitting a pull request (or on each push if you have a fork with a "main" branch and actions enabled).

That is true, but without further steps, none of the CI test jobs will pass in most forks.

Possible solutions

Expand the documentation (flawed)

Although people who are working on the code in a fork should typically add their fork's upstream remote, either by cloning with gh or by running git remote add ... (where ... is the upstream remote, which for most but not all forks is this original repository), doing that locally doesn't fix CI.

The documentation could be made more detailed, to show the use of git remote add ..., to advise unchecking the only-main box when creating a fork, and to explain how to fetch tags from upstream and push them to a fork. But I think doing that, or at least relying on it, had a few disadvantages:

• The burden to new contributors getting up and running to work on the project would be greater.
• If CI tests still fail in a fork, it will be unclear to users if this is a GitPython bug or a mistake they made.
• The point of init-tests-after-clone.sh is so people don't have to follow these kinds of special steps.

Have the init script try to add an upstream remote (flawed)

init-tests-after-clone.sh or the CI test workflows could be extended to add an upstream remote under some conditions, but I recommend against that, because in forks of forks, it is unclear which remote should be added or whether this operation can be done safely. (The GitHub API could be used to discover the actual parent or other information about the fork network, so I think feasibility is not the main problem.) Relatedly, using gh may not be sufficient in a fork of a fork, if the immediate upstream does not have the version tags.

To elaborate on the safety issue: It would be intuitive to think that adding a repository's immediate upstream fork, even if the user is not fully aware it is being done, would be a safe operation. However, it actually has security implications, even under the assumption that the user both trusts this original repository and trusted the (possibly different) fork-network member when they made the fork. This is due to a combination of two factors:

• The upstream may have become untrusted, due to some subtleties of fork networks. Because it is nontrivial to deliberately detach or reparent a fork (I believe this requires action by a GitHub employee, or deleting and recreating the fork), the owner of a fork of a fork may initially trust the immediate upstream, stop trusting it, but continue to retain it. Also, if a fork's immediate upstream is deleted, the fork is reparented automatically, and the new upstream, which also might not be this original repository, may be untrusted. (Because the risk is mainly when using a fork of a fork, and someone who uses such a fork is likely aware of whether they trust its current immediate upstream repository, I think it is still okay to recommend cloning with gh. Unlike whatever goes in init-tests-after-clone.sh, users can easily decide not to use gh.)
• init-tests-after-clone.sh is unsafe if some remotes are untrusted. A user may clone the repo, tell their editor to trust the folder--or have cloned it somewhere the editor is configured to trust subfolders of--and then run the script. If the script adds an untrusted remote, then git checkout master can create a branch from that remote, and the editor may execute code from the untrusted remote branch. For example, if the remote branch has .vscode/settings.json with a unit testing configuration, VS Code would load modules to perform test discovery, executing their top-level code. Adding the remote at the end of the script (after checkout commands) would partly mitigate this, but it is common to delete __testing_point__ and rerun the script. A stronger mitigation could be to change the checkout logic to avoid this, though some alternatives carry their own risks. Ultimately, even if init-tests-after-clone.sh were known to be safe in all reasonable use cases, I think it is best to avoid the automatic and implicit addition of potentially untrusted remotes to an existing local repository.

Have the init script add specific version tags itself (flawed)

init-tests-after-clone.sh could run git tag commands to add version tags that are missing. Version tag names could be obtained in a few ways:

• Hard-coding a handful of names known to be needed by the tests.
• Examining the test code to extract the names.
• Running git ls-remote on a hard-coded URL for this original repository, and filtering the names for just version tags.

As I see it, the problem is the same with all those ways: The upstream tags may be (and, it turns out, are) annotated tags. But the script would be creating lightweight tags, or, much worse, annotated tags not equivalent to the originals.

I think this could be confusing even if a repository with real original annotated tags is never later added as a remote, because it would feel like whatever same-named version tags are present downstream should be equivalent to the upstream tags, and people might assume that. But since users of a fork should add the upstream remote in most cases, users who have not done this may later do so, and then the situation would become even more confusing.

Have the init script fetch original tags without adding a remote (less flawed?)

In view of the downsides of the above approaches but also of the current situation, I think init-tests-after-clone.sh should, as a backup strategy when nothing that looks like a version tag is present, fetch tags whose names start with a digit from a hard-coded gitpython-developers/GitPython repository URL, passing the URL to git fetch and not adding a remote.

This should only be a backup strategy, and because it is unusual--especially because it might mislead a user into thinking their fork or some other remote listed in git remote -v has version tags--a warning should be issued when it is done. If version tags are detected after fetching tags from all remotes that are configured, then it should not be attempted.

Furthermore, it would be undesirable to fetch version tags into a clone of a fork that publishes its own packages or otherwise has its own version tags, because it could cause confusion, and because the GitPython version tags might end up getting pushed to the fork's own remote by accident. Therefore, anything that looks like a version tag should, if present, prevent this backup strategy from being tried. GitPython only uses version tags that start with a digit, and only such tags should be fetched from the hard-coded remote. But forks that publish their own releases might use the other common convention where version tags begin with v followed by a digit, so the presence of any tag like that should also prevent this from being done.

Although this is not perfect, I plan to open a PR for this very soon. It seems to me that it is reasonable to do this, to improve the situation until...

Don't use own repo, or at least not its tags, in tests (ideal future solution)

This is, of course, ultimately the solution to the whole category of limitations of which this issue is a part (#914). If this were done in full, then the init-tests-after-clone.sh script could go away altogether.