[1/2,RESEND] CODING_STYLE: explicitly call out label indentation
diff mbox series

Message ID 7072cc5e-8137-762b-53a1-c4a80d19ff08@suse.com
State New
Headers show
Series
  • ./CODING_STYLE adjustments
Related show

Commit Message

Jan Beulich July 19, 2019, 9:18 a.m. UTC
Since the behavior of "diff -p" to use an unindented label as context
identifier often makes it harder to review patches, make explicit the
requirement for labels to be indented.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

Comments

Tamas K Lengyel July 19, 2019, 1:18 p.m. UTC | #1
On Fri, Jul 19, 2019 at 3:19 AM Jan Beulich <JBeulich@suse.com> wrote:
>
> Since the behavior of "diff -p" to use an unindented label as context
> identifier often makes it harder to review patches, make explicit the
> requirement for labels to be indented.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

This style requirement wouldn't really work with astyle as-is.

Tamas
Jan Beulich July 19, 2019, 1:21 p.m. UTC | #2
On 19.07.2019 15:18, Tamas K Lengyel wrote:
> On Fri, Jul 19, 2019 at 3:19 AM Jan Beulich <JBeulich@suse.com> wrote:
>>
>> Since the behavior of "diff -p" to use an unindented label as context
>> identifier often makes it harder to review patches, make explicit the
>> requirement for labels to be indented.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> This style requirement wouldn't really work with astyle as-is.

Personally I view proper "diff -p" context in patches as quite
a bit more important than automatic style checking. But perhaps
that's just because I do quite a lot of patch review ...

Jan
Tamas K Lengyel July 19, 2019, 1:28 p.m. UTC | #3
On Fri, Jul 19, 2019 at 7:22 AM Jan Beulich <JBeulich@suse.com> wrote:
>
> On 19.07.2019 15:18, Tamas K Lengyel wrote:
> > On Fri, Jul 19, 2019 at 3:19 AM Jan Beulich <JBeulich@suse.com> wrote:
> >>
> >> Since the behavior of "diff -p" to use an unindented label as context
> >> identifier often makes it harder to review patches, make explicit the
> >> requirement for labels to be indented.
> >>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >
> > This style requirement wouldn't really work with astyle as-is.
>
> Personally I view proper "diff -p" context in patches as quite
> a bit more important than automatic style checking. But perhaps
> that's just because I do quite a lot of patch review ...

Which is a valid point. I don't really care which style option it
really is, if it helps you, I'm fine with it. But as a maintainer who
does this on a volunteer basis when I have a 5-minute window, I'm not
going to spend my time looking for style issues. So in the ongoing
vm_event series that's under review where you explicitly called out
that the vm_event maintainers have to review and point out all style
issues, that's a no-go from my side. So either we have automatic style
checks or I'm just going to Ack patches with style issues.

Tamas

Patch
diff mbox series

--- a/CODING_STYLE
+++ b/CODING_STYLE
@@ -31,6 +31,10 @@  void fun(void)
      }
  }
  
+Due to the behavior of GNU diffutils "diff -p", labels should be
+indented by at least one blank.  Non-case labels inside switch() bodies
+are preferred to be indented the same as the block's case labels.
+
  White space
  -----------