[Gluster-devel] Mixing style and other changes in a patch

[Niels de Vos]

On Thu, Feb 16, 2017 at 08:41:23AM -0500, Jeff Darcy wrote:
> In the last few days, I've seen both of these kinds of review comments
> (not necessarily on my own patches or from the same reviewers).
> 
> (a) "Please fix the style in the entire function where you changed one line."
> 
> (b) "This style change should be in a separate patch."
> 
> It's clearly not helpful to have patches delayed for both reasons.
> Which should prevail?  I think our general practice has been more
> toward (b) and that's also my personal preference.  In that case
> instances of (a) should not occur.  Or maybe people feel it should be
> the other way around.  Can we get a consensus here?

I do not like to have patches change the coding style alone. Only for
the lines at the beginning of the function I prefer to see (a) being
followed, but blocking a patch should not be the case. Still, if some of
the regular contributors do not follow the coding-style, I tend to get
annoyed with it and my give them a -1 in the hope they do repeat that in
the future. If it is really the only thing that bothers me, I tend to
send an updated patch for them (after a chat on IRC).

Doing coding-style fixes on lines that have no other modifications tend
to make 'git blame' more difficult to follow. I use that a lot when
trying to figure out when a problem has been introduced. So in general,
I really do not like (b).

Niels
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.gluster.org/pipermail/gluster-devel/attachments/20170216/fbd3bdb1/attachment.sig>