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

./tools/check-typo-since trunk #1905

Merged
merged 3 commits into from Dec 9, 2018

Conversation

Projects
None yet
3 participants
@gasche
Copy link
Member

commented Jul 15, 2018

I have not learned to use the check-typo pre-commit-hook yet (and I think there are various situations where I want to run check-typo at other times than at pre-commits), but I already find just running ./tools/check-typo (to check the whole repository) very slow, it takes slightly above one minute on my machine.

This PR proposes an auxiliary tool, check-typo-since, which takes a git reference as argument, and runs check-typo on all the files that have changed since this reference -- due to further commit, or not-yet-committed changes. The following could be useful

./tools/check-typo-since trunk  # check my whole feature branch
./tools/check-typo-since HEAD~1 # check last commit
./tools/check-typo-since HEAD # check uncommitted changes

I first considered adding this feature to check-typo directly (--since trunk), but the control flow of check-typo is horrible spaghetti and I don't want to touch it.

To make check-typo-since work well, I had to make some tweaks to .gitattributes. In particular, instead of having /manual ocaml-typo=prune, which tells that the manual directory is to be pruned, I changed it into /manual** ocaml-typo=prune, which tells that manual and any file underneath is to be pruned. This makes the implementation of check-typo-since simple and easy, but one has to remember to always use the child-closure operator ** when pruning in gitattributes.

(I realize now that some of the logic is very close to the code of the pre-commit-hook, but that I made different choices which could also be used to simplify the pre-commit-hook's logic, if @dra27 agrees that they are sensible choices.)

@gasche gasche requested a review from dra27 Jul 15, 2018

@gasche gasche force-pushed the gasche:faster-check-typo branch from 9f86703 to 8afb771 Jul 15, 2018

@dra27
Copy link
Contributor

left a comment

Firstly, thanks for doing this, because I haven't got to writing it!

Given that this is a script for developers, I think it better to have a version which maybe wants more features in the future than spend ages without one.

It's worth fixing the couple of issues I've highlighted - longer term, doing git diff against trunk will cause the script to scan files which have been changed by upstream changes - it would be better (I think) to identify the merge-base with the branch specified and scan the commits inbetween for the list of files - which is what travis-ci.sh does.

Show resolved Hide resolved .gitattributes Outdated
Show resolved Hide resolved .gitattributes Outdated
Show resolved Hide resolved .gitattributes Outdated
Show resolved Hide resolved .gitattributes Outdated
Show resolved Hide resolved tools/check-typo-since Outdated
Show resolved Hide resolved tools/check-typo-since Outdated

@gasche gasche referenced this pull request Jul 17, 2018

Merged

Simpler check-typo #1910

@gasche gasche force-pushed the gasche:faster-check-typo branch 3 times, most recently from 493852b to 24eed32 Jul 18, 2018

@gasche

This comment has been minimized.

Copy link
Member Author

commented Jul 18, 2018

I have rebased this PR on top of #1910, and addressed the review comments -- but I think that another bashism-detection-check would not hurt.

@gasche gasche force-pushed the gasche:faster-check-typo branch from 24eed32 to 527d209 Jul 18, 2018

@gasche gasche closed this in #1910 Oct 19, 2018

@gasche gasche reopened this Oct 19, 2018

@gasche

This comment has been minimized.

Copy link
Member Author

commented Oct 19, 2018

(Not sure why this was automatically closed, but I need to rebase it on trunk now that #1910 is in.)

@gasche gasche force-pushed the gasche:faster-check-typo branch from 527d209 to 622f782 Oct 19, 2018

@gasche

This comment has been minimized.

Copy link
Member Author

commented Oct 19, 2018

I just rebased this PR on top of trunk; reviews are warmly welcome. (But then again, this is not high priority.)

@gasche gasche force-pushed the gasche:faster-check-typo branch from 622f782 to 46951bb Oct 19, 2018

@gasche gasche force-pushed the gasche:faster-check-typo branch from 46951bb to cf4e672 Nov 6, 2018

@gasche

This comment has been minimized.

Copy link
Member Author

commented Nov 6, 2018

Gentle ping: given that all issues of the previous review round have been addressed, could we accept this PR?

gasche added some commits Jul 15, 2018

.gitattributes: prune directories recursively
Before this change, check-typo would run on manual/Makefile for
example, while this file lives within a pruned directory so it
ought to be ignored by the tool.

Note: the check-typo code seems to assume that the only pruned things
are directory, it prints "pruned directory ..." when something is
pruned. I haven't changed this part of the logic; but note that normal
./check-typo invocation will only check pruning for directories.
check-typo: make --check-prune faster
this comment special-cases the prune-detection logic to use the `git
check-attr` layer directly, instead of using the convenience function
`get_attrs ..` which parses its output.

On my machine, calling --check-prune on the testsuite files goes from
17s to 12s when this patch is applied.
add a variant of check-typo to compare against a git reference
./tools/check-typo-since trunk
./tools/check-typo-since HEAD~10

In most cases, this should be much faster than running check-typo on
the whole directory.
@damiendoligez
Copy link
Member

left a comment

Looks good to me, but I'll let @dra27 have another look.

@gasche

This comment has been minimized.

Copy link
Member Author

commented Nov 23, 2018

Ping :-)

@gasche

This comment has been minimized.

Copy link
Member Author

commented Dec 8, 2018

@damiendoligez (or @dra27) would you approve the PR? The feature is useful and the diff touches non-critical part of the codebase.

@dra27

This comment has been minimized.

Copy link
Contributor

commented Dec 9, 2018

Sorry for the delay getting back to this one <pages out m4sh and pages back in awk> ... LGTM!

@dra27 dra27 merged commit b98d3e7 into ocaml:trunk Dec 9, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
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.