[PATCH] notes: update documentation for `use_default_notes`

[PATCH] notes: update documentation for `use_default_notes`

[Kristoffer Haugsbakk]

`suppress_default_notes` was renamed to `use_default_notes` in
3a03cf6b1d (notes: refactor display notes default handling,
2011-03-29).

The commit message says that “values less than one [indicates] “not
set” ”, but I think what was meant was “less than zero”.

Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---

Notes (series):
    See also: https://lore.kernel.org/git/cover.1685441207.git.code@khaugsbakk.name
    
    Found while working on that. But this change could be posted
    separately.

 notes.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/notes.h b/notes.h
index c1682c39a9..69b6120146 100644
--- a/notes.h
+++ b/notes.h
@@ -286,8 +286,9 @@ void disable_display_notes(struct display_notes_opt *opt, int *show_notes);
  * If 'opt' is not NULL, then it specifies additional settings for the
  * displaying:
  *
- * - suppress_default_notes indicates that the notes from
- *   core.notesRef and notes.displayRef should not be loaded.
+ * - use_default_notes: less than `0` is "unset", which means that the
+ *   deafult refs are show iff no other notes are given. Else treat like
+ *   a boolean.
  *
  * - extra_notes_refs may contain a list of globs (in the same style
  *   as notes.displayRef) where notes should be loaded from.
-- 
2.41.0.rc2

[PATCH v2] notes: update documentation for `use_default_notes`

[Kristoffer Haugsbakk]

`suppress_default_notes` was renamed to `use_default_notes` in
3a03cf6b1d (notes: refactor display notes default handling,
2011-03-29).

The commit message says that “values less than one [indicates] “not
set” ”, but I think what was meant was “less than zero”.

Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---

Notes (series):
    § v2
    
    • Fix sloppy typos (sorry about that)
    • “default notes” instead of “default refs”
    • Don’t drop “it” (“treat it like”)

 notes.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/notes.h b/notes.h
index c1682c39a9..826a2595fa 100644
--- a/notes.h
+++ b/notes.h
@@ -286,8 +286,9 @@ void disable_display_notes(struct display_notes_opt *opt, int *show_notes);
  * If 'opt' is not NULL, then it specifies additional settings for the
  * displaying:
  *
- * - suppress_default_notes indicates that the notes from
- *   core.notesRef and notes.displayRef should not be loaded.
+ * - use_default_notes: less than `0` is "unset", which means that the
+ *   default notes are shown iff no other notes are given. Else treat it
+ *   like a boolean.
  *
  * - extra_notes_refs may contain a list of globs (in the same style
  *   as notes.displayRef) where notes should be loaded from.
-- 
2.41.0.rc2

Re: [PATCH v2] notes: update documentation for `use_default_notes`

[Jeff King]

On Wed, May 31, 2023 at 02:14:21PM +0200, Kristoffer Haugsbakk wrote:

> `suppress_default_notes` was renamed to `use_default_notes` in
> 3a03cf6b1d (notes: refactor display notes default handling,
> 2011-03-29).
> 
> The commit message says that “values less than one [indicates] “not
> set” ”, but I think what was meant was “less than zero”.

Yes, I think you're right. Thanks for catching my error, however
belatedly. ;)

> diff --git a/notes.h b/notes.h
> index c1682c39a9..826a2595fa 100644
> --- a/notes.h
> +++ b/notes.h
> @@ -286,8 +286,9 @@ void disable_display_notes(struct display_notes_opt *opt, int *show_notes);
>   * If 'opt' is not NULL, then it specifies additional settings for the
>   * displaying:
>   *
> - * - suppress_default_notes indicates that the notes from
> - *   core.notesRef and notes.displayRef should not be loaded.
> + * - use_default_notes: less than `0` is "unset", which means that the
> + *   default notes are shown iff no other notes are given. Else treat it
> + *   like a boolean.
>   *
>   * - extra_notes_refs may contain a list of globs (in the same style
>   *   as notes.displayRef) where notes should be loaded from.

This looks good to me. Arguably these should be documented in the
definition of "struct display_notes_opt", which would have avoided the
problem (3a03cf6b1d would not have forgotten to update the name since it
would be right there already). But it may not be worth the trouble to
move them now.

-Peff

Re: [PATCH v2] notes: update documentation for `use_default_notes`

[Kristoffer Haugsbakk]

Hi Peff

On Thu, Jun 1, 2023, at 19:52, Jeff King wrote:
> This looks good to me. Arguably these should be documented in the
> definition of "struct display_notes_opt", which would have avoided the
> problem (3a03cf6b1d would not have forgotten to update the name since it
> would be right there already). But it may not be worth the trouble to
> move them now.

Sounds like a good change to me. I’ll make a new version tomorrow unless
someone disagrees about that before that time.

-- 
Kristoffer Haugsbakk

[PATCH v3 0/2] notes: update documentation for `use_default_notes`

[Kristoffer Haugsbakk]

While I was working on [1] I found that the doc for the struct
`display_notes_opt` hadn’t been updated when one of the members were
renamed. I chose to post a separate patch/series.

[1] https://lore.kernel.org/git/cover.1685441207.git.code@khaugsbakk.name

§ Changes in v3 by patch

• 1: change initial “else” to “otherwise”
• 1. tweak commit message
• 2: New: move documentation to struct definition

Kristoffer Haugsbakk (2):
  notes: update documentation for `use_default_notes`
  notes: move the documentation to the struct

 notes.h | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

Range-diff against v2:
1:  3c757bcb3d ! 1:  3eaa725d3a notes: update documentation for `use_default_notes`
    @@ Commit message
         2011-03-29).

         The commit message says that “values less than one [indicates] “not
    -    set” ”, but I think what was meant was “less than zero”.
    +    set” ”, but what was meant was probably “less than zero” (the author of
    +    3a03cf6b1d agrees on this point).

         Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>

    -
    - ## Notes (series) ##
    -    § v2
    -
    -    • Fix sloppy typos (sorry about that)
    -    • “default notes” instead of “default refs”
    -    • Don’t drop “it” (“treat it like”)
    -
      ## notes.h ##
     @@ notes.h: void disable_display_notes(struct display_notes_opt *opt, int *show_notes);
       * If 'opt' is not NULL, then it specifies additional settings for the
    @@ notes.h: void disable_display_notes(struct display_notes_opt *opt, int *show_not
     - * - suppress_default_notes indicates that the notes from
     - *   core.notesRef and notes.displayRef should not be loaded.
     + * - use_default_notes: less than `0` is "unset", which means that the
    -+ *   default notes are shown iff no other notes are given. Else treat it
    -+ *   like a boolean.
    ++ *   default notes are shown iff no other notes are given. Otherwise,
    ++ *   treat it like a boolean.
       *
       * - extra_notes_refs may contain a list of globs (in the same style
       *   as notes.displayRef) where notes should be loaded from.
-:  ---------- > 2:  99c88c74ce notes: move the documentation to the struct
--
2.41.0

[PATCH v3 1/2] notes: update documentation for `use_default_notes`

[Kristoffer Haugsbakk]

`suppress_default_notes` was renamed to `use_default_notes` in
3a03cf6b1d (notes: refactor display notes default handling,
2011-03-29).

The commit message says that “values less than one [indicates] “not
set” ”, but what was meant was probably “less than zero” (the author of
3a03cf6b1d agrees on this point).

Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
 notes.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/notes.h b/notes.h
index c1682c39a9..a823840e49 100644
--- a/notes.h
+++ b/notes.h
@@ -286,8 +286,9 @@ void disable_display_notes(struct display_notes_opt *opt, int *show_notes);
  * If 'opt' is not NULL, then it specifies additional settings for the
  * displaying:
  *
- * - suppress_default_notes indicates that the notes from
- *   core.notesRef and notes.displayRef should not be loaded.
+ * - use_default_notes: less than `0` is "unset", which means that the
+ *   default notes are shown iff no other notes are given. Otherwise,
+ *   treat it like a boolean.
  *
  * - extra_notes_refs may contain a list of globs (in the same style
  *   as notes.displayRef) where notes should be loaded from.
-- 
2.41.0

[PATCH v3 2/2] notes: move the documentation to the struct

[Kristoffer Haugsbakk]

Its better to document the struct members on the struct definition
instead of on a function that takes a pointer to such a struct. This
will also make it easier to update the documentation in the future.

Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---

Notes (series):
    Suggested in: https://lore.kernel.org/git/20230601175218.GB4165405@coredump.intra.peff.net/

 notes.h | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/notes.h b/notes.h
index a823840e49..c63b275d7a 100644
--- a/notes.h
+++ b/notes.h
@@ -255,6 +255,14 @@ void free_notes(struct notes_tree *t);
 
 struct string_list;
 
+/*
+ * - use_default_notes: less than `0` is "unset", which means that the
+ *   default notes are shown iff no other notes are given. Otherwise,
+ *   treat it like a boolean.
+ *
+ * - extra_notes_refs may contain a list of globs (in the same style
+ *   as notes.displayRef) where notes should be loaded from.
+ */
 struct display_notes_opt {
 	int use_default_notes;
 	struct string_list extra_notes_refs;
@@ -283,15 +291,7 @@ void disable_display_notes(struct display_notes_opt *opt, int *show_notes);
 /*
  * Load the notes machinery for displaying several notes trees.
  *
- * If 'opt' is not NULL, then it specifies additional settings for the
- * displaying:
- *
- * - use_default_notes: less than `0` is "unset", which means that the
- *   default notes are shown iff no other notes are given. Otherwise,
- *   treat it like a boolean.
- *
- * - extra_notes_refs may contain a list of globs (in the same style
- *   as notes.displayRef) where notes should be loaded from.
+ * 'opt' may be NULL.
  */
 void load_display_notes(struct display_notes_opt *opt);
 
-- 
2.41.0

Re: [PATCH v3 2/2] notes: move the documentation to the struct

[Jeff King]

On Sun, Jun 04, 2023 at 08:54:00PM +0200, Kristoffer Haugsbakk wrote:

> Its better to document the struct members on the struct definition
> instead of on a function that takes a pointer to such a struct. This
> will also make it easier to update the documentation in the future.

This is better, I think, but...

> +/*
> + * - use_default_notes: less than `0` is "unset", which means that the
> + *   default notes are shown iff no other notes are given. Otherwise,
> + *   treat it like a boolean.
> + *
> + * - extra_notes_refs may contain a list of globs (in the same style
> + *   as notes.displayRef) where notes should be loaded from.
> + */
>  struct display_notes_opt {
>  	int use_default_notes;
>  	struct string_list extra_notes_refs;

...what I had meant to suggest was putting the documentation next to
each field, like:

  struct foo {
	/* when set, enable "bar" for all frotz's */
	int use_bar;

	/* etc... */
  };

For an example, try "struct bloom_filter_settings" in bloom.h (though
there are many others, too).

-Peff




> @@ -283,15 +291,7 @@ void disable_display_notes(struct display_notes_opt *opt, int *show_notes);
>  /*
>   * Load the notes machinery for displaying several notes trees.
>   *
> - * If 'opt' is not NULL, then it specifies additional settings for the
> - * displaying:
> - *
> - * - use_default_notes: less than `0` is "unset", which means that the
> - *   default notes are shown iff no other notes are given. Otherwise,
> - *   treat it like a boolean.
> - *
> - * - extra_notes_refs may contain a list of globs (in the same style
> - *   as notes.displayRef) where notes should be loaded from.
> + * 'opt' may be NULL.
>   */
>  void load_display_notes(struct display_notes_opt *opt);
>  
> -- 
> 2.41.0
> 

Re: [PATCH v3 2/2] notes: move the documentation to the struct

[Kristoffer Haugsbakk]

On Mon, Jun 5, 2023, at 06:50, Jeff King wrote:
> This is better, I think, but...
>
> [snip]
>
> ...what I had meant to suggest was putting the documentation next to
> each field, like:

That make even more sense. :)

-- 
Kristoffer Haugsbakk

[PATCH v4 0/2] notes: update documentation for `use_default_notes`

[Kristoffer Haugsbakk]

While I was working on [1] I found that the doc for the struct
`display_notes_opt` hadn’t been updated when one of the members were
renamed. I chose to post a separate patch/series.

[1] https://lore.kernel.org/git/cover.1685441207.git.code@khaugsbakk.name

§ Changes in v4 by patch

• 2: Document the struct members directly

Kristoffer Haugsbakk (2):
  notes: update documentation for `use_default_notes`
  notes: move the documentation to the struct

 notes.h | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

Range-diff against v3:
-:  ---------- > 1:  3eaa725d3a notes: update documentation for `use_default_notes`
1:  99c88c74ce ! 2:  ba147707ef notes: move the documentation to the struct
    @@ Metadata
      ## Commit message ##
         notes: move the documentation to the struct

    -    Its better to document the struct members on the struct definition
    -    instead of on a function that takes a pointer to such a struct. This
    -    will also make it easier to update the documentation in the future.
    +    Its better to document the struct members directly instead of on a
    +    function that takes a pointer to the struct. This will also make it
    +    easier to update the documentation in the future.
    +
    +    Make adjustments for this new context. Also drop “may contain” since we
    +    don’t need to emphasize that a list could be empty.

         Suggested-by: Jeff King <peff@peff.net>
         Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>


      ## Notes (series) ##
    -    Suggested in: https://lore.kernel.org/git/20230601175218.GB4165405@coredump.intra.peff.net/
    +    v3: Suggested in: https://lore.kernel.org/git/20230601175218.GB4165405@coredump.intra.peff.net/
    +    v4:
    +        • Put docs on each member instead
    +        • Update commit message accordingly
    +        • Drop “may contain” since we don’t have to emphasize that a list
    +          could be empty
    +        • Formatting is based on clang-format and `bloom_filter_settings` in
    +          `bloom.h`

      ## notes.h ##
     @@ notes.h: void free_notes(struct notes_tree *t);
    -
      struct string_list;

    -+/*
    -+ * - use_default_notes: less than `0` is "unset", which means that the
    -+ *   default notes are shown iff no other notes are given. Otherwise,
    -+ *   treat it like a boolean.
    -+ *
    -+ * - extra_notes_refs may contain a list of globs (in the same style
    -+ *   as notes.displayRef) where notes should be loaded from.
    -+ */
      struct display_notes_opt {
    ++	/*
    ++	 * Less than `0` is "unset", which means that the default notes
    ++	 * are shown iff no other notes are given. Otherwise,
    ++	 * treat it like a boolean.
    ++	 */
      	int use_default_notes;
    ++
    ++	/*
    ++	 * A list of globs (in the same style as notes.displayRef) where
    ++	 * notes should be loaded from.
    ++	 */
      	struct string_list extra_notes_refs;
    + };
    +
     @@ notes.h: void disable_display_notes(struct display_notes_opt *opt, int *show_notes);
      /*
       * Load the notes machinery for displaying several notes trees.
--
2.41.0

[PATCH v4 1/2] notes: update documentation for `use_default_notes`

[Kristoffer Haugsbakk]

`suppress_default_notes` was renamed to `use_default_notes` in
3a03cf6b1d (notes: refactor display notes default handling,
2011-03-29).

The commit message says that “values less than one [indicates] “not
set” ”, but what was meant was probably “less than zero” (the author of
3a03cf6b1d agrees on this point).

Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
 notes.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/notes.h b/notes.h
index c1682c39a9..a823840e49 100644
--- a/notes.h
+++ b/notes.h
@@ -286,8 +286,9 @@ void disable_display_notes(struct display_notes_opt *opt, int *show_notes);
  * If 'opt' is not NULL, then it specifies additional settings for the
  * displaying:
  *
- * - suppress_default_notes indicates that the notes from
- *   core.notesRef and notes.displayRef should not be loaded.
+ * - use_default_notes: less than `0` is "unset", which means that the
+ *   default notes are shown iff no other notes are given. Otherwise,
+ *   treat it like a boolean.
  *
  * - extra_notes_refs may contain a list of globs (in the same style
  *   as notes.displayRef) where notes should be loaded from.
-- 
2.41.0

[PATCH v4 2/2] notes: move the documentation to the struct

[Kristoffer Haugsbakk]

Its better to document the struct members directly instead of on a
function that takes a pointer to the struct. This will also make it
easier to update the documentation in the future.

Make adjustments for this new context. Also drop “may contain” since we
don’t need to emphasize that a list could be empty.

Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---

Notes (series):
    v3: Suggested in: https://lore.kernel.org/git/20230601175218.GB4165405@coredump.intra.peff.net/
    v4:
        • Put docs on each member instead
        • Update commit message accordingly
        • Drop “may contain” since we don’t have to emphasize that a list
          could be empty
        • Formatting is based on clang-format and `bloom_filter_settings` in
          `bloom.h`

 notes.h | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/notes.h b/notes.h
index a823840e49..064fd7143a 100644
--- a/notes.h
+++ b/notes.h
@@ -256,7 +256,17 @@ void free_notes(struct notes_tree *t);
 struct string_list;
 
 struct display_notes_opt {
+	/*
+	 * Less than `0` is "unset", which means that the default notes
+	 * are shown iff no other notes are given. Otherwise,
+	 * treat it like a boolean.
+	 */
 	int use_default_notes;
+
+	/*
+	 * A list of globs (in the same style as notes.displayRef) where
+	 * notes should be loaded from.
+	 */
 	struct string_list extra_notes_refs;
 };
 
@@ -283,15 +293,7 @@ void disable_display_notes(struct display_notes_opt *opt, int *show_notes);
 /*
  * Load the notes machinery for displaying several notes trees.
  *
- * If 'opt' is not NULL, then it specifies additional settings for the
- * displaying:
- *
- * - use_default_notes: less than `0` is "unset", which means that the
- *   default notes are shown iff no other notes are given. Otherwise,
- *   treat it like a boolean.
- *
- * - extra_notes_refs may contain a list of globs (in the same style
- *   as notes.displayRef) where notes should be loaded from.
+ * 'opt' may be NULL.
  */
 void load_display_notes(struct display_notes_opt *opt);
 
-- 
2.41.0

Re: [PATCH v4 0/2] notes: update documentation for `use_default_notes`

[Jeff King]

On Mon, Jun 05, 2023 at 12:05:21PM +0200, Kristoffer Haugsbakk wrote:

> While I was working on [1] I found that the doc for the struct
> `display_notes_opt` hadn’t been updated when one of the members were
> renamed. I chose to post a separate patch/series.
> 
> [1] https://lore.kernel.org/git/cover.1685441207.git.code@khaugsbakk.name
> 
> § Changes in v4 by patch
> 
> • 2: Document the struct members directly

Thanks, this version looks great to me!

-Peff