| Message ID | 20160809160703.GA10637@potion |
|---|---|
| State | New |
| Headers | show |
>> Make scripts/checkpatch.pl accept tabs in linux-headers/, instead of >> changing scripts/update-linux-headers.sh to expand tabs when importing. >> >> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> >> --- >> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl >> index 929708721299..38232d4b25c3 100755 >> --- a/scripts/checkpatch.pl >> +++ b/scripts/checkpatch.pl >> @@ -1355,7 +1355,7 @@ sub process { >> next if ($realfile !~ /\.(h|c|cpp|pl)$/); >> >> # in QEMU, no tabs are allowed >> - if ($rawline =~ /^\+.*\t/) { >> + if ($rawline =~ /^\+.*\t/ && $realfile !~ /^linux-headers\//) { >> my $herevet = "$here\n" . cat_vet($rawline) . "\n"; >> ERROR("code indent should never use tabs\n" . $herevet); >> $rpt_cleaners = 1; >> > > Could you do the same for standard-headers/ too? Sure, and when we are at it ... are *.pl files going to be reindented? e.g. checkpatch.pl uses tabs consistently, get_maintainer.pl is a horrible mix of tabs and spaces [1], and clean-header-guards.pl uses 4 spaces for an indent. --- 1: get_maintainer.pl assumes that tab is 8 spaces and uses 1 tab for every two consecutive indents and 4 spaces for indents that cannot be tabified, then tops the ugliness by also using 8 and 12 spaces for 2 and 3 indents, respectively.
2016-08-09 18:39+0200, Paolo Bonzini: > On 09/08/2016 18:37, Radim Krčmář wrote: >>>> Make scripts/checkpatch.pl accept tabs in linux-headers/, instead of >>>> changing scripts/update-linux-headers.sh to expand tabs when importing. >>>> >>>> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> >>>> --- >>>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl >>>> index 929708721299..38232d4b25c3 100755 >>>> --- a/scripts/checkpatch.pl >>>> +++ b/scripts/checkpatch.pl >>>> @@ -1355,7 +1355,7 @@ sub process { >>>> next if ($realfile !~ /\.(h|c|cpp|pl)$/); >>>> >>>> # in QEMU, no tabs are allowed >>>> - if ($rawline =~ /^\+.*\t/) { >>>> + if ($rawline =~ /^\+.*\t/ && $realfile !~ /^linux-headers\//) { >>>> my $herevet = "$here\n" . cat_vet($rawline) . "\n"; >>>> ERROR("code indent should never use tabs\n" . $herevet); >>>> $rpt_cleaners = 1; >>>> >>> >>> Could you do the same for standard-headers/ too? >> >> Sure, and when we are at it ... are *.pl files going to be reindented? >> >> e.g. checkpatch.pl uses tabs consistently, get_maintainer.pl is a >> horrible mix of tabs and spaces [1], and clean-header-guards.pl uses 4 >> spaces for an indent. > > I've sent a patch series to fix the most obvious issues in automated > checkpatch email, it skips the tab check on imported Perl scripts. I will base on that, thanks.
Hi,
Your series seems to have some coding style problems. See output below for
more information:
Message-id: 20160809160703.GA10637@potion
Type: series
Subject: [Qemu-devel] [PATCH] checkpatch: allow tabs in linux-headers
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git show --no-patch --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
e923294 checkpatch: allow tabs in linux-headers
=== OUTPUT BEGIN ===
Checking PATCH 1/1: checkpatch: allow tabs in linux-headers...
ERROR: code indent should never use tabs
#106: FILE: scripts/checkpatch.pl:1358:
+^I^Iif ($rawline =~ /^\+.*\t/ && $realfile !~ /^linux-headers\//) {$
total: 1 errors, 0 warnings, 8 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org
On Tue, 9 Aug 2016 12:14:14 -0400 (EDT) Paolo Bonzini <pbonzini@redhat.com> wrote: > > > ----- Original Message ----- > > From: "Radim Krčmář" <rkrcmar@redhat.com> > > To: no-reply@ec2-52-6-146-230.compute-1.amazonaws.com > > Cc: famz@redhat.com, ehabkost@redhat.com, mst@redhat.com, qemu-devel@nongnu.org, peterx@redhat.com, "jan kiszka" > > <jan.kiszka@web.de>, pbonzini@redhat.com, rth@twiddle.net > > Sent: Tuesday, August 9, 2016 6:07:04 PM > > Subject: [PATCH] checkpatch: allow tabs in linux-headers > > > > 2016-08-09 08:31-0700, no-reply@ec2-52-6-146-230.compute-1.amazonaws.com: > > > Hi, > > > > > > Your series seems to have some coding style problems. See output below for > > > more information: > > > > > > Message-id: 20160809150333.9991-1-rkrcmar@redhat.com > > > Type: series > > > Subject: [Qemu-devel] [PATCH for-2.7 0/2] intel-iommu: restrict EIM to > > > quirkless KVM > > > > > > === TEST SCRIPT BEGIN === > > > #!/bin/bash > > > > > > BASE=base > > > n=1 > > > total=$(git log --oneline $BASE.. | wc -l) > > > failed=0 > > > > > > commits="$(git log --format=%H --reverse $BASE..)" > > > for c in $commits; do > > > echo "Checking PATCH $n/$total: $(git show --no-patch --format=%s > > > $c)..." > > > if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; > > > then > > > failed=1 > > > echo > > > fi > > > n=$((n+1)) > > > done > > > > > > exit $failed > > > === TEST SCRIPT END === > > > > > > Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 > > > Switched to a new branch 'test' > > > e018fb0 intel-iommu: restrict EIM to quirkless KVM > > > 5ef6f2f linux-headers: update to v4.8-rc1 > > > > > > === OUTPUT BEGIN === > > > Checking PATCH 1/2: linux-headers: update to v4.8-rc1... > > > ERROR: code indent should never use tabs > > > #32: FILE: linux-headers/linux/kvm.h:885: > > > +^Iunion {$ > > > > > > ERROR: code indent should never use tabs > > > #33: FILE: linux-headers/linux/kvm.h:886: > > > +^I^I__u32 pad;$ > > > > > > ERROR: code indent should never use tabs > > > #34: FILE: linux-headers/linux/kvm.h:887: > > > +^I^I__u32 devid;$ > > > > > > ERROR: code indent should never use tabs > > > #35: FILE: linux-headers/linux/kvm.h:888: > > > +^I};$ > > > > > > ERROR: code indent should never use tabs > > > #43: FILE: linux-headers/linux/kvm.h:1034: > > > +#define KVM_MSI_VALID_DEVID^I(1U << 0)$ > > > > > > ERROR: code indent should never use tabs > > > #50: FILE: linux-headers/linux/kvm.h:1040: > > > +^I__u32 devid;$ > > > > > > ERROR: code indent should never use tabs > > > #51: FILE: linux-headers/linux/kvm.h:1041: > > > +^I__u8 pad[12];$ > > > > > > ERROR: code indent should never use tabs > > > #59: FILE: linux-headers/linux/kvm.h:1086: > > > +^IKVM_DEV_TYPE_ARM_VGIC_ITS,$ > > > > > > ERROR: code indent should never use tabs > > > #60: FILE: linux-headers/linux/kvm.h:1087: > > > +#define KVM_DEV_TYPE_ARM_VGIC_ITS^IKVM_DEV_TYPE_ARM_VGIC_ITS$ > > > > > > total: 9 errors, 0 warnings, 51 lines checked > > > > > > Your patch has style problems, please review. If any of these errors > > > are false positives report them to the maintainer, see > > > CHECKPATCH in MAINTAINERS. > > > > These indentation errors are false positives. > > ---8<--- > > Make scripts/checkpatch.pl accept tabs in linux-headers/, instead of > > changing scripts/update-linux-headers.sh to expand tabs when importing. > > > > Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> > > --- > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > > index 929708721299..38232d4b25c3 100755 > > --- a/scripts/checkpatch.pl > > +++ b/scripts/checkpatch.pl > > @@ -1355,7 +1355,7 @@ sub process { > > next if ($realfile !~ /\.(h|c|cpp|pl)$/); > > > > # in QEMU, no tabs are allowed > > - if ($rawline =~ /^\+.*\t/) { > > + if ($rawline =~ /^\+.*\t/ && $realfile !~ /^linux-headers\//) { > > my $herevet = "$here\n" . cat_vet($rawline) . "\n"; > > ERROR("code indent should never use tabs\n" . $herevet); > > $rpt_cleaners = 1; > > > > Could you do the same for standard-headers/ too? I think it would be better to not apply any qemu coding style checks to a headers update. Something like 'check if this contains header updates _only_' would make more sense, but that is beyond my nonexisting perl skills...
2016-08-10 09:09+0200, Cornelia Huck: > On Tue, 9 Aug 2016 12:14:14 -0400 (EDT) > Paolo Bonzini <pbonzini@redhat.com> wrote: >> > Make scripts/checkpatch.pl accept tabs in linux-headers/, instead of >> > changing scripts/update-linux-headers.sh to expand tabs when importing. >> > >> > Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> >> > --- >> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl >> > index 929708721299..38232d4b25c3 100755 >> > --- a/scripts/checkpatch.pl >> > +++ b/scripts/checkpatch.pl >> > @@ -1355,7 +1355,7 @@ sub process { >> > next if ($realfile !~ /\.(h|c|cpp|pl)$/); >> > >> > # in QEMU, no tabs are allowed >> > - if ($rawline =~ /^\+.*\t/) { >> > + if ($rawline =~ /^\+.*\t/ && $realfile !~ /^linux-headers\//) { >> > my $herevet = "$here\n" . cat_vet($rawline) . "\n"; >> > ERROR("code indent should never use tabs\n" . $herevet); >> > $rpt_cleaners = 1; >> > >> >> Could you do the same for standard-headers/ too? > > I think it would be better to not apply any qemu coding style checks to > a headers update. Something like 'check if this contains header updates > _only_' would make more sense, but that is beyond my nonexisting perl > skills... I have posted another vesion that does not check for any code style in hunks that modify linux-headers and include/standard-headers, http://lists.nongnu.org/archive/html/qemu-devel/2016-08/msg01824.html We still want to check header-only updates in other headers ... Your condition would draw attention to linux header updates that also touch other files, but I think that a diffstat is enough. The script would need some preprocessing to know that only headers are modified or buffering of errors until the script knows that only headers were modified; neither is hard, but the added complexity is not compensated by usefulness, IMO.
On Wed, 10 Aug 2016 15:55:28 +0200 Radim Krčmář <rkrcmar@redhat.com> wrote: > 2016-08-10 09:09+0200, Cornelia Huck: > > On Tue, 9 Aug 2016 12:14:14 -0400 (EDT) > > Paolo Bonzini <pbonzini@redhat.com> wrote: > >> > Make scripts/checkpatch.pl accept tabs in linux-headers/, instead of > >> > changing scripts/update-linux-headers.sh to expand tabs when importing. > >> > > >> > Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> > >> > --- > >> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > >> > index 929708721299..38232d4b25c3 100755 > >> > --- a/scripts/checkpatch.pl > >> > +++ b/scripts/checkpatch.pl > >> > @@ -1355,7 +1355,7 @@ sub process { > >> > next if ($realfile !~ /\.(h|c|cpp|pl)$/); > >> > > >> > # in QEMU, no tabs are allowed > >> > - if ($rawline =~ /^\+.*\t/) { > >> > + if ($rawline =~ /^\+.*\t/ && $realfile !~ /^linux-headers\//) { > >> > my $herevet = "$here\n" . cat_vet($rawline) . "\n"; > >> > ERROR("code indent should never use tabs\n" . $herevet); > >> > $rpt_cleaners = 1; > >> > > >> > >> Could you do the same for standard-headers/ too? > > > > I think it would be better to not apply any qemu coding style checks to > > a headers update. Something like 'check if this contains header updates > > _only_' would make more sense, but that is beyond my nonexisting perl > > skills... > > I have posted another vesion that does not check for any code style in > hunks that modify linux-headers and include/standard-headers, > http://lists.nongnu.org/archive/html/qemu-devel/2016-08/msg01824.html > > We still want to check header-only updates in other headers ... > Your condition would draw attention to linux header updates that also > touch other files, but I think that a diffstat is enough. > > The script would need some preprocessing to know that only headers are > modified or buffering of errors until the script knows that only headers > were modified; neither is hard, but the added complexity is not > compensated by usefulness, IMO. > If there's no quick way to check, it's not worth spending too much time on it, I agree.
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 929708721299..38232d4b25c3 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -1355,7 +1355,7 @@ sub process { next if ($realfile !~ /\.(h|c|cpp|pl)$/); # in QEMU, no tabs are allowed - if ($rawline =~ /^\+.*\t/) { + if ($rawline =~ /^\+.*\t/ && $realfile !~ /^linux-headers\//) { my $herevet = "$here\n" . cat_vet($rawline) . "\n"; ERROR("code indent should never use tabs\n" . $herevet); $rpt_cleaners = 1;