-
Notifications
You must be signed in to change notification settings - Fork 12.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make the pre-commit script pre-push instead #88313
Conversation
This should make it substantially less annoying, and hopefully more people will find it useful. In particular, it will no longer run tidy each time you run `git commit --amend` or rebase a branch. This also warns if you have the old script in pre-commit; see the HACK comment for details.
Hm, I do worry that this will presumably make a "fix tidy" commit at the end of a patch more likely. A pre-commit hook helps ensure all commits in a series are already well-formatted and such... |
I've thought a little bit more about this in the past few days and ultimately I'll leave it up to you (r=me if you want) but I am not sure this is the right move. In some sense, ideally tidy is fast enough that running it pre-commit is not noticeable. But I don't think that's true today. Without that, I doubt that pre-commit/pre-push hooks are ever going to be quite ideal, in practice. I don't use them myself partially for that reason, so it's hard for me to evaluate whether this recommendation is the right one to make. Part of me wants to suggest we provide the baseline script and then let users cp it into the right place (we can tell them / let them select, potentially, where) on x.py setup... |
@Mark-Simulacrum the main reason I want this is because I have a hunch a bunch of people have turned it off because it's too slow; I certainly have. I agree it does make a "fix formatting" commit more likely - maybe we could have it suggest |
Alternatively, we could suggest https://github.com/tummychow/git-absorb which hopefully will put it in the right commit without adding much work or adding a cleanup commit. |
Either option seems like a reasonable idea to me -- the first seems like the better path, in general, though more likely to run into rebase errors (though ones that arguably are "good" in the sense of ensuring that the diffs are all 'nice', without the "last" commit being bloated with unexpected formatting changes that actually belong in earlier commits). |
The nice thing is the algorithm for fixing these is simple enough to be automated (maybe I should write a tool for it at some point?), you just always choose the incoming change instead of the formatted one. |
Hm, yes, I imagine there's probably a merge strategy for that already? I think you can tell git which to use. |
I am not planning on following up on this. I still think pre-push instead of pre-commit is a better model for this but I don't really want to debate it, feel free to close if you disagree. |
@bors r+ rollup Seems OK to me. |
📌 Commit 9d664b2 has been approved by |
Make the pre-commit script pre-push instead This should make it substantially less annoying, and hopefully more people will find it useful. In particular, it will no longer run tidy each time you run `git commit --amend` or rebase a branch. This also warns if you have the old script in pre-commit; see the HACK comment for details. r? `@Mark-Simulacrum` cc `@caass`
Make the pre-commit script pre-push instead This should make it substantially less annoying, and hopefully more people will find it useful. In particular, it will no longer run tidy each time you run `git commit --amend` or rebase a branch. This also warns if you have the old script in pre-commit; see the HACK comment for details. r? ``@Mark-Simulacrum`` cc ``@caass``
Make the pre-commit script pre-push instead This should make it substantially less annoying, and hopefully more people will find it useful. In particular, it will no longer run tidy each time you run `git commit --amend` or rebase a branch. This also warns if you have the old script in pre-commit; see the HACK comment for details. r? ```@Mark-Simulacrum``` cc ```@caass```
Rollup of 13 pull requests Successful merges: - rust-lang#88313 (Make the pre-commit script pre-push instead) - rust-lang#91530 (Suggest 1-tuple parentheses on exprs without existing parens) - rust-lang#92724 (Cleanup c_str.rs) - rust-lang#93208 (Impl {Add,Sub,Mul,Div,Rem,BitXor,BitOr,BitAnd}Assign<$t> for Wrapping<$t> for rust 1.60.0) - rust-lang#93394 (Don't allow {} to refer to implicit captures in format_args.) - rust-lang#93416 (remove `allow_fail` test flag) - rust-lang#93487 (Fix linking stage1 toolchain in `./x.py setup`) - rust-lang#93673 (Linkify sidebar headings for sibling items) - rust-lang#93680 (Drop json::from_reader) - rust-lang#93682 (Update tracking issue for `const_fn_trait_bound`) - rust-lang#93722 (Use shallow clones for submodules managed by rustbuild, not just bootstrap.py) - rust-lang#93723 (Rerun bootstrap's build script when RUSTC changes) - rust-lang#93737 (bootstrap: prefer using '--config' over 'RUST_BOOTSTRAP_CONFIG') Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
This should make it substantially less annoying, and hopefully more
people will find it useful. In particular, it will no longer run tidy
each time you run
git commit --amend
or rebase a branch.This also warns if you have the old script in pre-commit; see the HACK
comment for details.
r? @Mark-Simulacrum cc @caass