Req: apply the check selectively on just the changed files

[ns-osmolsky]

Context: a sizeable code base into which I had added clang-format. People use it... but only a subset of source lines is up to the spec. Obviously, I can run clang-format on all files... but that will pollute Git history.

Ask: apply clang-format just to the delta that is being merged.

Justification: this way the large code base will slowly and inevitably converge.

[jidicula]

A couple of approaches I can think of:

• Does the check-path argument help with this? For example, if some directories are formatted but others are not, you could set up a matrix option for check-path and progressively add directories as they become clean.

• Alternatively, you could use the exclude-regex to ensure this action doesn't check the noncompliant files.

Otherwise, if your request is to run clang-format checks on just a few lines that have changed (and not whole files), I'm not sure that's possible.

If I were in your shoes I'd still bite the bullet and just run it repo-wide, git history pollution be damned (that way all the formatting changes are isolated to a single commit). Then, if I need to use git blame for archaeological reasons, I'd ignore specific commits with the --ignore-rev option.

[ns-osmolsky]

A couple of approaches I can think of:

• Does the check-path argument help with this? For example, if some directories are formatted but others are not, you could set up a matrix option for check-path and progressively add directories as they become clean.
• Alternatively, you could use the exclude-regex to ensure this action doesn't check the noncompliant files.

Unfortunately, in this codebase, very few files are fully clang-format-compliant. I am trying to figure out a way to improve on that.

Otherwise, if your request is to run clang-format checks on just a few lines that have changed (and not whole files), I'm not sure that's possible.

Well, here is a tool that people use in their pre-commit hooks: https://github.com/llvm/llvm-project/blob/main/clang/tools/clang-format/clang-format-diff.py

I run it in my local repo via another helper script... but it comes down to this:

git diff -U0 --no-color --relative HEAD^ | clang-format-diff.py -p1 -i

I thought the Action may be able to do something similar. Basically we'd have to get a diff to the top of the destination branch, filter the source files and then check the resulting delta.

[ns-osmolsky]

I've just tried running the action and it simply scans files in the predefined directories. So, here are two feature requests:

1. The Action should just scan the changed file set, as scanning the whole tree is only feasible for projects that are already fully compliant. Also, scanning the whole tree is too costly when you have thousands on .cc/.cpp files...
2. It would be nice to have an option to scan just the changed regions of the files. The mechanics of such a mode are described above and this mode is predicated on the "selective" scan in Item (1).

[r-barnes]

I would also find this useful.

[jidicula]

Implementation caveat: need to be careful about differences in clang-format-py in older versions of clang-format - at some point, the tool switched from Python 2 to 3.

Possible solution: build a separate set of Docker images in the GHCR for this repo and base them on Ubuntu Python images for 2 or 3, depending on the major version of clang-format. Build definitions would be similar to those in https://github.com/jidicula/clang-format-action/blob/main/.github/workflows/clang-format-image.yml

[linuxmaniac]

clang-format-${CLANG_FORMAT_VERSION} package should provide already /usr/bin/clang-format-diff- ${CLANG_FORMAT_VERSION}
For instance: https://packages.ubuntu.com/jammy/amd64/clang-format-14/filelist
and the package already depends on the proper Python version, see: https://packages.ubuntu.com/jammy/clang-format-14

[jidicula]

clang-format-${CLANG_FORMAT_VERSION} package should provide already /usr/bin/clang-format-diff- ${CLANG_FORMAT_VERSION}
For instance: https://packages.ubuntu.com/jammy/amd64/clang-format-14/filelist
and the package already depends on the proper Python version, see: https://packages.ubuntu.com/jammy/clang-format-14

Neat, and it's available for all versions this action supports! https://packages.ubuntu.com/bionic/amd64/clang-format-3.9/filelist

[linuxmaniac]

@jidicula what about linuxmaniac@91849f4 as a base for getting this done?

Can you please take a look?

[tnoczyns-volue]

@linuxmaniac +1 for the feature.
@jidicula any chances of integrating this in the near future?

[moran-inadvantage]

+1. This feature would cut our pipeline down significantly.

[danra]

@moran-inadvantage How about PRing your branch?

[jidicula]

PRs are definitely welcome - sorry that I haven't had much time to look into this lately. @linuxmaniac or @moran-inadvantage if you want to PR your changes into this repo, go ahead and I'll approve the CI runs on it to see how these solutions work against the test matrix.