[0/2] checkpatch: Do appropriate kernel->qemu renaming

Message

Aleksandar Markovic June 20, 2020, 1:32 p.m. UTC

There are several places where 'kernel' is mentioned instead of
'qemu' in checkpatch.pl.

This small series corrects this.

Aleksandar Markovic (2):
  checkpatch: Rename top_of_kernel_tree() to top_of_qemu_tree()
  checkpatch: Change occurences of 'kernel' to 'qemu' in user messages

 scripts/checkpatch.pl | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

Comments

Peter Maydell June 20, 2020, 2:25 p.m. UTC | #1

On Sat, 20 Jun 2020 at 14:33, Aleksandar Markovic
<[e-mail]> wrote:
> There are several places where 'kernel' is mentioned instead of
> 'qemu' in checkpatch.pl.
>
> This small series corrects this.

So, the thing about this kind of change is that it's implicitly saying "we have
forked checkpatch and will not try to update to newer versions of it from
the kernel tree", because this sort of no-behavioural-change tends to
get in the way of that kind of update by obscuring the delta between
the kernel version and ours. Effectively I think we *have* ended up
with our own fork,
simply because we haven't cared to do that kind of update from the kernel's
version and we've gradually added our own changes to our copy. But it seems
like it's worth discussing the general principle.

thanks
-- PMM

Aleksandar Markovic June 20, 2020, 3:09 p.m. UTC | #2

суб, 20. јун 2020. у 16:25 Peter Maydell <[e-mail]> је
написао/ла:
>
> On Sat, 20 Jun 2020 at 14:33, Aleksandar Markovic
> <[e-mail]> wrote:
> > There are several places where 'kernel' is mentioned instead of
> > 'qemu' in checkpatch.pl.
> >
> > This small series corrects this.
>
> So, the thing about this kind of change is that it's implicitly saying "we have
> forked checkpatch and will not try to update to newer versions of it from
> the kernel tree", because this sort of no-behavioural-change tends to
> get in the way of that kind of update by obscuring the delta between
> the kernel version and ours. Effectively I think we *have* ended up
> with our own fork,
> simply because we haven't cared to do that kind of update from the kernel's
> version and we've gradually added our own changes to our copy. But it seems
> like it's worth discussing the general principle.
>

I do not say (explicitly or implicitly) that we should or should not
follow and attempt to mirror changes in checkpatch.pl from kernel. (In
fact, I think we should.)

However, I don't think that several differences (in this series, I
thinks altogether 7 lines) that would originate from difference of
names QEMU vs. kernel would not be any significant obstacle for a
potential future attempts to do comparison QEMU checkpatch vs kernel
chekpatch.

Take a look at two versions of top_of_kernel_tree below - they already
differ in their body. Left this way, QEMU's checkpatch version simply
violates basic naming principles in software development. And, it
looks we want to sacrifice the principle - just for the sake of
convenience of a potential engineer having 7 less line in his diff
(out of much more).

I don't insist on these two patches. I, of course, leave the decision
to Peter, Paolo, Stefan, Michael, or others tracking kernel's
checkpatch script.

Thanks,
Aleksandar

QEMU version:

sub top_of_kernel_tree {
my ($root) = @_;

my @tree_check = (
"COPYING", "MAINTAINERS", "Makefile",
"README.rst", "docs", "VERSION",
"linux-user", "softmmu"
);

foreach my $check (@tree_check) {
if (! -e $root . '/' . $check) {
return 0;
}
}
return 1;
}

Kernel version:

sub top_of_kernel_tree {
my ($root) = @_;

my @tree_check = (
"COPYING", "CREDITS", "Kbuild", "MAINTAINERS", "Makefile",
"README", "Documentation", "arch", "include", "drivers",
"fs", "init", "ipc", "kernel", "lib", "scripts",
);

foreach my $check (@tree_check) {
if (! -e $root . '/' . $check) {
return 0;
}
}
return 1;
}

> thanks
> -- PMM

Aleksandar Markovic June 20, 2020, 3:20 p.m. UTC | #3

суб, 20. јун 2020. у 17:09 Aleksandar Markovic
<[e-mail]> је написао/ла:
>
> суб, 20. јун 2020. у 16:25 Peter Maydell <[e-mail]> је
> написао/ла:
> >
> > On Sat, 20 Jun 2020 at 14:33, Aleksandar Markovic
> > <[e-mail]> wrote:
> > > There are several places where 'kernel' is mentioned instead of
> > > 'qemu' in checkpatch.pl.
> > >
> > > This small series corrects this.
> >
> > So, the thing about this kind of change is that it's implicitly saying "we have
> > forked checkpatch and will not try to update to newer versions of it from
> > the kernel tree", because this sort of no-behavioural-change tends to
> > get in the way of that kind of update by obscuring the delta between
> > the kernel version and ours. Effectively I think we *have* ended up
> > with our own fork,
> > simply because we haven't cared to do that kind of update from the kernel's
> > version and we've gradually added our own changes to our copy. But it seems
> > like it's worth discussing the general principle.
> >
>
> I do not say (explicitly or implicitly) that we should or should not
> follow and attempt to mirror changes in checkpatch.pl from kernel. (In
> fact, I think we should.)
>
> However, I don't think that several differences (in this series, I
> thinks altogether 7 lines) that would originate from difference of
> names QEMU vs. kernel would not be any significant obstacle for a
> potential future attempts to do comparison QEMU checkpatch vs kernel
> chekpatch.
>

Sorry, I meant to say:

However, I don't think that several differences (in this series, I
think, altogether 7 lines) that would originate from difference of
names (QEMU vs. kernel) would be any significant obstacle for a
potential future attempts to do comparison QEMU checkpatch vs kernel
checkpatch.



> Take a look at two versions of top_of_kernel_tree below - they already
> differ in their body. Left this way, QEMU's checkpatch version simply
> violates basic naming principles in software development. And, it
> looks we want to sacrifice the principle - just for the sake of
> convenience of a potential engineer having 7 less line in his diff
> (out of much more).
>
> I don't insist on these two patches. I, of course, leave the decision
> to Peter, Paolo, Stefan, Michael, or others tracking kernel's
> checkpatch script.
>
> Thanks,
> Aleksandar
>
> QEMU version:
>
> sub top_of_kernel_tree {
> my ($root) = @_;
>
> my @tree_check = (
> "COPYING", "MAINTAINERS", "Makefile",
> "README.rst", "docs", "VERSION",
> "linux-user", "softmmu"
> );
>
> foreach my $check (@tree_check) {
> if (! -e $root . '/' . $check) {
> return 0;
> }
> }
> return 1;
> }
>
> Kernel version:
>
> sub top_of_kernel_tree {
> my ($root) = @_;
>
> my @tree_check = (
> "COPYING", "CREDITS", "Kbuild", "MAINTAINERS", "Makefile",
> "README", "Documentation", "arch", "include", "drivers",
> "fs", "init", "ipc", "kernel", "lib", "scripts",
> );
>
> foreach my $check (@tree_check) {
> if (! -e $root . '/' . $check) {
> return 0;
> }
> }
> return 1;
> }
>
> > thanks
> > -- PMM

Stefan Hajnoczi June 23, 2020, 8:24 a.m. UTC | #4

On Sat, Jun 20, 2020 at 05:20:41PM +0200, Aleksandar Markovic wrote:
> суб, 20. јун 2020. у 17:09 Aleksandar Markovic
> <[e-mail]> је написао/ла:
> >
> > суб, 20. јун 2020. у 16:25 Peter Maydell <[e-mail]> је
> > написао/ла:
> > >
> > > On Sat, 20 Jun 2020 at 14:33, Aleksandar Markovic
> > > <[e-mail]> wrote:
> > > > There are several places where 'kernel' is mentioned instead of
> > > > 'qemu' in checkpatch.pl.
> > > >
> > > > This small series corrects this.
> > >
> > > So, the thing about this kind of change is that it's implicitly saying "we have
> > > forked checkpatch and will not try to update to newer versions of it from
> > > the kernel tree", because this sort of no-behavioural-change tends to
> > > get in the way of that kind of update by obscuring the delta between
> > > the kernel version and ours. Effectively I think we *have* ended up
> > > with our own fork,
> > > simply because we haven't cared to do that kind of update from the kernel's
> > > version and we've gradually added our own changes to our copy. But it seems
> > > like it's worth discussing the general principle.
> > >
> >
> > I do not say (explicitly or implicitly) that we should or should not
> > follow and attempt to mirror changes in checkpatch.pl from kernel. (In
> > fact, I think we should.)
> >
> > However, I don't think that several differences (in this series, I
> > thinks altogether 7 lines) that would originate from difference of
> > names QEMU vs. kernel would not be any significant obstacle for a
> > potential future attempts to do comparison QEMU checkpatch vs kernel
> > chekpatch.
> >
> 
> Sorry, I meant to say:
> 
> However, I don't think that several differences (in this series, I
> think, altogether 7 lines) that would originate from difference of
> names (QEMU vs. kernel) would be any significant obstacle for a
> potential future attempts to do comparison QEMU checkpatch vs kernel
> checkpatch.

I looked through the 2020, 2019, and 2018 git log for checkpatch.pl and
found few commits that were directly applied from Linux. Some were
rewritten from scratch for QEMU and inspired by Linux commits. In
practice QEMU maintains a fork rather than a mirror of checkpatch.pl.

Let's encourage improvements to checkpatch.pl, even if that means
diverging further from Linux, because it's a valuable tool for improving
coding style, quality, etc. If there is a feeling that changes to
checkpatch.pl are discouraged there will be more coding style
inconsistencies, reviewers will have to point out trivial issues
manually, etc because we're afraid to touch the script.

Stefan

Michael S. Tsirkin June 23, 2020, 8:26 a.m. UTC | #5

On Sat, Jun 20, 2020 at 03:32:05PM +0200, Aleksandar Markovic wrote:
> There are several places where 'kernel' is mentioned instead of
> 'qemu' in checkpatch.pl.
> 
> This small series corrects this.


fine by me

Acked-by: Michael S. Tsirkin <[e-mail]>

> Aleksandar Markovic (2):
>   checkpatch: Rename top_of_kernel_tree() to top_of_qemu_tree()
>   checkpatch: Change occurences of 'kernel' to 'qemu' in user messages
> 
>  scripts/checkpatch.pl | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> -- 
> 2.20.1