Add git pre-push hook #728
Conversation
|
Hey @tommy-gilligan, thanks for this! Just wanted to let you know that, between other work and time off for the holidays, we likely won't get a chance to review this for at least a few weeks, but we haven't forgotten about it. We'll follow up once everyone's back at work and we've cleared out our backlog. |
|
Thank you for updating me on this. You didn't have to do that and it is very kind of you! |
|
Okay, had a chance to look at this. Unfortunately you'll need to rebase changes that have happened in the interim. This looks great, though! My one concern would be how long it takes all of these scripts to execute locally; I assume it's not too much time, but it's also doing enough parsing and shell script work that it wouldn't surprise me if it was at least a noticeable increase. Thanks again for doing this! In the long run, I think this will save us a lot of needless headache with extra CI round trips. |
globstarApple switched to Zsh some years back and only an old version of Bash is kept for compatibility. This version of Bash does not support globstar. The scripts could be converted to Zsh but I think Bash is still mostly what is used on Linux out of the box, so I don't think that's a good idea. This is why I've replaced globstar with find in TimingA temporary change to Having already made some changes to Relying on Before check_fmt improvementAfter check_fmt improvementRemaining
|
There was a problem hiding this comment.
Amazing, thank you so much for your work on this!
The timings look totally acceptable to me - sub-one-second latency addition for git push shouldn't be a problem, especially given the benefit. You're more than welcome to work on optimizing, but maybe let's do that in a separate PR? That way we can isolate those changes in case it breaks anything.
The githook maybe adds a bit of noise to STDOUT/STDERR when running git from a terminal. Please let me know if anything should be done about this.
I'd recommend, in the pre-push script, just piping the stdout of each subcommand to /dev/null. We're good about printing errors to stderr, so this shouldn't swallow any.
Good idea!
I've ended up with everything but the formatting script redirecting to Additionally, I found a small programming error in |
tommy-gilligan
force-pushed
the
main
branch
3 times, most recently
from
5025fe2 to
41b70d9
Compare
There was a problem hiding this comment.
I'd recommend, in the pre-push script, just piping the stdout of each subcommand to /dev/null. We're good about printing errors to stderr, so this shouldn't swallow any.
I've ended up with everything but the formatting script redirecting to
/dev/null.rustfmt --checkuses stdout for displaying a diff that highlights files that need to be formatted and this will be useful for the user to see. Along the way I added-qto acargo install, I think it makes sense to keep that.
Sounds good!
Additionally, I found a small programming error in
check_versions.sh. Because it was small, I went ahead and fixed it here.
Oh, what was the error?
Sorry I should have explained: |
* [ci] Extract scripts in YAML to individual ci/*.sh * [ci] Rename check_job_dependencies for consistency * [ci] Run some checks on pre-push * [ci] check for yq before trying to run it * [ci] replace globstar for better compatibility * [ci] streamline check_fmt for performance * [ci] Use /usr/bin/env for improved compatibility * [ci] check that there are files for formatting * [ci] set check_all_toolchains_tested executable * [ci] make githook quieter (errors still show) * [ci] help user to know they are using pre-push hook * [ci] add documentation for git hook config * [ci] use function args instead of globals in check_versions.sh * [ci] fix formatting in git hooks documentation * [ci] direct all ci script stdout to null in git hook * [ci] make sure rustfmt stdout gets printed * [ci] explain why check_fmt.sh is not silenced
Previously, some scripts had output which would only be generated on failure, but this output was passed to stdout, which `githooks/pre-push` pipes to `/dev/null`. In this commit, any such output is unconditionally redirected to `stderr` - if there's any output, we want to see it. This previously resulted in very opaque errors during `git push`, which should be addressed by this commit. Also, in #728, we added `ci/check_all_toolchains_tested.sh`, but didn't add it to `githooks/pre-push`. In this commit, we fix that, and also add a test to `githooks/pre-push` to validate that all scripts in `ci/*` at least show up in `githooks/pre-push` by name. This isn't a foolproof check, but it should catch obvious errors.
Previously, some scripts had output which would only be generated on failure, but this output was passed to stdout, which `githooks/pre-push` pipes to `/dev/null`. In this commit, any such output is unconditionally redirected to `stderr` - if there's any output, we want to see it. This previously resulted in very opaque errors during `git push`, which should be addressed by this commit. Also, in #728, we added `ci/check_all_toolchains_tested.sh`, but didn't add it to `githooks/pre-push`. In this commit, we fix that, and also add a test to `githooks/pre-push` to validate that all scripts in `ci/*` at least show up in `githooks/pre-push` by name. This isn't a foolproof check, but it should catch obvious errors.
There are definitely different approaches that can be taken to sharing git hooks (some projects install git hooks automatically). Here I've gone with something simple: just have a directory
githooksthat the developer/user is expected to point their git at by manually changingcore.hooksPath.Documenting installation is necessary but I've not yet done it. Didn't want to go too far down the path I've chosen before getting feedback.
I also haven't yet done anything much to deal with stdout/stderr noise.