Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5;
From:   lsoaresp@redhat.com
To:     Masahiro Yamada <masahiroy@kernel.org>
Cc:     Leonardo Bras <leobras@redhat.com>,
        Randy Dunlap <rdunlap@infradead.org>,
        Nicolas Schier <nicolas@fjasle.eu>,
        Nathan Chancellor <nathan@kernel.org>,
        Nick Desaulniers <ndesaulniers@google.com>,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        linux-kernel@vger.kernel.org, linux-kbuild@vger.kernel.org
Subject: Re: [RFC PATCH v5 1/1] scripts: Introduce a default git.orderFile
Date:   Mon, 11 Dec 2023 10:13:52 -0300
Message-ID: <ZXcLEKSBg9Bd1qEu@LeoBras>
In-Reply-To: <CAK7LNAQk3Nm53qf95p7yQbSQ_M3phD_n5OMGxFWorGg_4fpQZg@mail.gmail.com>
References: <20231208181802.88528-3-leobras@redhat.com> <CAK7LNAQk3Nm53qf95p7yQbSQ_M3phD_n5OMGxFWorGg_4fpQZg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
Precedence: bulk

From: Leonardo Bras <masahiroy@kernel.org>

On Sun, Dec 10, 2023 at 04:13:54AM +0900, Masahiro Yamada wrote:
> On Sat, Dec 9, 2023 at 3:19 AM Leonardo Bras <leobras@redhat.com> wrote:
> >
> > When reviewing patches, it looks much nicer to have some changes shown
> > before others, which allow better understanding of the patch before the
> > the .c files reviewing.
> >
> > Introduce a default git.orderFile, in order to help developers getting the
> > best ordering easier.
> >
> > Signed-off-by: Leonardo Bras <leobras@redhat.com>
> > Acked-by: Randy Dunlap <rdunlap@infradead.org>
> >
> > ---
> > Changes since RFCv4:
> > - Added scripts/* into "build system" section
> > - Added "git-specific" section with this script and .gitignore
> > - Thanks for this feedback Nicolas!
> >
> > Changes since RFCv3:
> > - Added "*types.h" matching so type headers appear before regular headers
> > - Removed line ends ($) in patterns: they previously provided a
> >   false-positive
> > - Fixed build patterns to allow matching Kconfig, Kbuild & Makefile
> >   in any subdirectory
> >
> > Changes since RFCv2:
> > - Fixed licence comment to from /**/ to #
> > - Fixed filename in how-to comment
> > - Fix build order: Kconfig -> Kbuild -> Makefile
> > - Add *.mk extension
> > - Add line-ends ($) to make sure and get the correct extensions
> > - Thanks Masahiro Yamada for above suggestions!
> > - 1 Ack, thanks Randy!
> >
> > Changes since RFCv1:
> > - Added Kconfig* (thanks Randy Dunlap!)
> > - Changed Kbuild to Kbuild* (improve matching)
> >
> >
> >  scripts/git.orderFile | 39 +++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 39 insertions(+)
> >  create mode 100644 scripts/git.orderFile
> >
> > diff --git a/scripts/git.orderFile b/scripts/git.orderFile
> > new file mode 100644
> > index 0000000000000..31649ff53d22c
> > --- /dev/null
> > +++ b/scripts/git.orderFile
> > @@ -0,0 +1,39 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +
> > +# order file for git, to produce patches which are easier to review
> > +# by diffing the important stuff like header changes first.
> > +#
> > +# one-off usage:
> > +#   git diff -O scripts/git.orderFile ...
> > +#
> > +# add to git config:
> > +#   git config diff.orderFile scripts/git.orderFile
> > +#
> > +
> > +MAINTAINERS
> > +
> > +# Documentation
> > +Documentation/*
> > +*.rst
> > +
> > +# git-specific
> > +.gitignore
> > +scripts/git.orderFile
> 

Hello Masahiro, thanks for the feedback!

> 
> 
> I think scripts/git.orderFile should be part of
> "scripts/*" below.
> 
> 
> 
> 
> 
> 
> > +
> > +# build system
> > +*Kconfig*
> > +*Kbuild*
> > +*Makefile*
> 
> I do not like this because "foo-Makefile-bar"
> is not a Makefile, but would match "*Makefile*".

That makes sense.

> 
> 
> If you do not use wildcard at all, 'Makefile'
> will match to the root-dir and sub-directories.

I tried a quick test here changing an mm/*.c file and mm/Makefile, and the 
above will print the .c file changes first in any situation here, so it 
won't have the desired behavior.

But if we want to achieve the above we can do so with a slight change in 
the suggestion:

> 
> 
> Kconfig
> */Kconfig*
> Kbuild
> Makefile
*/Makefile
> *.mak
> *.mk
> scripts/*
> 
> 
> may satisfy your needs mostly.
> 

I have tried the following in the Kernel root:

$ find . |grep Makefile |grep -v Makefile$
./arch/arm/mach-s3c/Makefile.s3c64xx
./arch/mips/Makefile.postlink
./arch/powerpc/Makefile.postlink
./arch/um/Makefile-os-Linux
./arch/um/Makefile-skas
./arch/um/scripts/Makefile.rules
./arch/x86/Makefile_32.cpu
./arch/x86/Makefile.um
./arch/x86/Makefile.postlink
./arch/riscv/Makefile.postlink
./drivers/firmware/efi/libstub/Makefile.zboot
./drivers/usb/serial/Makefile-keyspan_pda_fw
[...]

$ find . |grep Kbuild |grep -v Kbuild$
./arch/mips/Kbuild.platforms
./scripts/Kbuild.include

Which leads to an honest question: 
Don't we want to show changes on those files before C files, for example?

If so, we need something like:

# build system
Kconfig*
*/Kconfig*
Kbuild*
*/Kbuild*
Makefile*
*/Makefile*
*.mak
*.mk
scripts/*

It would get rid of "foo-Makefile-bar" case but still match 
"Makefile-bar" case, which seems to be used around.

Is that ok?

Thanks!
Leo