Re: [RFC PATCH v2 1/1] scripts: Introduce a default git.orderFile

[Leonardo Bras]

On Wed, Sep 13, 2023 at 04:39:08AM -0300, Leonardo Bras wrote:
> On Tue, Sep 12, 2023 at 04:51:47PM -0300, Leonardo Bras wrote:
> > On Tue, Sep 12, 2023 at 04:53:11PM +0900, Masahiro Yamada wrote:
> > > On Tue, Sep 12, 2023 at 8:45 AM Leonardo Bras <leobras@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > > ---
> > > >
> > > > Please provide feedback on what else to add / remove / reorder here!
> > > >
> > > > Changes since RFCv1:
> > > > - Added Kconfig* (thanks Randy Dunlap!)
> > > > - Changed Kbuild to Kbuild* (improve matching)
> > > >
> > > >  scripts/git.orderFile | 32 ++++++++++++++++++++++++++++++++
> > > >  1 file changed, 32 insertions(+)
> > > >  create mode 100644 scripts/git.orderFile
> > > >
> > > > diff --git a/scripts/git.orderFile b/scripts/git.orderFile
> > > > new file mode 100644
> > > > index 000000000000..819f0a957fe3
> > > > --- /dev/null
> > > > +++ b/scripts/git.orderFile
> > > > @@ -0,0 +1,32 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > > +
> > > 
> > > 
> > > Please use "# SPDX-License-Identifier: GPL-2.0".
> > > 
> > > /* ... */ is not a valid comment style for the orderfile.
> > 
> > Oh, you are right.
> > My bad, it was a last minute change.
> > 
> > > 
> > > 
> > > 
> > > 
> > > > +# 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
> > > 
> > > 
> > > These comments are bogus.
> > > 
> > > 
> > > I guess this comment header was copied from QEMU,
> > 
> > Yes, I tried to adapt it from QEMU to kernel needs.
> > 
> > 
> > > but you changed the file path
> > > from scripts/git.orderfile to scripts/git.orderFile.
> > > 
> > > 
> > > You need to adjust the comment lines to
> > > 
> > > 
> > >     git diff -O scripts/git.orderFile ...
> > > 
> > >     git config diff.orderFile scripts/git.orderFile
> > > 
> > >
> > 
> > Adjusted, thanks!
> >  
> > > 
> > > Or, you need to get the file path back to scripts/git.orderfile
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > > +#
> > > > +
> > > > +MAINTAINERS
> > > > +
> > > > +# Documentation
> > > > +Documentation/*
> > > > +*.rst
> > > > +
> > > > +# build system
> > > > +Kbuild*
> > > > +Kconfig*
> > > > +Makefile*
> > > 
> > > 
> > > Kbuild* and Makefile* are interchangeable.
> > > (both are for GNU Make)
> > > 
> > > Kconfig* are different types.
> > > 
> > > 
> > > Better to arrange the order to
> > > 
> > > Kconfig*
> > > Kbuild*
> > > Makefile*
> > > 
> > > 
> > 
> > Oh, that makes sense.
> > Done!
> > 
> > > 
> > > 
> > > 
> > > > +*.mak
> > > 
> > > QEMU consistently uses only *.mak.
> > > 
> > > I just realized the kernel tree uses both *.mak and *.mk
> > > 
> > > masahiro@zoe:~/ref/linux(master)$ find .  -name '*.mak'
> > > ./tools/scripts/utilities.mak
> > > masahiro@zoe:~/ref/linux(master)$ find .  -name '*.mk'
> > > ./tools/testing/selftests/lib.mk
> > > ./tools/testing/selftests/ptp/testptp.mk
> > 
> > Sure, I will add '*.mk' after '*.mak', getting:
> > 
> > # build system
> > Kconfig*
> > Kbuild*
> > Makefile*
> > *.mak
> > *.mk
> > 
> > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > BTW, I quickly tested this, but
> > > it did not work as I expected.
> > > 
> > > 
> > > 
> > > 
> > > 
> > > masahiro@zoe:~/ref/linux(aaa)$ git diff --name-only d34599b^..d34599b
> > > MAINTAINERS
> > > drivers/Kconfig
> > > drivers/Makefile
> > > drivers/cache/Kconfig
> > > drivers/cache/Makefile
> > > drivers/cache/ax45mp_cache.c
> > > 
> > > masahiro@zoe:~/ref/linux(aaa)$ git diff --name-only -O
> > > scripts/git.orderFile d34599b^..d34599b
> > > MAINTAINERS
> > > drivers/cache/ax45mp_cache.c
> > > drivers/Kconfig
> > > drivers/Makefile
> > > drivers/cache/Kconfig
> > > drivers/cache/Makefile
> > > 
> > > masahiro@zoe:~/ref/linux(aaa)$ git diff --name-only -O
> > > scripts/git.orderFile d34599b..d34599b^
> > > MAINTAINERS
> > > drivers/cache/ax45mp_cache.c
> > > drivers/Kconfig
> > > drivers/Makefile
> > > drivers/cache/Kconfig
> > > drivers/cache/Makefile
> > > 
> > > 
> > > 
> > > 
> > > 
> > > My expectation was the following:
> > > 
> > > MAINTAINERS
> > > drivers/Kconfig
> > > drivers/cache/Kconfig
> > > drivers/Makefile
> > > drivers/cache/Makefile
> > > drivers/cache/ax45mp_cache.c
> > > 
> > > 
> > > It did not work like that.
> > > Am I missing something?
> > 
> > I can reproduce this same behavior for this commit list, and this is odd.
> > 
> > When I added a line-end at the .c extension, it works as expected:
> > 
> > *.c$
> > 
> > I think this makes sense. 
> > Just to make sure, I will add an line-end at every pattern with extension:
> > 
> > *.h$
> > *.c$
> > *.mk$
> 
> Oh, nevermind. This breaks the matching, and results are crazy.
> I will revert it on a v4.
> 
> The real solver is:
> */Kconfig*
> */Kbuild*
> */Makefile*

actually, to match root dir:

*Kconfig*
*Kbuild*
*Makefile*

> 
> The thing is that if I add just "Kconfig*" it only matches a Kconfig* in
> the root dir.
> 
> 
> > 
> > and so on.
> > Does that work for you?
> > 
> > 
> > I will send a v3 soon.
> > Thanks!
> > Leo
> > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > > +
> > > > +# semantic patches
> > > > +*.cocci
> > > > +
> > > > +# headers
> > > > +*.h
> 
> I was talking on a previous thread, and it would probably be interesting
> to add "*types.h" before *.h.
> 
> I need to think about a way to filter them out when matching "*.h", or it
> won't work because of:
> 
> Git doc:
> "The output order is determined by the order of glob patterns in <orderfile>.
> All files with pathnames that match the first pattern are output first, all
> files with pathnames that match the second pattern (but not the first) are
> output next, and so on."
> 
> i.e. the file will be put in the category of the last pattern it matches,
> and it makes harder to get "*types.h" before "*.h".
> 
> Trying to think on some solution.

Arg, nevermind. The matching failure caused a lot of confusion in my mind.

It works fine if we do:

*types.h
*.h
*.c

Will send a v4 soon.

> 
> > > > +
> > > > +# code
> > > > +*.c
> > > > --
> > > > 2.42.0
> > > >
> > > 
> > > 
> > > 
> > > 
> > > 
> > > -- 
> > > Best Regards
> > > Masahiro Yamada
> > >