René's URL Explorer Experiment

Title: Improve scripts and tool configurations by EliahKagan · Pull Request #1693 · gitpython-developers/GitPython · GitHub

Open Graph Title: Improve scripts and tool configurations by EliahKagan · Pull Request #1693 · gitpython-developers/GitPython

X Title: Improve scripts and tool configurations by EliahKagan · Pull Request #1693 · gitpython-developers/GitPython

Description: Fixes #1690 Fixes #1691 Fixes #1692 This combines the solutions I recommended in those three issues on a single branch. Although I feel this does compose into a coherent whole, with each of those issues' practical ramifications having some tendrils into each other, and opening separate PRs would have resulted in nontrivial merge conflicts... nonetheless this PR is broader than I would usually like. But I have done it this way because it was by working on them together that I was able to get clear on the distinct ideas that I presented as those issues, as well as whether the solutions would work reasonably when all combined. This PR is what remains after pieces that seemed reasonable to sever into their on PRs, such as #1684, have been removed, and excessively enterprising ideas, such as writing a batch-file version of init-tests-after-clone.sh (discussed in #1691) or switching lint.yml to use pre-commit.ci, abandoned or deferred. However, I would be pleased to make further changes as requested. I think the changes here are feasible to review together, but I am willing to rework this into multiple more narrowly scoped PRs if necessary. The problems and their solutions in general are presented in the three linked issues that this would close. (Details are given in commit messages.) Specific considerations that I suspect may be important when reviewing this PR follow. Shell script portability I believe the shell scripts that pertain to building do not need to be any more portable than they are now: they run on bash version 3 or later, which most systems have or can get. I did make modifications to them, some of which were inspired and emboldened by shellcheck, but not for portability to other shells. I take #1661 (comment) (specifically: d18d90a, 1e0b3f9) as an indication of a general preference of echo over printf. The bash shell provides an echo builtin that has good design decisions and, more importantly, behaves the same in bash on any system, because it is a builtin. (I have actually slightly increased the use of echo in bash in this PR.) However, for POSIX shell scripts that assume only what the standard insists on (or that call echo through another command, such as with find -exec or xargs, thereby using an external echo executable), the situation is considerably messier. To write a portable script, I have avoided the use of echo in the init script that, for portability, I have changed from bash to sh. What fallback version-tag fetching looks like Fallback fetching of version tags is the "back-up" strategy that is part of the fix for #1690. It works both locally and on CI. Here's what it looks like on CI (I temporarily deleted all the version tags from my fork and reran the tip of this PR's branch): Ubuntu Cygwin (In case for any reason you want to see the version from an earlier commit and prior to multiple rebases that I had shown before in an earlier draft of this PR description, that's here.) Fallback version-tag fetching would fail on old git At least as currently written, it uses a feature that was introduced in Git 2.6.0. I have posted a review comment on the specific code this affects, with full details. Security implications of where init-tests-after-clone.sh makes master Edit: Based on #1615, I think you might prefer git checkout -B master not be used, for reasons of stability rather than security. Maybe git checkout -B master ea43defd777a9c0751fc44a9c6a622fc2dbd18a0 should even be used, to get the same old commit even in forks that don't have the master branch, and to add the same commits to the top of the reflog history even when __testing_point__ is deleted and the script rerun. Currently, on main, init-tests-after-clone.sh creates or switches to a master branch like this: git checkout master || git checkout -b master One of the changes in this PR is to ensure that master cannot be interpreted as a path--it has to be taken to be a branch, or at least a ref--which is mainly for better output to stderr when the branch doesn't exist, but also to safeguard against rare situations where a file or directory named master exists: git checkout master -- || git checkout -b master However, there is a simpler, more elegant, and more robust way to get a suitable master branch for tests, that I think is more likely to work well most of the time [edit: though the answer to #1615 suggests a reason it might not]: git checkout -B master master is already getting reset back to __testing_point__, so the main disadvantage of -B, that one can lose one's branch if it is not pushed, applies already. I would like to do it that way (and I have a branch for it, ready to go). But it has security implications, specifically for people who review pull requests. As detailed in #1690, wherever master gets checked out, editors and IDEs may be fooled into running unreviewed untrusted code from there. Using the -B way I want to use would be a mitigation for the situation described in #1690 (though that's not the main reason I want to do it). But it would be an exacerbation of it for a reviewer, because it would weaken the already brittle assurance git diff main feature would provide. Consider: main ← evil ← evil ← evil ← feature There, a pull request proposes a useful feature on the feature branch. But the preceding three commits have evil code in them that, if an editor/IDE imports modules to do test discovery (or any other operation the editor reserves for "trusted folders"), will be run. The tip of feature removes the evil code, so git diff main feature does not reveal it. But because master is checked out at the tip of feature even if master already exists in the reviewer's local or remote repository, the script's reflog-building commands reset to each of the evil commits. This may not be a serious problem. When reviewing, one should already not rely merely on git diff main feature, and CI in pull requests is set up to run on the pull-request trigger (which, unlike pull-request-target which is dangerous if not used very carefully, runs with the fork's permissions, not allowing elevation). Furthermore, one may already not have master locally or on a remote, in which case the existing checkout code already creates it at HEAD, so it's not like this is a new situation. Furthermore, it's generally unnecessary to run that script while reviewing, even if one opens up an only partially reviewed PR branch in an editor that may perform unsafe operations based on its contents. Nonetheless, I wanted to bring this up rather than just adding a commit to change it to git checkout -B master. black via pre-commit: I'm not sure what's best. It seems to me that there is actually just one thing that, in hindsight, would have been better to have developed separately. Adding black to pre-commit, and using that on CI, presents choices to be made while reviewing that are practically separate from the other changes. The core issue is that all the other tools run via pre-commit only perform checks, while the way most users of black and pre-commit would expect and prefer they be used together is for pre-commit to actually perform auto-formatting. This is actually no problem on CI; the GitHub Action being used accepts code changes from a hook as a check failure. But there should be a way to just lint locally, that includes checking for black-conforming formatting and that does not change any files. Therefore, I have set up two hooks for black: one that runs by default and formats, and another that does not run by default and that only performs checks. The tox linting environment and lint.yml CI workflow use the check-only hook and skip the formatting one. But there should also be a way to do this locally without building the project and setting up tox environments and without typing in a complex pre-commit command. So I have made make lint do that. This is all documented as part of the readme improvements. It is inelegant, though, because everything else done by makefiles in this project is directly related to building. There are a few options, which I present in descending order of my preference, but they are all things I think are reasonable: Use this, for now. Use this, for now, but add a note in the readme about how make lint may go away. (I think this is not really necessary, because the development instructions are not implied to be stable. But I am not sure.) Change pre-commit to have only the check-only black hook, at least for now, and modify the readme accordingly. Then there is no need for make lint because pre-commit run --all-files just lints, as it did before, but including black. This has the disadvantage that people who like black and pre-commit probably prefer that they facilitate auto-formatting, but it is otherwise elegant. I suspect you might prefer this approach, therefore I have made a commit for it on my sh-nofmt branch, which can be easily fast-forward merged into here (or opened as a separate PR if the change is desired after merging this). Remove everything to do with black from .pre-commit-config.yml on this branch, modifying the readme accordingly, and propose it separately. It could be removed by rebasing, or just by adding a commit to remove it. This is more work, but actually a bigger reason this is not my preferred approach is the same reason I had added black to pre-commit in the first place: with everything else being done by pre-commit, and with this project being in black style (aside from this project's very long lines), it is unintuitive and unexpected that it would not, in any form, be usable via pre-commit.

Open Graph Description: Fixes #1690 Fixes #1691 Fixes #1692 This combines the solutions I recommended in those three issues on a single branch. Although I feel this does compose into a coherent whole, with each of those i...

X Description: Fixes #1690 Fixes #1691 Fixes #1692 This combines the solutions I recommended in those three issues on a single branch. Although I feel this does compose into a coherent whole, with each of those i...

Opengraph URL: https://github.com/gitpython-developers/GitPython/pull/1693

X: @github

direct link

Domain: github.com

Links:

Viewport: width=device-width