[Gluster-devel] Automatic clang-format for GitHub PRs

[Xavi Hernandez]

On Wed, Feb 10, 2021 at 1:33 PM Amar Tumballi <amar at kadalu.io> wrote:

>
>
> On Wed, Feb 10, 2021 at 3:29 PM Xavi Hernandez <xhernandez at redhat.com>
> wrote:
>
>> Hi all,
>>
>> I'm wondering if enforcing clang-format for all patches is a good idea...
>>
>> I've recently seen patches where clang-format is doing changes on parts
>> of the code that have not been touched by the patch. Given that all files
>> were already formatted by clang-format long ago, this shouldn't happen.
>>
>> This means that as the clang-format version evolves, the formatting with
>> the same configuration is not the same. This introduces unnecessary noise
>> to the file history that I think it should be avoided.
>>
>> Additionally, I've also seen some cases where some constructs are
>> reformatted in an uglier or less clear way. I think it's very hard to come
>> up with a set of rules that formats everything in the best possible way.
>>
>> For all these reasons, I would say we shouldn't enforce clang-format to
>> accept a PR. I think it's a good test to run to catch some clear formatting
>> issues, but it shouldn't vote for patch acceptance.
>>
>> What do you think ?
>>
>>
> One thing I have noticed is, as long as some test is 'skipped', no one
> bothers to check. It would be great if the whole diff (in case of failure)
> is posted as a comment, so we can consider that while merging. I would
> request one to invest time on posting the failure message as a comment back
> into issue from jenkins if possible, and later implement skip behavior.
> Otherwise, considering we have >10 people having ability to merge patches,
> many people may miss having a look on clang-format issues.
>

I agree that it could be hard to enforce some rules, but what I'm seeing
lately is that the clang-format version from Fedora 33 doesn't format the
code the same way as a previous version with the same configuration in some
cases (this also seems to happen with much older versions). This causes
failures in the clang check that need manual modifications to update the
patches.

Xavi
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.gluster.org/pipermail/gluster-devel/attachments/20210211/e0a5f437/attachment.html>