René's URL Explorer Experiment

{"@context":"https://schema.org","@type":"DiscussionForumPosting","headline":"Tests don't readily pass in forks","articleBody":"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.\r\n\r\nBy 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 *un*checked, 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`.\r\n\r\nTwo 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...\r\n\r\n- the fork gains the tags between being cloned and the tests being run, *or*\r\n- the upstream remote was added but never yet fetched from (this happens in GitHub Codespaces).\r\n\r\n#1654 (72e48aa) also adds instructions for cloning a fork with [`gh`](https://cli.github.com/), 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](https://github.com/gitpython-developers/GitPython/blob/7d4f6c68c657586f27125f25b15f02c5a3d7f35c/README.md?plain=1#L169-L170) says:\r\n\r\n\u003e 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).\r\n\r\nThat is true, but without further steps, none of the CI test jobs will *pass* in most forks.\r\n\r\n## Possible solutions\r\n\r\n### Expand the documentation (flawed)\r\n\r\nAlthough 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](https://github.com/gitpython-developers/GitPython/network/members) forks is this original repository), doing that locally doesn't fix CI.\r\n\r\nThe 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:\r\n\r\n- The burden to new contributors getting up and running to work on the project would be greater.\r\n- 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.\r\n- The point of `init-tests-after-clone.sh` is so people don't have to follow these kinds of special steps.\r\n\r\n### Have the init script try to add an upstream remote (flawed)\r\n\r\n`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.\r\n\r\nTo 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:\r\n\r\n- **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`.)\r\n- **`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.\r\n\r\n### Have the init script add specific version tags itself (flawed)\r\n\r\n`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:\r\n\r\n- Hard-coding a handful of names known to be needed by the tests.\r\n- Examining the test code to extract the names.\r\n- Running `git ls-remote` on a hard-coded URL for this original repository, and filtering the names for just version tags.\r\n\r\nAs 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.\r\n\r\nI 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.\r\n\r\n### Have the init script fetch original tags without adding a remote (less flawed?)\r\n\r\nIn 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.\r\n\r\nThis 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.\r\n\r\nFurthermore, 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.\r\n\r\nAlthough 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...\r\n\r\n### Don't use own repo, or at least not its tags, in tests (ideal future solution)\r\n\r\nThis 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.","author":{"url":"https://github.com/EliahKagan","@type":"Person","name":"EliahKagan"},"datePublished":"2023-10-04T02:04:49.000Z","interactionStatistic":{"@type":"InteractionCounter","interactionType":"https://schema.org/CommentAction","userInteractionCount":0},"url":"https://github.com/1690/GitPython/issues/1690"}