Supply a Git pre-commit hook for tools/check-typo#1148
Conversation
|
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 |
3be9f82 to
d5679b8
Compare
32d592e to
fe48e94
Compare
|
Hmm - need to figure out why the copyright-header warning is triggering for all these scripts, but this is nearly there... |
8803d89 to
eae8e4d
Compare
|
Lovely - so apparently gawk regexps don't work properly in Ubuntu 12.4.5. This all appears to be working now... |
eae8e4d to
7252b00
Compare
|
@dra27 didn't have time to look into the code so far, sorry.
General question: would it be easy / doable to extract the code that
adds check-typo to travis and make a PR with just that? To me the two
features are distinct and it would simplify discussions to have them in
separate PRs.
|
|
@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. |
|
Incidentally, the bulk of the code is to do with Travis - the pre-commit hook only needs a tiny tweak in tools/check-typo |
|
First of all, thanks for having rewritten the history, @dra27. It is
easier to read like this.
I have two technical questions.
1. What is the svnrules variable for, actually?
I realise it was there before, I am wondering whether it shouldn't be
renamed to something more appropriate?
2. On Travis, is it mandatory to create a check-typo-failed file?
Would it be possible to keep track of the failure in a shell variable or
do you really need something more persistent?
|
|
@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? |
|
David Allsopp (2017/04/20 02:54 -0700):
@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)?
Couldn't we just _not_ mention any revision control system and use
"rules"?
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 just went quickly over the code, so sorry if my question is naïve.
I think it's nice to accept file names with spaces, but I do not
understand why this implies using a sub shell?
I don't think it's possible to communicate the state
back without a file?
Even by using different exit codes?
|
|
@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. |
21dcaf6 to
30025af
Compare
|
@shindere - I was more testing that the script exists and, given that it's a script, |
|
David Allsopp (2018/06/15 01:43 -0700):
@shindere - I was more testing that the script exists and, given that
it's a script, `-x` seems better than `-e`?
But why do you need to test that it exists before running it?
In case it does not exist, wouldn't trying to run it triger an error
anyway?
b`./tools/check-typo` and `tools/check-typo` are equivalent - there's
a slash in there, so it's never `PATH`-searched?!
Ah I wasn't aware of that! Thanks!
|
|
@shindere - I was just going along with a neater error message than "command not found"! |
|
David Allsopp (2018/06/15 02:24 -0700):
@shindere - I was just going along with a neater error message than
"command not found"!
Ah! Well, to be honest given that the script is part of the repo I find
this a bit overkill (I would prefer not having to do that systematically
for every script of the repo one owuld wnat to run...) but if you
like it that way, I'm fine.
|
|
@damiendoligez - I've made enough subsequent changes that I'd like to double-check that this still has your approval before merging? |
|
@dra27 yes, we're still good. |
3d259e3 to
d87d065
Compare
Automatically runs tools/check-typo and rejects the commit if they don't pass.
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_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.
d87d065 to
b71c4a7
Compare
Without the leading slash, these apply in multiple places.
4bb97e3 to
2adc6af
Compare
|
Right, several things fixed:
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. |
This ensures that check-typo's default rules are picked up!
This GPR provides a pre-commit script which can be used as a Githook to automate the use of
tools/check-typowhen developing.I have ensured that
tools/check-typooperates on the index and not the working tree (you can test this by adding a long line or a tab character to a file, doinggit add -u, correcting the file in your working tree and then runninggit 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).