Dear Jonathan,
this v2 series addresses all review feedback of the patch v1 here:
https://lore.kernel.org/linux-doc/[email protected]/
Immediate actionable review feedback was:
from Jani Nikula:
- turn categories into subheadings
- use common heading adornment
- change to bullet or autonumbered lists
- propose those changes as separate additional patches
from Randy Dunlap:
- if subheadings, drop the colons at the end.
- acked change to test with linux-next
- Stephen Rothwell requested item 1 to stay item 1.
- pointed out swapping the config names in the commit message.
v1 -> v2:
The commit message of patch 1/3 is improved addressing Randy's
feedback on the commit message.
The diff itself of patch 1/3 is unchanged.
Patch 2/3 and 3/3 addresses Jani's and Randy's feedback.
The extended discussion and feedback was:
- Is the checkstack script worth mentioning or can it be replaced?
- missing some nowadays more important points.
- consider getting it coherent with submitting-patches.rst
I have put the extended feedback onto my todo list; for the next
iteration on this document---after cleaning up submitting-patches and
making the howto and submitting-patches more coherent.
I followed Jani's request and created three patches, this might help
in the next/final review---if any further review happens now.
However, I do not think the kernel repository needs to be swamped with
three patches for this 'logically one change' to a single document. So,
I also squashed the three patches back into one patch, sent out as
PATCH v2-squashed:
https://lore.kernel.org/linux-doc/[email protected]/
Please either pick this patch series or just the PATCH v2-squashed as
you see fit.
Lukas Bulwahn (3):
docs: submit-checklist: structure by category
docs: submit-checklist: use subheadings
docs: submit-checklist: change to autonumbered lists
Documentation/process/submit-checklist.rst | 163 +++++++++++----------
1 file changed, 88 insertions(+), 75 deletions(-)
--
2.43.2
While going through the submit checklist, the list order seemed rather
random, probably just by historical coincidences of always adding yet the
next point someone thought of at the end of the list.
Structure and order them by the category of such activity,
reviewing, documenting, checking with tools, building and testing.
As the diff of the reordering is large:
Review code now includes previous points 1, 5 and 22.
Review Kconfig includes previous 6, 7 and 8.
Documenting includes previous 11, 15, 16, 17, 18 and 23.
Checking with tools includes previous 5, 9 and 10.
Building includes previous 2, 3, 20 and 24.
Testing includes previous 12, 13, 14, 19 and 21.
Previous point 4 (compile for ppc64) was merged into point 3 (build for
many architectures), as it was just a further note to cross-compiling.
Previous point 5 was split into one in review and one in checking
to have every previous point in the right category.
Point 11 was shortened, as building documentation is mentioned already
in Build your code, 1d.
A note that was presented visually much too aggressive in the HTML view was
turned into a simple "Note that..." sentence in the enumeration.
The recommendation to test with the -mm patchset (previous 21, now
testing, point 5) was updated to the current state of affairs to test with
a recent tag of linux-next.
Note that the previous first point still remains the first list even after
reordering. Randy confirmed that it was important to Stephen Rothwell to
keep 'include what you use' to be the first in the list.
While at it, replace the reference to the obsolete CONFIG_DEBUG_SLAB with
CONFIG_SLUB_DEBUG.
Reviewed-by: Randy Dunlap <[email protected]>
Tested-by: Randy Dunlap <[email protected]>
Signed-off-by: Lukas Bulwahn <[email protected]>
---
Documentation/process/submit-checklist.rst | 157 +++++++++++----------
1 file changed, 84 insertions(+), 73 deletions(-)
diff --git a/Documentation/process/submit-checklist.rst b/Documentation/process/submit-checklist.rst
index b1bc2d37bd0a..7d8dba942fe8 100644
--- a/Documentation/process/submit-checklist.rst
+++ b/Documentation/process/submit-checklist.rst
@@ -11,110 +11,121 @@ These are all above and beyond the documentation that is provided in
and elsewhere regarding submitting Linux kernel patches.
+*Review your code:*
+
1) If you use a facility then #include the file that defines/declares
that facility. Don't depend on other header files pulling in ones
that you use.
-2) Builds cleanly:
+2) Check your patch for general style as detailed in
+ :ref:`Documentation/process/coding-style.rst <codingstyle>`.
- a) with applicable or modified ``CONFIG`` options ``=y``, ``=m``, and
- ``=n``. No ``gcc`` warnings/errors, no linker warnings/errors.
+3) All memory barriers {e.g., ``barrier()``, ``rmb()``, ``wmb()``} need a
+ comment in the source code that explains the logic of what they are doing
+ and why.
- b) Passes ``allnoconfig``, ``allmodconfig``
- c) Builds successfully when using ``O=builddir``
+*Review Kconfig changes:*
- d) Any Documentation/ changes build successfully without new warnings/errors.
- Use ``make htmldocs`` or ``make pdfdocs`` to check the build and
- fix any issues.
+1) Any new or modified ``CONFIG`` options do not muck up the config menu and
+ default to off unless they meet the exception criteria documented in
+ ``Documentation/kbuild/kconfig-language.rst`` Menu attributes: default value.
-3) Builds on multiple CPU architectures by using local cross-compile tools
- or some other build farm.
+2) All new ``Kconfig`` options have help text.
-4) ppc64 is a good architecture for cross-compilation checking because it
- tends to use ``unsigned long`` for 64-bit quantities.
+3) Has been carefully reviewed with respect to relevant ``Kconfig``
+ combinations. This is very hard to get right with testing---brainpower
+ pays off here.
-5) Check your patch for general style as detailed in
- :ref:`Documentation/process/coding-style.rst <codingstyle>`.
- Check for trivial violations with the patch style checker prior to
- submission (``scripts/checkpatch.pl``).
- You should be able to justify all violations that remain in
- your patch.
+*Provide documentation:*
-6) Any new or modified ``CONFIG`` options do not muck up the config menu and
- default to off unless they meet the exception criteria documented in
- ``Documentation/kbuild/kconfig-language.rst`` Menu attributes: default value.
+1) Include :ref:`kernel-doc <kernel_doc>` to document global kernel APIs.
+ (Not required for static functions, but OK there also.)
-7) All new ``Kconfig`` options have help text.
+2) All new ``/proc`` entries are documented under ``Documentation/``
-8) Has been carefully reviewed with respect to relevant ``Kconfig``
- combinations. This is very hard to get right with testing -- brainpower
- pays off here.
+3) All new kernel boot parameters are documented in
+ ``Documentation/admin-guide/kernel-parameters.rst``.
+
+4) All new module parameters are documented with ``MODULE_PARM_DESC()``
-9) Check cleanly with sparse.
+5) All new userspace interfaces are documented in ``Documentation/ABI/``.
+ See ``Documentation/ABI/README`` for more information.
+ Patches that change userspace interfaces should be CCed to
+ [email protected].
-10) Use ``make checkstack`` and fix any problems that it finds.
+6) If any ioctl's are added by the patch, then also update
+ ``Documentation/userspace-api/ioctl/ioctl-number.rst``.
- .. note::
- ``checkstack`` does not point out problems explicitly,
- but any one function that uses more than 512 bytes on the stack is a
- candidate for change.
+*Check your code with tools:*
-11) Include :ref:`kernel-doc <kernel_doc>` to document global kernel APIs.
- (Not required for static functions, but OK there also.) Use
- ``make htmldocs`` or ``make pdfdocs`` to check the
- :ref:`kernel-doc <kernel_doc>` and fix any issues.
+1) Check for trivial violations with the patch style checker prior to
+ submission (``scripts/checkpatch.pl``).
+ You should be able to justify all violations that remain in
+ your patch.
-12) Has been tested with ``CONFIG_PREEMPT``, ``CONFIG_DEBUG_PREEMPT``,
- ``CONFIG_DEBUG_SLAB``, ``CONFIG_DEBUG_PAGEALLOC``, ``CONFIG_DEBUG_MUTEXES``,
- ``CONFIG_DEBUG_SPINLOCK``, ``CONFIG_DEBUG_ATOMIC_SLEEP``,
- ``CONFIG_PROVE_RCU`` and ``CONFIG_DEBUG_OBJECTS_RCU_HEAD`` all
- simultaneously enabled.
+2) Check cleanly with sparse.
-13) Has been build- and runtime tested with and without ``CONFIG_SMP`` and
- ``CONFIG_PREEMPT.``
+3) Use ``make checkstack`` and fix any problems that it finds.
+ Note that ``checkstack`` does not point out problems explicitly,
+ but any one function that uses more than 512 bytes on the stack is a
+ candidate for change.
-14) All codepaths have been exercised with all lockdep features enabled.
-15) All new ``/proc`` entries are documented under ``Documentation/``
+*Build your code:*
+
+1) Builds cleanly:
+
+ a) with applicable or modified ``CONFIG`` options ``=y``, ``=m``, and
+ ``=n``. No ``gcc`` warnings/errors, no linker warnings/errors.
+
+ b) Passes ``allnoconfig``, ``allmodconfig``
+
+ c) Builds successfully when using ``O=builddir``
+
+ d) Any Documentation/ changes build successfully without new warnings/errors.
+ Use ``make htmldocs`` or ``make pdfdocs`` to check the build and
+ fix any issues.
-16) All new kernel boot parameters are documented in
- ``Documentation/admin-guide/kernel-parameters.rst``.
+2) Builds on multiple CPU architectures by using local cross-compile tools
+ or some other build farm. Note that ppc64 is a good architecture for
+ cross-compilation checking because it tends to use ``unsigned long`` for
+ 64-bit quantities.
-17) All new module parameters are documented with ``MODULE_PARM_DESC()``
+3) Newly-added code has been compiled with ``gcc -W`` (use
+ ``make KCFLAGS=-W``). This will generate lots of noise, but is good
+ for finding bugs like "warning: comparison between signed and unsigned".
-18) All new userspace interfaces are documented in ``Documentation/ABI/``.
- See ``Documentation/ABI/README`` for more information.
- Patches that change userspace interfaces should be CCed to
- [email protected].
+4) If your modified source code depends on or uses any of the kernel
+ APIs or features that are related to the following ``Kconfig`` symbols,
+ then test multiple builds with the related ``Kconfig`` symbols disabled
+ and/or ``=m`` (if that option is available) [not all of these at the
+ same time, just various/random combinations of them]:
-19) Has been checked with injection of at least slab and page-allocation
- failures. See ``Documentation/fault-injection/``.
+ ``CONFIG_SMP``, ``CONFIG_SYSFS``, ``CONFIG_PROC_FS``, ``CONFIG_INPUT``,
+ ``CONFIG_PCI``, ``CONFIG_BLOCK``, ``CONFIG_PM``, ``CONFIG_MAGIC_SYSRQ``,
+ ``CONFIG_NET``, ``CONFIG_INET=n`` (but latter with ``CONFIG_NET=y``).
- If the new code is substantial, addition of subsystem-specific fault
- injection might be appropriate.
-20) Newly-added code has been compiled with ``gcc -W`` (use
- ``make KCFLAGS=-W``). This will generate lots of noise, but is good
- for finding bugs like "warning: comparison between signed and unsigned".
+*Test your code:*
-21) Tested after it has been merged into the -mm patchset to make sure
- that it still works with all of the other queued patches and various
- changes in the VM, VFS, and other subsystems.
+1) Has been tested with ``CONFIG_PREEMPT``, ``CONFIG_DEBUG_PREEMPT``,
+ ``CONFIG_SLUB_DEBUG``, ``CONFIG_DEBUG_PAGEALLOC``, ``CONFIG_DEBUG_MUTEXES``,
+ ``CONFIG_DEBUG_SPINLOCK``, ``CONFIG_DEBUG_ATOMIC_SLEEP``,
+ ``CONFIG_PROVE_RCU`` and ``CONFIG_DEBUG_OBJECTS_RCU_HEAD`` all
+ simultaneously enabled.
-22) All memory barriers {e.g., ``barrier()``, ``rmb()``, ``wmb()``} need a
- comment in the source code that explains the logic of what they are doing
- and why.
+2) Has been build- and runtime tested with and without ``CONFIG_SMP`` and
+ ``CONFIG_PREEMPT.``
-23) If any ioctl's are added by the patch, then also update
- ``Documentation/userspace-api/ioctl/ioctl-number.rst``.
+3) All codepaths have been exercised with all lockdep features enabled.
-24) If your modified source code depends on or uses any of the kernel
- APIs or features that are related to the following ``Kconfig`` symbols,
- then test multiple builds with the related ``Kconfig`` symbols disabled
- and/or ``=m`` (if that option is available) [not all of these at the
- same time, just various/random combinations of them]:
+4) Has been checked with injection of at least slab and page-allocation
+ failures. See ``Documentation/fault-injection/``.
+ If the new code is substantial, addition of subsystem-specific fault
+ injection might be appropriate.
- ``CONFIG_SMP``, ``CONFIG_SYSFS``, ``CONFIG_PROC_FS``, ``CONFIG_INPUT``, ``CONFIG_PCI``, ``CONFIG_BLOCK``, ``CONFIG_PM``, ``CONFIG_MAGIC_SYSRQ``,
- ``CONFIG_NET``, ``CONFIG_INET=n`` (but latter with ``CONFIG_NET=y``).
+5) Tested with the most recent tag of linux-next to make sure that it still
+ works with all of the other queued patches and various changes in the VM,
+ VFS, and other subsystems.
--
2.43.2
During review (see Link), Jani Nikula suggested to use proper subheadings
instead of using italics to indicate the different new top-level
categories in the checklist. Further the top heading should follow the
common scheme.
Use subheadings. Adjust to common heading adornment.
Link: https://lore.kernel.org/linux-doc/[email protected]/
Signed-off-by: Lukas Bulwahn <[email protected]>
---
Documentation/process/submit-checklist.rst | 26 ++++++++++++----------
1 file changed, 14 insertions(+), 12 deletions(-)
diff --git a/Documentation/process/submit-checklist.rst b/Documentation/process/submit-checklist.rst
index 7d8dba942fe8..e531dd504b6c 100644
--- a/Documentation/process/submit-checklist.rst
+++ b/Documentation/process/submit-checklist.rst
@@ -1,7 +1,8 @@
.. _submitchecklist:
+=======================================
Linux Kernel patch submission checklist
-~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+=======================================
Here are some basic things that developers should do if they want to see their
kernel patch submissions accepted more quickly.
@@ -10,8 +11,8 @@ These are all above and beyond the documentation that is provided in
:ref:`Documentation/process/submitting-patches.rst <submittingpatches>`
and elsewhere regarding submitting Linux kernel patches.
-
-*Review your code:*
+Review your code
+================
1) If you use a facility then #include the file that defines/declares
that facility. Don't depend on other header files pulling in ones
@@ -24,8 +25,8 @@ and elsewhere regarding submitting Linux kernel patches.
comment in the source code that explains the logic of what they are doing
and why.
-
-*Review Kconfig changes:*
+Review Kconfig changes
+======================
1) Any new or modified ``CONFIG`` options do not muck up the config menu and
default to off unless they meet the exception criteria documented in
@@ -37,7 +38,8 @@ and elsewhere regarding submitting Linux kernel patches.
combinations. This is very hard to get right with testing---brainpower
pays off here.
-*Provide documentation:*
+Provide documentation
+=====================
1) Include :ref:`kernel-doc <kernel_doc>` to document global kernel APIs.
(Not required for static functions, but OK there also.)
@@ -57,8 +59,8 @@ and elsewhere regarding submitting Linux kernel patches.
6) If any ioctl's are added by the patch, then also update
``Documentation/userspace-api/ioctl/ioctl-number.rst``.
-
-*Check your code with tools:*
+Check your code with tools
+==========================
1) Check for trivial violations with the patch style checker prior to
submission (``scripts/checkpatch.pl``).
@@ -72,8 +74,8 @@ and elsewhere regarding submitting Linux kernel patches.
but any one function that uses more than 512 bytes on the stack is a
candidate for change.
-
-*Build your code:*
+Build your code
+===============
1) Builds cleanly:
@@ -107,8 +109,8 @@ and elsewhere regarding submitting Linux kernel patches.
``CONFIG_PCI``, ``CONFIG_BLOCK``, ``CONFIG_PM``, ``CONFIG_MAGIC_SYSRQ``,
``CONFIG_NET``, ``CONFIG_INET=n`` (but latter with ``CONFIG_NET=y``).
-
-*Test your code:*
+Test your code
+==============
1) Has been tested with ``CONFIG_PREEMPT``, ``CONFIG_DEBUG_PREEMPT``,
``CONFIG_SLUB_DEBUG``, ``CONFIG_DEBUG_PAGEALLOC``, ``CONFIG_DEBUG_MUTEXES``,
--
2.43.2
During review (see Link), Jani Nikula suggested to use autonumbered
lists instead of manually-maintained bullet numbering.
Turn all lists into autonumbered lists.
Link: https://lore.kernel.org/linux-doc/[email protected]/
Signed-off-by: Lukas Bulwahn <[email protected]>
---
Documentation/process/submit-checklist.rst | 48 +++++++++++-----------
1 file changed, 24 insertions(+), 24 deletions(-)
diff --git a/Documentation/process/submit-checklist.rst b/Documentation/process/submit-checklist.rst
index e531dd504b6c..c984b747a755 100644
--- a/Documentation/process/submit-checklist.rst
+++ b/Documentation/process/submit-checklist.rst
@@ -14,62 +14,62 @@ and elsewhere regarding submitting Linux kernel patches.
Review your code
================
-1) If you use a facility then #include the file that defines/declares
+#. If you use a facility then #include the file that defines/declares
that facility. Don't depend on other header files pulling in ones
that you use.
-2) Check your patch for general style as detailed in
+#. Check your patch for general style as detailed in
:ref:`Documentation/process/coding-style.rst <codingstyle>`.
-3) All memory barriers {e.g., ``barrier()``, ``rmb()``, ``wmb()``} need a
+#. All memory barriers {e.g., ``barrier()``, ``rmb()``, ``wmb()``} need a
comment in the source code that explains the logic of what they are doing
and why.
Review Kconfig changes
======================
-1) Any new or modified ``CONFIG`` options do not muck up the config menu and
+#. Any new or modified ``CONFIG`` options do not muck up the config menu and
default to off unless they meet the exception criteria documented in
``Documentation/kbuild/kconfig-language.rst`` Menu attributes: default value.
-2) All new ``Kconfig`` options have help text.
+#. All new ``Kconfig`` options have help text.
-3) Has been carefully reviewed with respect to relevant ``Kconfig``
+#. Has been carefully reviewed with respect to relevant ``Kconfig``
combinations. This is very hard to get right with testing---brainpower
pays off here.
Provide documentation
=====================
-1) Include :ref:`kernel-doc <kernel_doc>` to document global kernel APIs.
+#. Include :ref:`kernel-doc <kernel_doc>` to document global kernel APIs.
(Not required for static functions, but OK there also.)
-2) All new ``/proc`` entries are documented under ``Documentation/``
+#. All new ``/proc`` entries are documented under ``Documentation/``
-3) All new kernel boot parameters are documented in
+#. All new kernel boot parameters are documented in
``Documentation/admin-guide/kernel-parameters.rst``.
-4) All new module parameters are documented with ``MODULE_PARM_DESC()``
+#. All new module parameters are documented with ``MODULE_PARM_DESC()``
-5) All new userspace interfaces are documented in ``Documentation/ABI/``.
+#. All new userspace interfaces are documented in ``Documentation/ABI/``.
See ``Documentation/ABI/README`` for more information.
Patches that change userspace interfaces should be CCed to
[email protected].
-6) If any ioctl's are added by the patch, then also update
+#. If any ioctl's are added by the patch, then also update
``Documentation/userspace-api/ioctl/ioctl-number.rst``.
Check your code with tools
==========================
-1) Check for trivial violations with the patch style checker prior to
+#. Check for trivial violations with the patch style checker prior to
submission (``scripts/checkpatch.pl``).
You should be able to justify all violations that remain in
your patch.
-2) Check cleanly with sparse.
+#. Check cleanly with sparse.
-3) Use ``make checkstack`` and fix any problems that it finds.
+#. Use ``make checkstack`` and fix any problems that it finds.
Note that ``checkstack`` does not point out problems explicitly,
but any one function that uses more than 512 bytes on the stack is a
candidate for change.
@@ -77,7 +77,7 @@ Check your code with tools
Build your code
===============
-1) Builds cleanly:
+#. Builds cleanly:
a) with applicable or modified ``CONFIG`` options ``=y``, ``=m``, and
``=n``. No ``gcc`` warnings/errors, no linker warnings/errors.
@@ -90,16 +90,16 @@ Build your code
Use ``make htmldocs`` or ``make pdfdocs`` to check the build and
fix any issues.
-2) Builds on multiple CPU architectures by using local cross-compile tools
+#. Builds on multiple CPU architectures by using local cross-compile tools
or some other build farm. Note that ppc64 is a good architecture for
cross-compilation checking because it tends to use ``unsigned long`` for
64-bit quantities.
-3) Newly-added code has been compiled with ``gcc -W`` (use
+#. Newly-added code has been compiled with ``gcc -W`` (use
``make KCFLAGS=-W``). This will generate lots of noise, but is good
for finding bugs like "warning: comparison between signed and unsigned".
-4) If your modified source code depends on or uses any of the kernel
+#. If your modified source code depends on or uses any of the kernel
APIs or features that are related to the following ``Kconfig`` symbols,
then test multiple builds with the related ``Kconfig`` symbols disabled
and/or ``=m`` (if that option is available) [not all of these at the
@@ -112,22 +112,22 @@ Build your code
Test your code
==============
-1) Has been tested with ``CONFIG_PREEMPT``, ``CONFIG_DEBUG_PREEMPT``,
+#. Has been tested with ``CONFIG_PREEMPT``, ``CONFIG_DEBUG_PREEMPT``,
``CONFIG_SLUB_DEBUG``, ``CONFIG_DEBUG_PAGEALLOC``, ``CONFIG_DEBUG_MUTEXES``,
``CONFIG_DEBUG_SPINLOCK``, ``CONFIG_DEBUG_ATOMIC_SLEEP``,
``CONFIG_PROVE_RCU`` and ``CONFIG_DEBUG_OBJECTS_RCU_HEAD`` all
simultaneously enabled.
-2) Has been build- and runtime tested with and without ``CONFIG_SMP`` and
+#. Has been build- and runtime tested with and without ``CONFIG_SMP`` and
``CONFIG_PREEMPT.``
-3) All codepaths have been exercised with all lockdep features enabled.
+#. All codepaths have been exercised with all lockdep features enabled.
-4) Has been checked with injection of at least slab and page-allocation
+#. Has been checked with injection of at least slab and page-allocation
failures. See ``Documentation/fault-injection/``.
If the new code is substantial, addition of subsystem-specific fault
injection might be appropriate.
-5) Tested with the most recent tag of linux-next to make sure that it still
+#. Tested with the most recent tag of linux-next to make sure that it still
works with all of the other queued patches and various changes in the VM,
VFS, and other subsystems.
--
2.43.2
Hi Lukas,
I might be nitpicking too much, but let me go ahead...
On Thu, 29 Feb 2024 04:07:43 +0100, Lukas Bulwahn wrote:
> During review (see Link), Jani Nikula suggested to use autonumbered
> lists instead of manually-maintained bullet numbering.
>
> Turn all lists into autonumbered lists.
>
> Link: https://lore.kernel.org/linux-doc/[email protected]/
> Signed-off-by: Lukas Bulwahn <[email protected]>
> ---
> Documentation/process/submit-checklist.rst | 48 +++++++++++-----------
> 1 file changed, 24 insertions(+), 24 deletions(-)
>
> diff --git a/Documentation/process/submit-checklist.rst b/Documentation/process/submit-checklist.rst
> index e531dd504b6c..c984b747a755 100644
> --- a/Documentation/process/submit-checklist.rst
> +++ b/Documentation/process/submit-checklist.rst
> @@ -14,62 +14,62 @@ and elsewhere regarding submitting Linux kernel patches.
> Review your code
> ================
>
> -1) If you use a facility then #include the file that defines/declares
> +#. If you use a facility then #include the file that defines/declares
> that facility. Don't depend on other header files pulling in ones
> that you use.
Wait. This will render the list starting from:
1. If you use ...
In patch 1/1, you didn't change the ")".
It was Jani who suggested "#.", but "#)" would work just fine.
For details, see docutils' documentation at:
https://docutils.sourceforge.io/docs/ref/rst/restructuredtext.html#enumerated-lists
By the way, you should be able to use auto enumeration in the 2nd-level
list as well.
> @@ -77,7 +77,7 @@ Check your code with tools
> Build your code
> ===============
>
> -1) Builds cleanly:
> +#. Builds cleanly:
>
> a) with applicable or modified ``CONFIG`` options ``=y``, ``=m``, and
> ``=n``. No ``gcc`` warnings/errors, no linker warnings/errors.
While the first item needs "a)", subsequent items can use "#)".
Either way,
Reviewed-by: Akira Yokosawa <[email protected]>
Thanks, Akira
On 2/28/24 19:07, Lukas Bulwahn wrote:
> During review (see Link), Jani Nikula suggested to use autonumbered
> lists instead of manually-maintained bullet numbering.
>
> Turn all lists into autonumbered lists.
>
> Link: https://lore.kernel.org/linux-doc/[email protected]/
> Signed-off-by: Lukas Bulwahn <[email protected]>
> ---
> Documentation/process/submit-checklist.rst | 48 +++++++++++-----------
> 1 file changed, 24 insertions(+), 24 deletions(-)
>
Reviewed-by: Randy Dunlap <[email protected]>
thanks.
--
#Randy
On 2/28/24 19:07, Lukas Bulwahn wrote:
> During review (see Link), Jani Nikula suggested to use proper subheadings
> instead of using italics to indicate the different new top-level
> categories in the checklist. Further the top heading should follow the
> common scheme.
>
> Use subheadings. Adjust to common heading adornment.
>
> Link: https://lore.kernel.org/linux-doc/[email protected]/
> Signed-off-by: Lukas Bulwahn <[email protected]>
> ---
> Documentation/process/submit-checklist.rst | 26 ++++++++++++----------
> 1 file changed, 14 insertions(+), 12 deletions(-)
>
Reviewed-by: Randy Dunlap <[email protected]>
thanks.
--
#Randy
On Thu, Feb 29, 2024 at 11:25 AM Jani Nikula <[email protected]> wrote:
>
> On Thu, 29 Feb 2024, Lukas Bulwahn <[email protected]> wrote:
> > from Jani Nikula:
> > - turn categories into subheadings
> > - use common heading adornment
> > - change to bullet or autonumbered lists
> > - propose those changes as separate additional patches
>
> I was hoping these cleanups would've come *first*, but up to Jon to
> decide if that really matters.
>
I see, changing to autonumbered lists could be done before, but the
subheadings only exist once reordered.
I am still in favor of the squashed commit: we are talking about a
very small document that central content has not been touched for a
long time. It is not code, where someone would bisect into. So, the
history does not need to carry every single stylistic change as yet
another commit.
Jani, I can do what you are asking for---but it is certainly going to
take yet a few hours to have the commits exactly into that form of
stylistic changes.
Let us see what Jon thinks.
Lukas
On Thu, 29 Feb 2024, Lukas Bulwahn <[email protected]> wrote:
> from Jani Nikula:
> - turn categories into subheadings
> - use common heading adornment
> - change to bullet or autonumbered lists
> - propose those changes as separate additional patches
I was hoping these cleanups would've come *first*, but up to Jon to
decide if that really matters.
BR,
Jani.
--
Jani Nikula, Intel
Lukas Bulwahn <[email protected]> writes:
> Dear Jonathan,
>
> this v2 series addresses all review feedback of the patch v1 here:
>
> https://lore.kernel.org/linux-doc/[email protected]/
>
> Immediate actionable review feedback was:
>
> from Jani Nikula:
> - turn categories into subheadings
> - use common heading adornment
> - change to bullet or autonumbered lists
> - propose those changes as separate additional patches
>
> from Randy Dunlap:
> - if subheadings, drop the colons at the end.
> - acked change to test with linux-next
> - Stephen Rothwell requested item 1 to stay item 1.
> - pointed out swapping the config names in the commit message.
>
> v1 -> v2:
> The commit message of patch 1/3 is improved addressing Randy's
> feedback on the commit message.
> The diff itself of patch 1/3 is unchanged.
>
> Patch 2/3 and 3/3 addresses Jani's and Randy's feedback.
>
> The extended discussion and feedback was:
>
> - Is the checkstack script worth mentioning or can it be replaced?
> - missing some nowadays more important points.
> - consider getting it coherent with submitting-patches.rst
>
> I have put the extended feedback onto my todo list; for the next
> iteration on this document---after cleaning up submitting-patches and
> making the howto and submitting-patches more coherent.
>
> I followed Jani's request and created three patches, this might help
> in the next/final review---if any further review happens now.
>
> However, I do not think the kernel repository needs to be swamped with
> three patches for this 'logically one change' to a single document. So,
> I also squashed the three patches back into one patch, sent out as
> PATCH v2-squashed:
>
> https://lore.kernel.org/linux-doc/[email protected]/
>
> Please either pick this patch series or just the PATCH v2-squashed as
> you see fit.
>
> Lukas Bulwahn (3):
> docs: submit-checklist: structure by category
> docs: submit-checklist: use subheadings
> docs: submit-checklist: change to autonumbered lists
>
> Documentation/process/submit-checklist.rst | 163 +++++++++++----------
> 1 file changed, 88 insertions(+), 75 deletions(-)
So I've applied the first two patches, since there doesn't seem to be
any disagreement over those. Once we figure out how we want the
autonumbering to be done, that can go in as well.
Thanks,
jon
Akira Yokosawa <[email protected]> writes:
>> -1) If you use a facility then #include the file that defines/declares
>> +#. If you use a facility then #include the file that defines/declares
>> that facility. Don't depend on other header files pulling in ones
>> that you use.
>
> Wait. This will render the list starting from:
>
> 1. If you use ...
>
> In patch 1/1, you didn't change the ")".
>
> It was Jani who suggested "#.", but "#)" would work just fine.
So I'm a little confused. Is the objection that it renders the number
as "1." rather than "1)"? That doesn't seem like the biggest of deals,
somehow, but am I missing something?
A bigger complaint I might raise is that auto-numbering restarts the
enumeration in each subsection, so we have a lot of steps #1, which is a
definite change from before.
That, of course, can be fixed by giving an explicit starting number in
each subsection, partially defeating the point of the change in the
first place.
I honestly have to wonder: does this document need the enumerated list
at all? We don't refer to the numbers anywhere, so I don't think there
is much useful information there. How about just using regular bulleted
lists instead?
That said, I don't have strong feelings one way or the other, and can
certainly apply it as-is if that's the consensus on what we should do.
Thanks,
jon
On 3/3/24 07:55, Jonathan Corbet wrote:
> Akira Yokosawa <[email protected]> writes:
>
>>> -1) If you use a facility then #include the file that defines/declares
>>> +#. If you use a facility then #include the file that defines/declares
>>> that facility. Don't depend on other header files pulling in ones
>>> that you use.
>>
>> Wait. This will render the list starting from:
>>
>> 1. If you use ...
>>
I have already said that Stephen Rothwell wanted this #1 item to be at the
top of the checklist. That makes it easy to tell people to "see submit-checklist
item #1".
>> In patch 1/1, you didn't change the ")".
>>
>> It was Jani who suggested "#.", but "#)" would work just fine.
>
> So I'm a little confused. Is the objection that it renders the number
> as "1." rather than "1)"? That doesn't seem like the biggest of deals,
> somehow, but am I missing something?
>
> A bigger complaint I might raise is that auto-numbering restarts the
> enumeration in each subsection, so we have a lot of steps #1, which is a
> definite change from before.
ack
> That, of course, can be fixed by giving an explicit starting number in
> each subsection, partially defeating the point of the change in the
> first place.
ack
> I honestly have to wonder: does this document need the enumerated list
> at all? We don't refer to the numbers anywhere, so I don't think there
> is much useful information there. How about just using regular bulleted
> lists instead?
That also works.
> That said, I don't have strong feelings one way or the other, and can
> certainly apply it as-is if that's the consensus on what we should do.
My preference is to leave the submit-checklist numbered from 1 to N,
without a repeated #1 in each section. But I'm not hung up on it.
thanks.
--
#Randy
On Sun, 03 Mar 2024 08:55:51 -0700, Jonathan Corbet wrote:
> Akira Yokosawa <[email protected]> writes:
>
>>> -1) If you use a facility then #include the file that defines/declares
>>> +#. If you use a facility then #include the file that defines/declares
>>> that facility. Don't depend on other header files pulling in ones
>>> that you use.
>>
>> Wait. This will render the list starting from:
>>
>> 1. If you use ...
>>
>> In patch 1/1, you didn't change the ")".
>>
>> It was Jani who suggested "#.", but "#)" would work just fine.
>
> So I'm a little confused. Is the objection that it renders the number
> as "1." rather than "1)"? That doesn't seem like the biggest of deals,
> somehow, but am I missing something?
>
I said at the top of my reply:
> I might be nitpicking too much, but let me go ahead...
, and my point here was to let Lukas aware of the variation of auto-
numbering patterns. I don't object the change from "1." to "1)" if
that change is intended and explained in the changelog.
HTML builder recognizes "1)", but renders it as "1.", while
LATEX builder renders it as "1)".
> A bigger complaint I might raise is that auto-numbering restarts the
> enumeration in each subsection, so we have a lot of steps #1, which is a
> definite change from before.
+1
> That, of course, can be fixed by giving an explicit starting number in
> each subsection, partially defeating the point of the change in the
> first place.
Right.
>
> I honestly have to wonder: does this document need the enumerated list
> at all? We don't refer to the numbers anywhere, so I don't think there
> is much useful information there. How about just using regular bulleted
> lists instead?
That would make a lot of sense.
Auto-numbered enumerated lists look mostly the same as bulleted lists
in the source.
No strong opinion, but I'd respect Randy's preference of not applying
this change.
Thanks, Akira
>
> That said, I don't have strong feelings one way or the other, and can
> certainly apply it as-is if that's the consensus on what we should do.
>
> Thanks,
>
> jon