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

[Leonardo Bras]

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$

and so on.
Does that work for you?


I will send a v3 soon.
Thanks!
Leo

> 
> 
> 
> 
> 
> > +
> > +# semantic patches
> > +*.cocci
> > +
> > +# headers
> > +*.h
> > +
> > +# code
> > +*.c
> > --
> > 2.42.0
> >
> 
> 
> 
> 
> 
> -- 
> Best Regards
> Masahiro Yamada
>