All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC PATCH v5 1/1] scripts: Introduce a default git.orderFile
@ 2023-12-08 18:18 Leonardo Bras
  2023-12-09 19:13 ` Masahiro Yamada
  2023-12-12  7:31 ` Christoph Hellwig
  0 siblings, 2 replies; 13+ messages in thread
From: Leonardo Bras @ 2023-12-08 18:18 UTC (permalink / raw)
  To: Randy Dunlap, Nicolas Schier, Masahiro Yamada, Nathan Chancellor,
	Nick Desaulniers, Mauro Carvalho Chehab, Leonardo Bras
  Cc: linux-kernel, linux-kbuild

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
+
+# build system
+*Kconfig*
+*Kbuild*
+*Makefile*
+*.mak
+*.mk
+scripts/*
+
+# semantic patches
+*.cocci
+
+# headers
+*types.h
+*.h
+
+# code
+*.c
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [RFC PATCH v5 1/1] scripts: Introduce a default git.orderFile
  2023-12-08 18:18 [RFC PATCH v5 1/1] scripts: Introduce a default git.orderFile Leonardo Bras
@ 2023-12-09 19:13 ` Masahiro Yamada
  2023-12-11 13:13   ` lsoaresp
  2023-12-12  7:31 ` Christoph Hellwig
  1 sibling, 1 reply; 13+ messages in thread
From: Masahiro Yamada @ 2023-12-09 19:13 UTC (permalink / raw)
  To: Leonardo Bras
  Cc: Randy Dunlap, Nicolas Schier, Nathan Chancellor,
	Nick Desaulniers, Mauro Carvalho Chehab, linux-kernel,
	linux-kbuild

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



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*".


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


Kconfig
*/Kconfig*
Kbuild
Makefile
*.mak
*.mk
scripts/*


may satisfy your needs mostly.




















> +*.mak
> +*.mk
> +scripts/*
> +
> +# semantic patches
> +*.cocci
> +
> +# headers
> +*types.h
> +*.h
> +
> +# code
> +*.c
> --
> 2.43.0
>


-- 
Best Regards
Masahiro Yamada

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC PATCH v5 1/1] scripts: Introduce a default git.orderFile
  2023-12-09 19:13 ` Masahiro Yamada
@ 2023-12-11 13:13   ` lsoaresp
  2023-12-11 13:17     ` Leonardo Bras Soares Passos
  2023-12-11 18:05     ` Masahiro Yamada
  0 siblings, 2 replies; 13+ messages in thread
From: lsoaresp @ 2023-12-11 13:13 UTC (permalink / raw)
  To: Masahiro Yamada
  Cc: Leonardo Bras, Randy Dunlap, Nicolas Schier, Nathan Chancellor,
	Nick Desaulniers, Mauro Carvalho Chehab, linux-kernel,
	linux-kbuild

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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC PATCH v5 1/1] scripts: Introduce a default git.orderFile
  2023-12-11 13:13   ` lsoaresp
@ 2023-12-11 13:17     ` Leonardo Bras Soares Passos
  2023-12-11 18:05     ` Masahiro Yamada
  1 sibling, 0 replies; 13+ messages in thread
From: Leonardo Bras Soares Passos @ 2023-12-11 13:17 UTC (permalink / raw)
  To: Masahiro Yamada
  Cc: Randy Dunlap, Nicolas Schier, Nathan Chancellor,
	Nick Desaulniers, Mauro Carvalho Chehab, linux-kernel,
	linux-kbuild

On Mon, Dec 11, 2023 at 10:14 AM <lsoaresp@redhat.com> wrote:
>
> From: Leonardo Bras <masahiroy@kernel.org>

Sorry about this, there seems to be a bug in my send-email script.
I will get it fixed.

[...]


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC PATCH v5 1/1] scripts: Introduce a default git.orderFile
  2023-12-11 13:13   ` lsoaresp
  2023-12-11 13:17     ` Leonardo Bras Soares Passos
@ 2023-12-11 18:05     ` Masahiro Yamada
  2023-12-11 22:03       ` lsoaresp
  1 sibling, 1 reply; 13+ messages in thread
From: Masahiro Yamada @ 2023-12-11 18:05 UTC (permalink / raw)
  To: lsoaresp
  Cc: Leonardo Bras, Randy Dunlap, Nicolas Schier, Nathan Chancellor,
	Nick Desaulniers, Mauro Carvalho Chehab, linux-kernel,
	linux-kbuild

On Mon, Dec 11, 2023 at 10:14 PM <lsoaresp@redhat.com> wrote:
>
> 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.



Hmm, you are right.


OK, your suggestion below looks good.


Thanks.






>
> 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
>
>


-- 
Best Regards
Masahiro Yamada

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC PATCH v5 1/1] scripts: Introduce a default git.orderFile
  2023-12-11 18:05     ` Masahiro Yamada
@ 2023-12-11 22:03       ` lsoaresp
  2023-12-11 22:41         ` leobras
  0 siblings, 1 reply; 13+ messages in thread
From: lsoaresp @ 2023-12-11 22:03 UTC (permalink / raw)
  To: Masahiro Yamada
  Cc: Leonardo Bras, lsoaresp, Randy Dunlap, Nicolas Schier,
	Nathan Chancellor, Nick Desaulniers, Mauro Carvalho Chehab,
	linux-kernel, linux-kbuild

From: Leonardo Bras <leobras@redhat.com>

On Tue, Dec 12, 2023 at 03:05:38AM +0900, Masahiro Yamada wrote:
> On Mon, Dec 11, 2023 at 10:14 PM <lsoaresp@redhat.com> wrote:
> >
> > 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.
> 
> 
> 
> Hmm, you are right.
> 
> 
> OK, your suggestion below looks good.
> 
> 
> Thanks.

Thank you for this feedback!

I will send a v6 shortly.

Thank you!
Leo

> 
> 
> 
> 
> 
> 
> >
> > 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
> >
> >
> 
> 
> -- 
> Best Regards
> Masahiro Yamada
> 


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC PATCH v5 1/1] scripts: Introduce a default git.orderFile
  2023-12-11 22:03       ` lsoaresp
@ 2023-12-11 22:41         ` leobras
  0 siblings, 0 replies; 13+ messages in thread
From: leobras @ 2023-12-11 22:41 UTC (permalink / raw)
  To: Masahiro Yamada
  Cc: Leonardo Bras, Randy Dunlap, Nicolas Schier, Nathan Chancellor,
	Nick Desaulniers, Mauro Carvalho Chehab, linux-kernel,
	linux-kbuild

From: Leonardo Bras <leobras@redhat.com>

On Mon, Dec 11, 2023 at 07:03:50PM -0300, lsoaresp@redhat.com wrote:
> From: Leonardo Bras <leobras@redhat.com>
> 
> On Tue, Dec 12, 2023 at 03:05:38AM +0900, Masahiro Yamada wrote:
> > On Mon, Dec 11, 2023 at 10:14 PM <lsoaresp@redhat.com> wrote:
> > >
> > > 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.
> > 
> > 
> > 
> > Hmm, you are right.
> > 
> > 
> > OK, your suggestion below looks good.
> > 
> > 
> > Thanks.
> 
> Thank you for this feedback!
> 
> I will send a v6 shortly.
> 
> Thank you!
> Leo
> 
> > 
> > 
> > 
> > 
> > 
> > 
> > >
> > > 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
> > >
> > >
> > 
> > 
> > -- 
> > Best Regards
> > Masahiro Yamada
> > 


RFCv6 patch at:
https://lore.kernel.org/all/20231211221338.127407-1-leobras@redhat.com/


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC PATCH v5 1/1] scripts: Introduce a default git.orderFile
  2023-12-08 18:18 [RFC PATCH v5 1/1] scripts: Introduce a default git.orderFile Leonardo Bras
  2023-12-09 19:13 ` Masahiro Yamada
@ 2023-12-12  7:31 ` Christoph Hellwig
  2023-12-12  8:09   ` Masahiro Yamada
  1 sibling, 1 reply; 13+ messages in thread
From: Christoph Hellwig @ 2023-12-12  7:31 UTC (permalink / raw)
  To: Leonardo Bras
  Cc: Randy Dunlap, Nicolas Schier, Masahiro Yamada, Nathan Chancellor,
	Nick Desaulniers, Mauro Carvalho Chehab, linux-kernel,
	linux-kbuild

Urrg, hell no.  Alphabetic order is the only one allowing sensible
searching.  If you have a different preference use your local .gitconfig
instead of enforcing completely random preference on others.


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC PATCH v5 1/1] scripts: Introduce a default git.orderFile
  2023-12-12  7:31 ` Christoph Hellwig
@ 2023-12-12  8:09   ` Masahiro Yamada
  2023-12-12 13:08     ` Christoph Hellwig
  0 siblings, 1 reply; 13+ messages in thread
From: Masahiro Yamada @ 2023-12-12  8:09 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Leonardo Bras, Randy Dunlap, Nicolas Schier, Nathan Chancellor,
	Nick Desaulniers, Mauro Carvalho Chehab, linux-kernel,
	linux-kbuild

On Tue, Dec 12, 2023 at 4:32 PM Christoph Hellwig <hch@infradead.org> wrote:
>
> Urrg, hell no.  Alphabetic order is the only one allowing sensible
> searching.  If you have a different preference use your local .gitconfig
> instead of enforcing completely random preference on others.



Unlike .gitignore, this feature is opt-in rather than enforced.

To use this, you need to run

'git config diff.orderFile scripts/git.orderFile'

or

'git diff -C scripts/git.orderFile'



Indeed, the file order is subjective, leaving
us a question "do we need it in upstream"?

At least, it is harmless for people who have no interest.


--
Best Regards
Masahiro Yamada

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC PATCH v5 1/1] scripts: Introduce a default git.orderFile
  2023-12-12  8:09   ` Masahiro Yamada
@ 2023-12-12 13:08     ` Christoph Hellwig
  2023-12-12 17:09       ` Leonardo Bras
  0 siblings, 1 reply; 13+ messages in thread
From: Christoph Hellwig @ 2023-12-12 13:08 UTC (permalink / raw)
  To: Masahiro Yamada
  Cc: Christoph Hellwig, Leonardo Bras, Randy Dunlap, Nicolas Schier,
	Nathan Chancellor, Nick Desaulniers, Mauro Carvalho Chehab,
	linux-kernel, linux-kbuild

On Tue, Dec 12, 2023 at 05:09:21PM +0900, Masahiro Yamada wrote:
> Unlike .gitignore, this feature is opt-in rather than enforced.
> 
> To use this, you need to run
> 
> 'git config diff.orderFile scripts/git.orderFile'
> 
> or
> 
> 'git diff -C scripts/git.orderFile'

Oh, ok.  That greatly reduces my concern.

> 
> Indeed, the file order is subjective, leaving
> us a question "do we need it in upstream"?
> 
> At least, it is harmless for people who have no interest.

.. but this is still a good question.  I'm not really sure there is
much of a need for it, but as long as it doesn't harm everyone else
I'm at least neutral on it.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC PATCH v5 1/1] scripts: Introduce a default git.orderFile
  2023-12-12 13:08     ` Christoph Hellwig
@ 2023-12-12 17:09       ` Leonardo Bras
  2023-12-15 17:02         ` Masahiro Yamada
  0 siblings, 1 reply; 13+ messages in thread
From: Leonardo Bras @ 2023-12-12 17:09 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Leonardo Bras, Masahiro Yamada, Randy Dunlap, Nicolas Schier,
	Nathan Chancellor, Nick Desaulniers, Mauro Carvalho Chehab,
	linux-kernel, linux-kbuild

On Tue, Dec 12, 2023 at 05:08:34AM -0800, Christoph Hellwig wrote:
> On Tue, Dec 12, 2023 at 05:09:21PM +0900, Masahiro Yamada wrote:
> > Unlike .gitignore, this feature is opt-in rather than enforced.
> > 
> > To use this, you need to run
> > 
> > 'git config diff.orderFile scripts/git.orderFile'
> > 
> > or
> > 
> > 'git diff -C scripts/git.orderFile'
> 
> Oh, ok.  That greatly reduces my concern.

Yes, it's an opt-in, so no user should be directly impacted.

> 
> > 
> > Indeed, the file order is subjective, leaving
> > us a question "do we need it in upstream"?

The main idea is patch generation.
This file's order is supposed to be the best order for reading a raw patch 
and understanding the code changes. 

> > 
> > At least, it is harmless for people who have no interest.
> 
> .. but this is still a good question.  I'm not really sure there is
> much of a need for it, but as long as it doesn't harm everyone else
> I'm at least neutral on it.

diff.orderfile was introduced in git to help order the git diff, and thus 
the patch generation, in a way that it's easier to understand what the 
commit / patch intends on doing. 

Take this example introducing a feature foo, you should see:
- Documentation on foo, if introduced
- How is foo enabled in build system, if needed
- The types / stucts / fields introduced by foo, if any
- The interface for using foo, if any
- The actual foo implementation.

Of course the actual order is open to discussion, and I encourage everyone 
to suggest any other items or order.

Thanks!
Leo


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC PATCH v5 1/1] scripts: Introduce a default git.orderFile
  2023-12-12 17:09       ` Leonardo Bras
@ 2023-12-15 17:02         ` Masahiro Yamada
  2023-12-15 18:30           ` Leonardo Bras Soares Passos
  0 siblings, 1 reply; 13+ messages in thread
From: Masahiro Yamada @ 2023-12-15 17:02 UTC (permalink / raw)
  To: Leonardo Bras
  Cc: Christoph Hellwig, Randy Dunlap, Nicolas Schier,
	Nathan Chancellor, Nick Desaulniers, Mauro Carvalho Chehab,
	linux-kernel, linux-kbuild

On Wed, Dec 13, 2023 at 2:10 AM Leonardo Bras <leobras@redhat.com> wrote:
>
> On Tue, Dec 12, 2023 at 05:08:34AM -0800, Christoph Hellwig wrote:
> > On Tue, Dec 12, 2023 at 05:09:21PM +0900, Masahiro Yamada wrote:
> > > Unlike .gitignore, this feature is opt-in rather than enforced.
> > >
> > > To use this, you need to run
> > >
> > > 'git config diff.orderFile scripts/git.orderFile'
> > >
> > > or
> > >
> > > 'git diff -C scripts/git.orderFile'
> >
> > Oh, ok.  That greatly reduces my concern.
>
> Yes, it's an opt-in, so no user should be directly impacted.


Applied to linux-kbuild.
Thanks.








> >
> > >
> > > Indeed, the file order is subjective, leaving
> > > us a question "do we need it in upstream"?
>
> The main idea is patch generation.
> This file's order is supposed to be the best order for reading a raw patch
> and understanding the code changes.
>
> > >
> > > At least, it is harmless for people who have no interest.
> >
> > .. but this is still a good question.  I'm not really sure there is
> > much of a need for it, but as long as it doesn't harm everyone else
> > I'm at least neutral on it.
>
> diff.orderfile was introduced in git to help order the git diff, and thus
> the patch generation, in a way that it's easier to understand what the
> commit / patch intends on doing.
>
> Take this example introducing a feature foo, you should see:
> - Documentation on foo, if introduced
> - How is foo enabled in build system, if needed
> - The types / stucts / fields introduced by foo, if any
> - The interface for using foo, if any
> - The actual foo implementation.
>
> Of course the actual order is open to discussion, and I encourage everyone
> to suggest any other items or order.
>
> Thanks!
> Leo
>
>


-- 
Best Regards
Masahiro Yamada

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [RFC PATCH v5 1/1] scripts: Introduce a default git.orderFile
  2023-12-15 17:02         ` Masahiro Yamada
@ 2023-12-15 18:30           ` Leonardo Bras Soares Passos
  0 siblings, 0 replies; 13+ messages in thread
From: Leonardo Bras Soares Passos @ 2023-12-15 18:30 UTC (permalink / raw)
  To: Masahiro Yamada
  Cc: Christoph Hellwig, Randy Dunlap, Nicolas Schier,
	Nathan Chancellor, Nick Desaulniers, Mauro Carvalho Chehab,
	linux-kernel, linux-kbuild

On Fri, Dec 15, 2023 at 2:03 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> On Wed, Dec 13, 2023 at 2:10 AM Leonardo Bras <leobras@redhat.com> wrote:
> >
> > On Tue, Dec 12, 2023 at 05:08:34AM -0800, Christoph Hellwig wrote:
> > > On Tue, Dec 12, 2023 at 05:09:21PM +0900, Masahiro Yamada wrote:
> > > > Unlike .gitignore, this feature is opt-in rather than enforced.
> > > >
> > > > To use this, you need to run
> > > >
> > > > 'git config diff.orderFile scripts/git.orderFile'
> > > >
> > > > or
> > > >
> > > > 'git diff -C scripts/git.orderFile'
> > >
> > > Oh, ok.  That greatly reduces my concern.
> >
> > Yes, it's an opt-in, so no user should be directly impacted.
>
>
> Applied to linux-kbuild.
> Thanks.

Thank you!



>
>
>
>
>
>
> > >
> > > >
> > > > Indeed, the file order is subjective, leaving
> > > > us a question "do we need it in upstream"?
> >
> > The main idea is patch generation.
> > This file's order is supposed to be the best order for reading a raw patch
> > and understanding the code changes.
> >
> > > >
> > > > At least, it is harmless for people who have no interest.
> > >
> > > .. but this is still a good question.  I'm not really sure there is
> > > much of a need for it, but as long as it doesn't harm everyone else
> > > I'm at least neutral on it.
> >
> > diff.orderfile was introduced in git to help order the git diff, and thus
> > the patch generation, in a way that it's easier to understand what the
> > commit / patch intends on doing.
> >
> > Take this example introducing a feature foo, you should see:
> > - Documentation on foo, if introduced
> > - How is foo enabled in build system, if needed
> > - The types / stucts / fields introduced by foo, if any
> > - The interface for using foo, if any
> > - The actual foo implementation.
> >
> > Of course the actual order is open to discussion, and I encourage everyone
> > to suggest any other items or order.
> >
> > Thanks!
> > Leo
> >
> >
>
>
> --
> Best Regards
> Masahiro Yamada
>


^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2023-12-15 18:30 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-12-08 18:18 [RFC PATCH v5 1/1] scripts: Introduce a default git.orderFile Leonardo Bras
2023-12-09 19:13 ` Masahiro Yamada
2023-12-11 13:13   ` lsoaresp
2023-12-11 13:17     ` Leonardo Bras Soares Passos
2023-12-11 18:05     ` Masahiro Yamada
2023-12-11 22:03       ` lsoaresp
2023-12-11 22:41         ` leobras
2023-12-12  7:31 ` Christoph Hellwig
2023-12-12  8:09   ` Masahiro Yamada
2023-12-12 13:08     ` Christoph Hellwig
2023-12-12 17:09       ` Leonardo Bras
2023-12-15 17:02         ` Masahiro Yamada
2023-12-15 18:30           ` Leonardo Bras Soares Passos

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.