Re: Coding Style Review and Automation

["Marek Marczykowski-Górecki" <marmarek@invisiblethingslab.com>]

[-- Attachment #1: Type: text/plain, Size: 2380 bytes --]

On Tue, Feb 11, 2025 at 03:06:08PM +0100, Roger Pau Monné wrote:
> On Tue, Feb 11, 2025 at 11:19:23AM +0100, Jan Beulich wrote:
> > On 11.02.2025 10:10, Luca Fancellu wrote:
> > >>> 3) The size of the patch after applying clang-format is huge. Really. Something
> > >>> like 9 MB. Even if everyone agrees that the approach is good and we can proceed
> > >>> with it, it is highly unlikely anyone will be able to review it. Considering
> > >>> that new patches are being added to the upstream during such a review, it may
> > >>> also lead to new code style violations or require a new review of that huge
> > >>> patch.
> > >>
> > >> I think this approach is difficult.  It would likely introduce a lot
> > >> of noise when using `git blame` (I know, it's just one extra jump,
> > >> but...), plus would likely break every patch that we currently have
> > >> in-flight.
> > > 
> > > I think we already discussed this in the past and having some churn was accepted,
> > > also about breaking existing patches, every change merged has the potential to do
> > > that, this one is more likely but it’s the game I guess?
> > 
> > That's easy to say if you have just a few patches in flight, yet I'm worried
> > about this when considering the hundreds of mine that are awaiting review.
> 
> There are also downstreams (including distros) with varying length of
> patch queues on top of Xen.  Arguably they have to rebase the queue
> every time they update, but a wide change in coding style will likely
> be fairly disruptive to them.
> 
> Don't take this as a reason to reject clang-format.  As mentioned
> elsewhere I think the format supported by clang-format would need to
> be fairly similar to the current Xen one (up to the point that chunks
> of code using the new and the old style could live together).  Then we
> would enforce it only for newly added chunks of code initially IMO.

While clang-format surely will force some changes (the earlier 9MB patch
is a data point here...), I generally expect it should match fairly
close to the current code style, and so the rebase shouldn't be _that_
painful. In some cases git likely will handle large part of the work
already.

It surely is a cost of introducing such a change, but IMO it's a cost
worth paying.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]