CI check for committing files in .gitignore#11346
Conversation
|
It's possible to add an ignored file by using |
545b9d3 to
0a48b2f
Compare
|
Many thanks for this PR, @dra27
Regarding your `.gitignore` change: I find the current way rather subtle
and error prone, and this has nothing to do with your change. To me,
it's rather that the fact that you had to do this change is in itself a
proof of how error-prone the current situation is.
It would seem a better alternative to me to melt all the `.gitignore`
files in the toplevel one, ven those of the manual. What do you think?
|
I agree, the prefixing forward-slashes look very strange! I think I reviewed the original PR for that change and didn't notice the problem then, either. I'd certainly see no problem with either merging all the |
|
I am fine with both merging the |
|
Rebased to remove the commits now subsumed by #11617. If we do go ahead with #11343 (and stop committing the That said, this could be merged now and we can have the check regardless. |
|
I'd be happy to approve andmerge this PR, I believe it's sane to do this
check.
One suggestion and one question:
Suggestion: `Check no ignored files committed`: how about using singular
(file) rather than plural (files) andadding "is" before committed?
Question: wouldn't it be worth adding this check to the pre-commit hook?
If that's being done, am I correct that the addition of an ignored file
oculd be caught during a cherry-pick or a rebase?
|
a799930 to
ee407af
Compare
|
Oops, I seem to have dropped this one:
Agree with having a sentence, but not with making it singular (I think it reads too pedantically, even for me!). I changed the message to match the commit message that it's just "Check that no ignored files have been committed"
The pre-commit hooks aren't run during cherry-pick or rebase, sadly. |
|
The code is obviously correct. Thanks.
I am still a bit puzzled that there is no way to run the test locally.
I do realise that adding the check to the pre-commit hook would not be a
complete solution, but as it would catch at least some mistakes, isn't
it worth doing so?
One other possibility would be to add the test to check-typo, which
would let users run it locally. What do you think?
As a last suggestion, which is slightly orthogonal to the previous one,
I'd like to suggest to isolate the test in its own script under `tools/`
to make it easier to invoke it from several places, rather than having
the logic embedded in only one file from which it can't be re-used.
|
ee407af to
b070d75
Compare
Good idea! Done as |
|
This is good to go as far as I am concerned so approving and merging.
|
CI check for committing files in .gitignore (cherry picked from commit 0250779)
This is a follow-on for #11343: if we stop committing
configure, we should ensure that it doesn't accidentally (or nefariously) get recommitted.There was one place in the testsuite where we commit a broken .cmi file which I've worked-around by usingcopyand there are.gitignorefiles in the manual's directories which had incorrect patterns.An example of this test failing can be seen at https://github.com/dra27/ocaml/runs/7002042376?check_suite_focus=true#step:6:8.