GitoxideLabs / gitoxide

An idiomatic, lean, fast & safe pure Rust implementation of Git
Apache License 2.0
8.91k stars 303 forks source link

Update an archive and add some missing executable bits #1589

Closed EliahKagan closed 1 month ago

EliahKagan commented 1 month ago

One of the fixture scripts was modified in b12c7c9 (#1587) to fix typos, which changed its hash, but a regenerated archive was not committed at that time.

Other fixture scripts were recently introduced, and they have current archives committed, but they were not given executable bits, even though they are executable scripts that start with #! and the other fixture scripts have +x.

This small "maintenance" PR addresses those two things (one per commit).

I have verified that tests pass locally on Ubuntu 24.04 LTS, macOS 14.6, and Windows 10, both before and after this change, and that the make_dangling_symlink_to_windows_reserved archive, which previously had an uncommitted change on Ubuntu and macOS after running the tests, is no longer changed when the test suite is run. (It did not show as dirty on Windows because gix-testtools suppresses archive generation on Windows, but committing the regenerated script should help Windows tests to run slightly faster, too.)

Byron commented 1 month ago

Thanks a lot, much appreciated.

I wondered if there should be a CI job that tests invariants, maybe based on just check-invariants, which simply runs a script to verify a few things. We might already have that in the form of the script that validates that gix-packetline-blocking still matches gix-packetline.

EliahKagan commented 1 month ago

I don't know a good near-instantaneous way to test whether archives need to be regenerated without having just run the tests. This is one of the reasons I implemented #1590 the way I did.

For scripts, though, we can certainly check all non-ignored files to make sure that files that seem to be scripts (or, I think better, that seem to be plain text) start with #! if and only if they are marked executable. Conceptually, I want to say this is sort of like checking the relationship between gix-packetline and gix-packetline-blocking, except that all of the design considerations point in the other direction:

Byron commented 1 month ago

I like the idea of implementing such functionality in Rust instead.

However, I'd hope to bring more of such 'internal' functionality into the internal-tools crate, which probably isn't cheap to build but when done, it can do a lot of work quickly. Maybe that could be a separate job to not prolong the runtime of anything else.

Maybe another argument could be made to say that tests/tools/main.rs should only contain tools that are useful to everyone, which means that the mess-in-the-middle sub-command is probably too specific and should rather go to internal-tools as well. However, having 'what-seems-like-shell-scripts' carry executable bits, seems like something useful to everyone.

Despite all of this, my disposition is to turn gix-testtools into a pure library, and move all new functionality to run within this repository into internal-tools, a crate the won't be published. After all, maybe the build times aren't that bad, I didn't check.

EliahKagan commented 1 month ago

Checking that fixtures have correct executable bits seems like something relevant to most users of gix-testtools, assuming most users of gix-testtools use it to run fixture scripts. So maybe it should go in gix-testtools. But that doesn't require that it go in main.rs: it could be implemented in a newly introduced public function in lib.rs and the utility that calls it could be elsewhere: either as a command-line tool in internal-tools, or just as a test case for gitoxide itself, alongside the top-level integration tests.

Byron commented 1 month ago

That's fair - let's put it into gix-testtools main.rs then - for now I wouldn't feel comfortable adding an actual 'test' or 'check' to the library though. If there is a demand, that could change, of course.