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

[Joe Julian]

I may not be a code contributor, but I do tend to read the code a lot to figure out how things work out to track down a bug. I find that format changes that are not related to code changes within the same commit just make the whole commit more complex and harder to read.

My preference would be that formatting fixes to existing code should be a separate commit.

On February 16, 2017 6:45:28 AM PST, Niels de Vos <ndevos at redhat.com> wrote:
>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

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.gluster.org/pipermail/gluster-devel/attachments/20170216/524bf3ab/attachment.html>