On Mon, May 20, 2019 at 10:05:49AM +0200, Hendrik Leppkes wrote: > On Mon, May 20, 2019 at 9:44 AM Reimar Döffinger > <reimar.doeffin...@gmx.de> wrote: > > On 20.05.2019, at 09:23, Mathieu Duponchelle <math...@centricular.com> > > wrote: > > > On 5/19/19 7:00 PM, Devin Heitmueller wrote: > > >> Are you suggesting he should make a patch which results in the > > >> indentation being wrong, and then submit a second patch which fixes > > >> the incorrect indentation introduced by the first patch? > > > > I think it should probably be up to the maintainer, but possibly yes. > > A lot of review still happens primarily by email, and if you re-indent code > > it becomes impossible to see what, if anything, you changed in that block. > > Enabling reviews of the patch via email means not re-indenting even if > > it becomes "wrong". > > Not everyone does reviews that way though, so some maintainers nowadays > > might prefer it differently? > > My main concern with such requests is that its added effort to even > make these patches, since many developers will just automatically > indent where appropriate (as it should be), or even have tooling to do > it for them. > Personally, I would have to use another editor to "un-indent" the > code, since my main editor automatically corrects this stuff for me in > newly written code. As such, I'll personally never comply with such > requests, because I think its silly, and it would be extraordinary > effort to even do it, first restoring original indent with a second > editor.
There's many ways to do it, but the reviewability issue is already solved by e.g. sending the patch as generated by diff -w (have not tried, but git send-email seems to accept that option), so I think you are overstating the effort a bit :) There is also the point that this should not be an issue normally: - It doesn't matter if it's just a small block of code. - If it's a large block of code, many times it means the code should have been refactored long ago and put into a separate function. So there often is the option of "volunteering" for some code cleanup first :) > I don't think its that much to ask for a reviewer to also use a tool > that can ignore whitespace. Maybe we can teach patchwork to offer a > button to view diffs without whitespace, if people don't want to use a > simple tool. FFmpeg used to have far too few reviewers. If that is still the case, I think it's reasonable to put reviewer convenience over submitter convenience. I don't see how patchwork is really useful: finding the patch there is already a pain, and we don't use it for reviews AT ALL, we use email. Now there might be an argument if we should have alternative/additional review tools to email, but it's a bit of a separate question. Personally, my suggestion would be that if you re-indented a lot and unless you know the most likely potential reviewers do not care, send a patch without whitespace changes at least in addition. I mean, I don't have a super-strong opinion, but if I have to dig through whitespace changes I'll probably just not review a patch, because far too often all I have at hand to do them is an email client... Best regards, Reimar Döffinger _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".