GH-109408: Move the C file whitespace check from patchcheck to pre-commit

[AA-Turner]

• Issue: Move Azure Pipelines CI to GitHub Actions #109408

[AlexWaygood]

On the one hand, the patchcheck script will currently autofix this issue for you if run it locally, and it seems a shame to delete that functionality without adding it to pre-commit. On the other hand, I like having this check as part of pre-commit, and I also somewhat doubt that many people are trying to commit C source code that uses tabs in it into CPython -- so maybe the ability to autofix tabs isn't actually that useful after all.

[AA-Turner]

We could change it to run python -c "python -c "import sys; n=(t:=(f:=open(sys.argv[1],'r+b')).read()).replace(b'\t',b' '*8); f.seek(0); f.write(n); sys.exit(t!=n)" as a 'system' script (or add a script in Tools/scripts to achieve the same effect) -- I believe that this would alter the source files if run locally.

A

[AlexWaygood]

We could change it to run python -c "python -c "import sys; n=(t:=(f:=open(sys.argv[1],'r+b')).read()).replace(b'\t',b' '*8); f.seek(0); f.write(n); sys.exit(t!=n)" as a 'system' script (or add a script in Tools/scripts to achieve the same effect) -- I believe that this would alter the source files if run locally.

A

I vote for the latter -- code golf is fun but pretty unmaintainable ;)

[AA-Turner]

I vote for the latter -- code golf is fun but pretty unmaintainable ;)

Done, I've just reused untabify, which is what we currently use -- wins all around!

A

[hugovk]

process() now has return values, which go to main(), but main() always returns None so we never get any errors here.

[AA-Turner]

Oh, I didn't realise! pre-commit was failing in my local testing when I introduced tabs, perhaps it just fails when a file is changed after the hook has run.

[hugovk]

We were running on .c and .h files before:

c_files= [fnforfninfile_pathsiffn.endswith(('.c', '.h'))]

Both of those are matched by the c type, so this would be closer to parity:

Suggested change

	types_or: ['c', 'c++']
	types_or: [c]

We do have half a dozen .cpp files in the codebase, do we want to expand to include them? Should we also add c++ for trailing-whitespace above?

[AA-Turner]

The test passed with 'c++' included, I imagined that it was a previous oversight that they weren't included. I'll check if the trailing whitespace check also passes.

[AA-Turner]

Tools/msi/bundle/bootstrap/PythonBootstrapperApplication.cpp fails with 16 lines changed. Other than that all good (@zooba would you be alright with us enabling the trailing whitespace check here? No real views either way, if you'd prefer to keep the whitespace then that's the status quo anyway!)

A

[hugovk]

(We can always remove c++ later if needed.)

[hugovk]

It's so much clearer like this! 👍

[hugovk]

@AA-Turner Looks good, please could you resolve the conflict?

[hugovk]

Thanks!

[miss-islington]

Thanks @AA-Turner for the PR, and @hugovk for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

[miss-islington]

Thanks @AA-Turner for the PR, and @hugovk for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

[miss-islington]

Sorry, @AA-Turner and @hugovk, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker f5edb56328b46f262b74a53343b8098a3934f761 3.12 

[miss-islington]

Sorry, @AA-Turner and @hugovk, I could not cleanly backport this to 3.11 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker f5edb56328b46f262b74a53343b8098a3934f761 3.11 

[bedevere-app]

GH-110636 is a backport of this pull request to the 3.12 branch.

[bedevere-app]

GH-110640 is a backport of this pull request to the 3.11 branch.