automatic formatting checks for pull requests?

Hi Hans

Can we add automatic formatting checks for pull requests?
Made a PR: https://github.com/php/php-src/pull/13417

It would be nice to have some formatting rules to harmonize the codebase as it is somewhat the wild west,
but as far as my understanding goes is that Clang format struggles to understand our codebase (namely macros) and is difficult to set-up for php-src.

Right. Consistent code style is nice, but what we have now is really
not that bad. There are a couple things I'd want if we enforce code
style:

• Fixing the style should be easy, running a single command without
  first pushing to CI.
• It should be fast too, so that I can easily run it for every commit,
  preferably even on-save in my editor.
• The new code style should be applied only to newly added sections or
  changed code, not entire files. Otherwise, we'll have many changes in
  large files, with endless merge conflicts when merging up from lower
  branches.
• The formatting tool should work for all php-src code, not just plain
  C code. We don't want to be forced to refactor old macros just because
  we need to add a single line to some long-standing code. Last time I
  tried clang-format, it utterly failed with our macros.

I haven't looked at your PR in detail, so I'm not sure which of these
points it satisfies. It would be great if you could quickly describe
how it works, and what the goals are.

Essentially, I'm just sceptical that this isn't more trouble than it's worth.

Ilija

IMO, clang-format isn't really suitable. Its untunable style is often far from the coding style that we currently have, and it makes some really odd choices as to when and how to wrap lines, making code definitely less readable.

cheers
Derick

Hi Hans

Can we add automatic formatting checks for pull requests?
Made a PR: https://github.com/php/php-src/pull/13417

It would be nice to have some formatting rules to harmonize the codebase as it is somewhat the wild west,
but as far as my understanding goes is that Clang format struggles to understand our codebase (namely macros) and is difficult to set-up for php-src.

Right. Consistent code style is nice, but what we have now is really
not that bad. There are a couple things I'd want if we enforce code
style:

• Fixing the style should be easy, running a single command without
  first pushing to CI.
• It should be fast too, so that I can easily run it for every commit,

Yeah.. Basically clang-format-diff (not clang-format, but clang-format-diff)
is difficult to work with, the easiest approach would be to make a
full copy of the current working dir,
and in that copy instruct git to reset to master and apply the diff
between master and the parent working dir,
and then have clang-format-diff analyze it and make its formatting suggestions,
-but- that will not be fast, that will be slow. Haven't found a fast
way to get clang-format-diff to work with a dirty workingdir against
master,
-yet- at least. Feel free to investigate, I'm pretty sure it can be
done, but don't know how to do it.

preferably even on-save in my editor.

If we can make it fast, we could make some opt-in git-commit-hook to format it
(but we need to figure out the dirty-working-dir-speed thing first)

• The new code style should be applied only to newly added sections or
  changed code, not entire files. Otherwise, we'll have many changes in
  large files, with endless merge conflicts when merging up from lower
  branches.

Yeah, that's already in place :)

• The formatting tool should work for all php-src code, not just plain
  C code. We don't want to be forced to refactor old macros just because
  we need to add a single line to some long-standing code. Last time I
  tried clang-format, it utterly failed with our macros.

Seems to work with macros when I tested it :) Tested it with the
following macros as of writing:
ZEND_PARSE_PARAMETERS_START
Z_PARAM_NUMBER
ZEND_PARSE_PARAMETERS_END
Z_TYPE_P
Z_LVAL_P
UNEXPECTED
ZEND_ASSERT
RETURN_DOUBLE
RETURN_LONG
RETURN_THROWS

While .php and .phpt and whatever formatting can be implemented in the future,
I would prefer baby steps, once we have *.[ch] accepted, improving it
with .php/whatever
should be easier in the future.

I haven't looked at your PR in detail, so I'm not sure which of these
points it satisfies. It would be great if you could quickly describe
how it works, and what the goals are.

Essentially, I'm just sceptical that this isn't more trouble than it's worth.

Ilija

Here's how it works, roughly:
for pull requests:

• creates a new dedicated CI job specifically to check for formatting errors
• If formatting errors are detected, the CI fails, with the last
  message being a command to fix the styling issue, it looks like, and I
  quote:

please run
curl https://termbin.com/zdzn | git apply -v

running that command, followed by git commit+git push should make the CI happy,
git apply will not stage/commit/push automatically however, so in
practice people will have to manually do:

curl https://termbin.com/zdzn | git apply -v;
(optionally run a git diff to just check that it looks right)
git commit -a --message 'formatting';
git push;

• we could add the last 2 steps to the actual error message, to make
  all 3 steps a copypasta.. would not be opposed.

More technically, it

• creates a new dedicated CI job specifically to check for formatting errors
• Ubuntu 22.04, clang-format-diff, php-cli, git, netcat
• creates a diff between current PR code and master branch
• does a bunch of tricks/hacks to get the environment suitable for
  clang-format-diff including switching to master,
  applying the diff to master, commiting the diff under a fake
  username/email (because CI git doesn't have a username/email by
  default and git refuse to commit without them)
• actually runs clang-format-diff against the last commit to format
  all the code in the last commit
• generate a diff against the changes clang-format-diff just made
• revert the last "fake username/email commit"
• if that clang-format-diff is empty, it just prints "code formatting
  check succeeded :)" and the CI finishes successfully
• if it's not empty, it prints, and I quote:

+ cat ../formatted.diff
diff --git a/test.c b/test.c
index 6df39be994..64cfab717d 100644
--- a/test.c
+++ b/test.c
@@ -1,5 +1,7 @@
#include <stdio.h>
-int main(){
- printf("Hello, World!\n");return 0;
+int main()
+{
+ printf("Hello, World!\n");
+ return 0;
}
\ No newline at end of file
+ echo 'code formatting check failed :('
code formatting check failed :(
++ cat ../formatted.diff
++ nc termbin.com 9999
/home/runner/work/_temp/3131a261-eaca-4987-9f8c-1b12e4645d76.sh: line
20: warning: command substitution: ignored null byte in input
+ patch_url=https://termbin.com/ul0f
Please run
+ echo 'Please run'
+ echo 'curl https://termbin.com/ul0f | git apply -v'
+ exit 1
curl https://termbin.com/ul0f | git apply -v
Error: Process completed with exit code 1.

and the CI run fails.

I think there is room for improvement with regards to all the
hacks/tricks currently done to get
the environment suitable for clang-format-diff, but I also think it's
already good enough for a CI
(but not good enough to comfortably run locally)

FWIW many real PHP projects already have something very similar called
StyleCI today, where StyleCI will give a link to a patch to fix
styling issues after pushing to
a pull request, for example chrome-php/chrome (
https://github.com/chrome-php/chrome/ ) use it: https://styleci.io

Right. Consistent code style is nice, but what we have now is really
not that bad. There are a couple things I'd want if we enforce code
style:

• Fixing the style should be easy, running a single command without
  first pushing to CI.
• It should be fast too, so that I can easily run it for every commit,
  preferably even on-save in my editor.
• The new code style should be applied only to newly added sections or
  changed code, not entire files. Otherwise, we'll have many changes in
  large files, with endless merge conflicts when merging up from lower
  branches.

Surely the best way is to apply the formatting tool on all branches that are supported (even in security support).
Have them be merged upwards, and then add the revisions of the commits to a .git-blame-ignore-revs file so that git blame doesn't care about them.

This should resolve the issue of making future merges difficult.

Best regards,

Gina P. Banyard