Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Automatic code formatting #1104

Open
liorsu opened this issue Mar 21, 2024 · 4 comments
Open

Automatic code formatting #1104

liorsu opened this issue Mar 21, 2024 · 4 comments

Comments

@liorsu
Copy link

liorsu commented Mar 21, 2024

Background
NFS-Ganesha is embracing the Kernel coding style and the checkpatch.pl to verify commit style.

The suggestion here is to stick with the existing coding style that Ganesha chose while integrating with clang-format for automatic code formatting.

We do that internally and that saves us a lot of development time from dealing with code formatting.

We can take the kernel .clang-format configuration file from- https://github.com/torvalds/linux/blob/master/.clang-format place it in the Ganesha root and run a one-time phase to format all the existing code (find . -iname *.h -o -iname *.c | xargs clang-format -i).

I tried it locally and there are many changes but they are not massive.

We can configure the pre-commit hook to verify that the formatting doesn't violate the clang-format configuration, similar to what checkpatch.pl is doing, just that it will be much easier to fix.

We can also configure git-blame to ignore the commit that does the reformatting (https://docs.github.com/en/repositories/working-with-files/using-files/viewing-a-file#ignore-commits-in-the-blame-view)

@ffilz WDYT?, we'd be happy to contribute that from what we do internally if the community has interest in it.

@ffilz
Copy link
Member

ffilz commented Mar 21, 2024

Push a patch to gerrithub and let's talk.

@liorsu
Copy link
Author

liorsu commented Mar 25, 2024

I uploaded the commits for that for review- https://review.gerrithub.io/q/topic:clang-format

Note that for the commit- https://review.gerrithub.io/c/ffilz/nfs-ganesha/+/1187343, checkpatch has many warnings and errors
Most are false positive and I saw discussions for (for example not recognizing a pointer correctly and mistaking it to be a multiplication operator that requires spaces around it).

I added a commit that changes the pre-commit hooks to verify the clang-format.
Clang-format has integration with many IDEs so developers can configure their IDE to use clang-format to automatically format their code without dealing with formatting at all.

We can also add to gerrit a clang-format verifier instead of the existing checkpath.

If that will be accepted I'll also update the documentation regarding clang-format instead of checkpatch.pl.

@xiaods
Copy link

xiaods commented May 17, 2024

I uploaded the commits for that for review- https://review.gerrithub.io/q/topic:clang-format

Note that for the commit- https://review.gerrithub.io/c/ffilz/nfs-ganesha/+/1187343, checkpatch has many warnings and errors Most are false positive and I saw discussions for (for example not recognizing a pointer correctly and mistaking it to be a multiplication operator that requires spaces around it).

I added a commit that changes the pre-commit hooks to verify the clang-format. Clang-format has integration with many IDEs so developers can configure their IDE to use clang-format to automatically format their code without dealing with formatting at all.

We can also add to gerrit a clang-format verifier instead of the existing checkpath.

If that will be accepted I'll also update the documentation regarding clang-format instead of checkpatch.pl.

please resolve the conflict files on gerrit.

@ffilz
Copy link
Member

ffilz commented May 17, 2024

This will require a re-submit when we are ready to merge it. We're not quite ready, but soon...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants