search

nfs-ganesha / nfs-ganesha

NFS-Ganesha is an NFSv3,v4,v4.1 fileserver that runs in user mode on most UNIX/Linux systems
1.45k stars 507 forks source link

Automatic code formatting #1104

Open liorsu opened 3 months ago

liorsu commented 3 months ago

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 commented 3 months ago

Push a patch to gerrithub and let's talk.

liorsu commented 3 months ago

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 commented 2 months ago

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 commented 2 months ago

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