Add pre-push git hook #26699
Add pre-push git hook #26699
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
zanderso
force-pushed
the
git-hooks
branch
2 times, most recently
from
3c8ef11 to
eb2fffa
Compare
|
|
||
| SRC_ROOT = os.path.dirname(os.path.dirname(os.path.dirname(os.path.dirname(os.path.abspath(__file__))))) | ||
| FLUTTER_DIR = os.path.join(SRC_ROOT, 'flutter') | ||
| DART_BIN = os.path.join(SRC_ROOT, 'third_party', 'dart', 'tools', 'sdks', 'dart-sdk', 'bin') |
There was a problem hiding this comment.
I have vague memories of this causing subtle problems in the past, but I can't quite remember what. Something about versions being slightly off from what people would normally expect to work with, as well as confusion about which Dart binary was actually getting used and for what. @chinmaygarde may remember.
Overall the code in here looks good, but I'm wondering if we're making things overly complicated here - we have gclient invoke a python script, which tells git to invoke a python script, which invokes a Dart program, which invokes some bash scripts that we'll someday refactor to either python or Dart.
Why not just make everything python?
There was a problem hiding this comment.
I have vague memories of this causing subtle problems in the past, but I can't quite remember what. Something about versions being slightly off from what people would normally expect to work with, as well as confusion about which Dart binary was actually getting used and for what. @chinmaygarde may remember.
Git hooks only work with git version >= 2.9. Is that what you mean?
If you mean the prebuilt Dart SDK from CIPD, we already use it pretty consistently to run standalone Dart programs in the tree.
Overall the code in here looks good, but I'm wondering if we're making things overly complicated here - we have gclient invoke a python script, which tells git to invoke a python script, which invokes a Dart program, which invokes some bash scripts that we'll someday refactor to either python or Dart.
It's not quite that bad. To set the git config, gclient sync invokes a python script. Then separately, on a git push, git invokes the python script, which invokes the Dart program (which invokes a shell script, which invokes another Dart program, which invokes e.g. clang-tidy in the linter case.)
The part in parens can be simplified by refactoring the ci/bin/lint.dart and ci/bin/format.dart scripts into proper Dart packages. I've started doing that here: #26722. Then it's just python -> Dart -> clang-tidy. The python part is kind of mandatory due to the need to have a #! at the top for the git hook.
Why not just make everything python?
My first pass at the pre-push hook was python. There were two problems:
- I was thinking about how I was going to extend it to analyze only a minimal set of Dart code on changes to .dart files. I found myself reimplementing in python some logic that we already had implemented in Dart in
ci/bin/format.dartandci/bin/lint.dart. So that didn't seem like the right approach. - I was writing a lot of python, and I wanted to be able to lint the code and write tests for it. Currently that's easy in Dart, but we don't have any setup for that for python. We should add it for python, probably, but given (1) it seemed like the right thing was to just use Dart.
There was a problem hiding this comment.
My concerns weren't about the hooks setting for git, but more about whether we can trust the Dart binary. It's also possible that I'm mixing something up here - I think the problem may have come from trying to invoke Dart directly as a gclient hook (not a git hook). At any rate, if we're using it successfully elsewhere we should be fine.
This change LGTM
This comment was marked as spam.
This comment was marked as spam.
3 participants
This PR adds a pre-push git hook that runs the checks under
ci/lint.shandci/format.sh. On my machine the hook adds about 5 seconds to agit push. I should be able to speed that up after refactoringci/bin/lint.dartandci/bin/format.dartas proper Dart packages and depending on them directly as libraries. (Then we can add more checks).