Skip to content
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

Supply a Git pre-commit hook for tools/check-typo #1148

Merged
merged 12 commits into from Jun 30, 2018

Conversation

Projects
None yet
3 participants
@dra27
Copy link
Contributor

commented Apr 11, 2017

This GPR provides a pre-commit script which can be used as a Githook to automate the use of tools/check-typo when developing.

I have ensured that tools/check-typo operates on the index and not the working tree (you can test this by adding a long line or a tab character to a file, doing git add -u, correcting the file in your working tree and then running git commit) but I haven't checked that the other sections of the script read the "correct" version of .gitattributes and so forth. This is something which should be addressed if this is generalised to the CI (which should really check each commit in turn on a pull request).

@dra27

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2017

I know he'll have other things on his mind today and tomorrow, but this should definitely not be merged without @damiendoligez approving the changes in tools/check-typo.

@dra27 dra27 force-pushed the dra27:git-precommit-hook branch 6 times, most recently from 3be9f82 to d5679b8 Apr 13, 2017

@dra27 dra27 added the suspended label Apr 17, 2017

@dra27 dra27 force-pushed the dra27:git-precommit-hook branch from 32d592e to fe48e94 Apr 17, 2017

@dra27

This comment has been minimized.

Copy link
Contributor Author

commented Apr 17, 2017

Hmm - need to figure out why the copyright-header warning is triggering for all these scripts, but this is nearly there...

@dra27 dra27 force-pushed the dra27:git-precommit-hook branch 4 times, most recently from 8803d89 to eae8e4d Apr 18, 2017

@dra27 dra27 removed the work-in-progress label Apr 18, 2017

@dra27

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2017

Lovely - so apparently gawk regexps don't work properly in Ubuntu 12.4.5.

This all appears to be working now...

@dra27 dra27 force-pushed the dra27:git-precommit-hook branch from eae8e4d to 7252b00 Apr 18, 2017

@shindere

This comment has been minimized.

Copy link
Contributor

commented Apr 18, 2017

@dra27

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2017

@shindere - please see the caml-devel email I just sent! These are three stacked features - while we can merge any or all of them, they are related so I prefer to leave one GPR while we discuss and then split it before merging.

@dra27

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2017

Incidentally, the bulk of the code is to do with Travis - the pre-commit hook only needs a tiny tweak in tools/check-typo

@shindere

This comment has been minimized.

Copy link
Contributor

commented Apr 20, 2017

@dra27

This comment has been minimized.

Copy link
Contributor Author

commented Apr 20, 2017

@shindere - thanks! I expect that all instances of svn could be renamed to git - I think it's just the rules that have been read from the .gitattributes file (I think the svn equivalent would have been properties)? I wrote the code defensively to allow for spaces in file names (because I'm a Windows user!) which means that the while loop is a subshell ... I don't think it's possible to communicate the state back without a file?

@shindere

This comment has been minimized.

Copy link
Contributor

commented Apr 20, 2017

@damiendoligez

This comment has been minimized.

Copy link
Member

commented Jun 2, 2017

@shindere we can't just call them "rules" because there are two kinds of rules, the default ones (rules) and the ones given by git attributes (svnrules).

@dra27 the changes to check-typo look OK to me (except I don't really understand the changes to the git commands) but the consensus on the developers list was that it's better to check the end result of a PR rather than every commit. I'm not sure how this relates to the fact that you're working on the index rather than the working tree.

@dra27 dra27 force-pushed the dra27:git-precommit-hook branch from 21dcaf6 to 30025af Aug 12, 2017

@dra27 dra27 referenced this pull request Aug 12, 2017

Merged

Various changes for tools/check-typo #1287

3 of 3 tasks complete

@damiendoligez damiendoligez added this to the 4.07-or-later milestone Sep 29, 2017

@damiendoligez damiendoligez removed this from the consider-for-4.07 milestone Jun 5, 2018

@dra27

This comment has been minimized.

Copy link
Contributor Author

commented Jun 15, 2018

@shindere - does that look sufficient in 3d259e3? Are those tests only run on trunk?

@shindere

This comment has been minimized.

Copy link
Contributor

commented Jun 15, 2018

@dra27

This comment has been minimized.

Copy link
Contributor Author

commented Jun 15, 2018

@shindere - I was more testing that the script exists and, given that it's a script, -x seems better than -e? ./tools/check-typo and tools/check-typo are equivalent - there's a slash in there, so it's never PATH-searched?!

@shindere

This comment has been minimized.

Copy link
Contributor

commented Jun 15, 2018

@dra27

This comment has been minimized.

Copy link
Contributor Author

commented Jun 15, 2018

@shindere - I was just going along with a neater error message than "command not found"!

@shindere

This comment has been minimized.

Copy link
Contributor

commented Jun 15, 2018

@dra27

This comment has been minimized.

Copy link
Contributor Author

commented Jun 15, 2018

@damiendoligez - I've made enough subsequent changes that I'd like to double-check that this still has your approval before merging?

@damiendoligez

This comment has been minimized.

Copy link
Member

commented Jun 18, 2018

@dra27 yes, we're still good.

@dra27 dra27 force-pushed the dra27:git-precommit-hook branch from 3d259e3 to d87d065 Jun 30, 2018

dra27 added some commits Apr 11, 2017

Supply a Git pre-commit hook for tools/check-typo
Automatically runs tools/check-typo and rejects the commit if they don't
pass.
Allow tools/check-typo to analyse non-HEAD commits
Three alterations to tools/check-typo:

1. Binary check may be on any commit where the default is HEAD
2. .gitattributes may be read from any commit, rather than just the
   working tree
3. Detection of files under version control may be relative to a
   specific commit, rather than relying on git ls-files
Travis sanity checks should use PR commits
TRAVIS_COMMIT_RANGE is not correct unless a PR is based on the tip of
the target branch - it will include commits being merged from the target
branch as well.

The check-typo, changes and tests builds now use
$TRAVIS_BRANCH..$TRAVIS_PULL_REQUEST_SHA which gives the precise range
of commits included in the pull request (i.e. the author's) only.

@dra27 dra27 force-pushed the dra27:git-precommit-hook branch from d87d065 to b71c4a7 Jun 30, 2018

Correct .gitattributes for ocaml-typo=prune
Without the leading slash, these apply in multiple places.

@dra27 dra27 force-pushed the dra27:git-precommit-hook branch from 4bb97e3 to 2adc6af Jun 30, 2018

@dra27

This comment has been minimized.

Copy link
Contributor Author

commented Jun 30, 2018

Right, several things fixed:

  • The hook script wasn't marked as executable (I developed it on Windows, so didn't notice...)
  • Files which have been deleted would result in an error message - they're now filtered out
  • The prune attribute wasn't taken into account, which meant that commits to the manual would fail. The process for checking that is more involved than would be nice. Ideally, we'd change the gitattributes to be /manual/** and then every file would show-up with prune however then the directory itself doesn't, so we'd need two entries for every pruned directory. So I opted for a recursive bash function because, well, why not.
  • Technically, the prune attribute should be being applied to directories in the root only (as it happens, manual/manual was also getting it), so I tidied that).

These issues were found because I've done a scan of check-typo over all open PRs which don't require rebasing (because of conflicts). In the interests of getting this merged, assuming it passes CI, I'll merge but if anyone wants to pick up any issues in the last 3 commits, please do and I'll open a new GPR.

Prefix calls to check-typo with ./
This ensures that check-typo's default rules are picked up!

@dra27 dra27 merged commit 3522037 into ocaml:trunk Jun 30, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@shindere shindere referenced this pull request Jul 2, 2018

Merged

add a release checklist #1866

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.