For many new contributors, we need to ask them to format their patch so that it matches the existing style. Even with recurring contributors, it is easy to forget reformatting after rebasing/making changes etc and fixing it is mostly a tedious affair.
I don't have much experience with CI stuff but I imagine it could be something where:
If we have high trust in the formatter: before your code gets merged, the commits are reformatted and then merged. I've seen this in practice before. This totally skips discussion around formatting altogether. This also avoids the "ugh, my PR failed because it's formatted wrong ".
If we have low-medium trust in the formatter: have the formatter add extra fixup! commits in PR that fix the formatting which can be looked at, and then automation takes care of rebasing the fixup! commits properly when the PR is "merged".
I'm sure there are probably a dozen ways to do this, IDK what the best one is.
How difficult would it be to set up automation for formatting? Are there good reasons to not implement something like this?
(To be clear: I'm not trying to discuss what style or exact clang-format options should be used. The question is about having automation do it for us.)
I was asking @xedin about this recently because for a long time I used to manually format changes block by block and sometimes(or many times) end up with some unformatted lines. After he told me about git-clang-format was thinking if maybe this could be automated using git hooks or something in a way that for like for git commit have a script that runs git-clang-format as post action and amends the commit automatically? Or maybe after a git add just run this automatically ... but in a way that can be turned on and off because some people may prefer not to use that way ...
Those are just some ideas, not really sure if any of this would work nice in practice, just somethings I was thinking about recently :)
I've worked on large projects in the past that used git pre-commit hooks for this purpose. I like them because they're relatively lightweight and it's easy to review changes before pushing, or skip formatting using git commit --no-verify. They do require some post clone setup though, to copy into .git/hooks (maybe update-checkout could do this automatically).
I'm not particularly opposed to doing this on CI, but I'd imagine that's a fair amount more work than improving the local tooling, which might be enough to cover 80-90% of cases.
I personally think that formatting code locally (automatically) before it’s pushed would be nice, as it can sometimes be hard to review poorly formatted code.
Whatever the workflow ends up being, I agree with others here that having some kind of automation for the formatting would be beneficial. IMO, having something integrated with CI would be nice, since it doesn't rely on all contributors setting up their hooks properly. To @suyashsrijan's point about reviewing unformatted code, maybe the formatting step could mostly be run as a pre-merge step, with the option to automatically apply a "fixup" commit via some @swift-ci command if a reviewer finds some PR particularly hard to grok?
As @owenv suggested, we can have update-checkout copy the hook; that should be easy to do. One additional requirement would be that developers would need to have clang-format installed. IMO that should be fine; a lot of projects ask you to have the formatter they use installed to format patches. In our case, it is a bit different that we build the formatter during the normal build, so in principle, we could use a locally built formatter instead of a pre-installed one, but I don't think it's too much of a stretch to ask people to have clang-format installed.
(To be clear, I'm not saying that let's go with hooks, but I'm saying that getting things set up should not be a lot of work.)
This is a good point. Having it easy to implement seems like a win. More implementation work => more room for bugs.
Yeah that's probably sufficient. I still anticipate we'll need the occasional "hey, could you run the formatter and push the changes?" comment on GitHub which just adds one more back-and-forth, but as @owenv notes, maybe 90% of cases covered is good enough compared to the required engineering effort.
".