[PATCH 0/2] string_list API users: use alloc + init, not calloc + strdup_strings

[PATCH 0/2] string_list API users: use alloc + init, not calloc + strdup_strings

[Ævar Arnfjörð Bjarmason]

As a small follow-up to Junio's
https://lore.kernel.org/git/xmqq7d471dns.fsf@gitster.g/; This is a
small set of fixes to use the string_list functions rather than
peeking into its guts during initalization.

I've been running my local git version with these changes for almost a
year, but for submision I wrote a coccinelle rule to cover most of it
in 1/2, along with a tests using the new recently landed coccicheck
test support.

The 2/2 is then things I had to manually change still, which weren't
covered by the conservative rule.

Ævar Arnfjörð Bjarmason (2):
  string_list API users + cocci: use string_list_init_dup()
  string-list API users: manually use string_list_init_*()

 contrib/coccinelle/string_list.cocci     | 8 ++++++++
 contrib/coccinelle/tests/string_list.c   | 7 +++++++
 contrib/coccinelle/tests/string_list.res | 7 +++++++
 notes-utils.c                            | 4 ++--
 reflog-walk.c                            | 2 +-
 refs.c                                   | 4 ++--
 resolve-undo.c                           | 8 ++++----
 revision.c                               | 4 ++--
 8 files changed, 33 insertions(+), 11 deletions(-)
 create mode 100644 contrib/coccinelle/string_list.cocci
 create mode 100644 contrib/coccinelle/tests/string_list.c
 create mode 100644 contrib/coccinelle/tests/string_list.res

-- 
2.37.1.1095.g64a1e8362fd

[PATCH 1/2] string_list API users + cocci: use string_list_init_dup()

[Ævar Arnfjörð Bjarmason]

Add a coccinelle rule to detect a particular misuse of the "struct
string_list" API. We have the *_INIT macros, but this code assumed
that a zero'd out "struct string_list" with a "strdup_string" set
would be the same as string_list_init_dup().

That assumption happens to be right, but let's instead use the helper
functions introduced in 183113a5ca9 (string_list: Add STRING_LIST_INIT
macro and make use of it., 2010-07-04).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 contrib/coccinelle/string_list.cocci     | 8 ++++++++
 contrib/coccinelle/tests/string_list.c   | 7 +++++++
 contrib/coccinelle/tests/string_list.res | 7 +++++++
 refs.c                                   | 4 ++--
 resolve-undo.c                           | 8 ++++----
 revision.c                               | 4 ++--
 6 files changed, 30 insertions(+), 8 deletions(-)
 create mode 100644 contrib/coccinelle/string_list.cocci
 create mode 100644 contrib/coccinelle/tests/string_list.c
 create mode 100644 contrib/coccinelle/tests/string_list.res

diff --git a/contrib/coccinelle/string_list.cocci b/contrib/coccinelle/string_list.cocci
new file mode 100644
index 00000000000..5d285d5732c
--- /dev/null
+++ b/contrib/coccinelle/string_list.cocci
@@ -0,0 +1,8 @@
+@@
+struct string_list *P;
+@@
+- CALLOC_ARRAY(P, 1);
++ ALLOC_ARRAY(P, 1);
+... when != P
+- (P)->strdup_strings = 1;
++ string_list_init_dup(P);
diff --git a/contrib/coccinelle/tests/string_list.c b/contrib/coccinelle/tests/string_list.c
new file mode 100644
index 00000000000..e77822b7682
--- /dev/null
+++ b/contrib/coccinelle/tests/string_list.c
@@ -0,0 +1,7 @@
+int init(void)
+{
+	struct string_list *list;
+
+	CALLOC_ARRAY(list, 1);
+	list->strdup_strings = 1;
+}
diff --git a/contrib/coccinelle/tests/string_list.res b/contrib/coccinelle/tests/string_list.res
new file mode 100644
index 00000000000..7e666f5bf48
--- /dev/null
+++ b/contrib/coccinelle/tests/string_list.res
@@ -0,0 +1,7 @@
+int init(void)
+{
+	struct string_list *list;
+
+	ALLOC_ARRAY(list, 1);
+	string_list_init_dup(list);
+}
diff --git a/refs.c b/refs.c
index 90bcb271687..83151a42b3a 100644
--- a/refs.c
+++ b/refs.c
@@ -1313,8 +1313,8 @@ int parse_hide_refs_config(const char *var, const char *value, const char *secti
 		while (len && ref[len - 1] == '/')
 			ref[--len] = '\0';
 		if (!hide_refs) {
-			CALLOC_ARRAY(hide_refs, 1);
-			hide_refs->strdup_strings = 1;
+			ALLOC_ARRAY(hide_refs, 1);
+			string_list_init_dup(hide_refs);
 		}
 		string_list_append(hide_refs, ref);
 	}
diff --git a/resolve-undo.c b/resolve-undo.c
index e81096e2d45..e66b8306fe0 100644
--- a/resolve-undo.c
+++ b/resolve-undo.c
@@ -15,8 +15,8 @@ void record_resolve_undo(struct index_state *istate, struct cache_entry *ce)
 		return;
 
 	if (!istate->resolve_undo) {
-		CALLOC_ARRAY(resolve_undo, 1);
-		resolve_undo->strdup_strings = 1;
+		ALLOC_ARRAY(resolve_undo, 1);
+		string_list_init_dup(resolve_undo);
 		istate->resolve_undo = resolve_undo;
 	}
 	resolve_undo = istate->resolve_undo;
@@ -57,8 +57,8 @@ struct string_list *resolve_undo_read(const char *data, unsigned long size)
 	int i;
 	const unsigned rawsz = the_hash_algo->rawsz;
 
-	CALLOC_ARRAY(resolve_undo, 1);
-	resolve_undo->strdup_strings = 1;
+	ALLOC_ARRAY(resolve_undo, 1);
+	string_list_init_dup(resolve_undo);
 
 	while (size) {
 		struct string_list_item *lost;
diff --git a/revision.c b/revision.c
index 0c6e26cd9c8..e44af92cacc 100644
--- a/revision.c
+++ b/revision.c
@@ -1578,8 +1578,8 @@ void clear_ref_exclusion(struct string_list **ref_excludes_p)
 void add_ref_exclusion(struct string_list **ref_excludes_p, const char *exclude)
 {
 	if (!*ref_excludes_p) {
-		CALLOC_ARRAY(*ref_excludes_p, 1);
-		(*ref_excludes_p)->strdup_strings = 1;
+		ALLOC_ARRAY(*ref_excludes_p, 1);
+		string_list_init_dup(*ref_excludes_p);
 	}
 	string_list_append(*ref_excludes_p, exclude);
 }
-- 
2.37.1.1095.g64a1e8362fd

[PATCH 2/2] string-list API users: manually use string_list_init_*()

[Ævar Arnfjörð Bjarmason]

Convert various code that didn't use string_list_init_*() to do so, in
cases where the only thing being allocated was the string list we can
change from CALLOC_ARRAY() to ALLOC_ARRAY(), the string_list_init_*()
function will zero out the memory.

This covers cases that weren't matched by tho coccinelle rule in the
preceding commit, which is conservative enough to care about the type
of what we're modifying.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 notes-utils.c | 4 ++--
 reflog-walk.c | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/notes-utils.c b/notes-utils.c
index d7d18e30f5a..73559d24ec8 100644
--- a/notes-utils.c
+++ b/notes-utils.c
@@ -129,8 +129,8 @@ struct notes_rewrite_cfg *init_copy_notes_for_rewrite(const char *cmd)
 	c->cmd = cmd;
 	c->enabled = 1;
 	c->combine = combine_notes_concatenate;
-	CALLOC_ARRAY(c->refs, 1);
-	c->refs->strdup_strings = 1;
+	ALLOC_ARRAY(c->refs, 1);
+	string_list_init_dup(c->refs);
 	c->refs_from_env = 0;
 	c->mode_from_env = 0;
 	if (rewrite_mode_env) {
diff --git a/reflog-walk.c b/reflog-walk.c
index 7aa6595a51f..2b17408f9a4 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -120,7 +120,7 @@ struct reflog_walk_info {
 void init_reflog_walk(struct reflog_walk_info **info)
 {
 	CALLOC_ARRAY(*info, 1);
-	(*info)->complete_reflogs.strdup_strings = 1;
+	string_list_init_dup(&((*info)->complete_reflogs));
 }
 
 void reflog_walk_info_release(struct reflog_walk_info *info)
-- 
2.37.1.1095.g64a1e8362fd

Re: [PATCH 2/2] string-list API users: manually use string_list_init_*()

[Elijah Newren]

On Wed, Jul 20, 2022 at 11:39 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
> Convert various code that didn't use string_list_init_*() to do so, in
> cases where the only thing being allocated was the string list we can
> change from CALLOC_ARRAY() to ALLOC_ARRAY(), the string_list_init_*()
> function will zero out the memory.
>
> This covers cases that weren't matched by tho coccinelle rule in the

s/tho/the/ ?

> preceding commit, which is conservative enough to care about the type
> of what we're modifying.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  notes-utils.c | 4 ++--
>  reflog-walk.c | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/notes-utils.c b/notes-utils.c
> index d7d18e30f5a..73559d24ec8 100644
> --- a/notes-utils.c
> +++ b/notes-utils.c
> @@ -129,8 +129,8 @@ struct notes_rewrite_cfg *init_copy_notes_for_rewrite(const char *cmd)
>         c->cmd = cmd;
>         c->enabled = 1;
>         c->combine = combine_notes_concatenate;
> -       CALLOC_ARRAY(c->refs, 1);
> -       c->refs->strdup_strings = 1;
> +       ALLOC_ARRAY(c->refs, 1);
> +       string_list_init_dup(c->refs);

But c->refs is a struct string_list *; why doesn't cocci recognize
this one and convert it?

>         c->refs_from_env = 0;
>         c->mode_from_env = 0;
>         if (rewrite_mode_env) {
> diff --git a/reflog-walk.c b/reflog-walk.c
> index 7aa6595a51f..2b17408f9a4 100644
> --- a/reflog-walk.c
> +++ b/reflog-walk.c
> @@ -120,7 +120,7 @@ struct reflog_walk_info {
>  void init_reflog_walk(struct reflog_walk_info **info)
>  {
>         CALLOC_ARRAY(*info, 1);
> -       (*info)->complete_reflogs.strdup_strings = 1;
> +       string_list_init_dup(&((*info)->complete_reflogs));

Makes sense.

>  }
>
>  void reflog_walk_info_release(struct reflog_walk_info *info)
> --
> 2.37.1.1095.g64a1e8362fd
>

[PATCH v2 0/6] string-list API user: fix API use, some with coccinelle

[Ævar Arnfjörð Bjarmason]

A larger v2, this:

 * Renames string_list.cocci to string-list.cocci (to be consistent
   with the string-list.[ch])

 * Answers Elijah's question about why the previous 2/2 left out one
   case, it's due to --disable-worth-trying-opt. That's now split out
   into its own commit.

   FWIW it's the only case with "make coccicheck" that we've "missed",
   including existing rules, but we just got lucky there.

   I have some follow-up patches to fix various such bugs in our
   coccicheck, but let's leave the larger fixes for now.

 * The new 4/6 duplicates Junio's
   https://lore.kernel.org/git/xmqq7d471dns.fsf@gitster.g/, but now
   we've got a coccinelle rule for it and similar inverse cases. It's
   still the only in-tree case, but it's probably good to keep the
   rule around regardless.

   I could also eject that *.C change and say this is based on
   Junio's, or both could make the same change and "merge" will sort
   it out. I do think 4/6 is a bit easier to read with an actual case
   where we change things, but then again there's the added tests...

 * A new 5/6 tweaks some "strdup_strings" dancing to a simpler
   pattern, this is adjusted from my local WIP branch of larger
   string_list API fixes.

 * A new 6/6 fixes a confusing case of dup v.s. nodup to use a more
   obvious callback pattern where we don't flip-flop.

Ævar Arnfjörð Bjarmason (6):
  string_list API users + cocci: use string_list_init_dup()
  cocci: apply string_list.cocci with --disable-worth-trying-opt
  reflog-walk.c: use string_list_init_dup()
  cocci: add "string_list" rule to swap "DUP" <-> "NODUP"
  string-list API users: don't tweak "strdup_strings" to free dupes
  notes.c: make "struct string_list display_notes_refs" non-static

 bisect.c                                 |  7 +++---
 builtin/remote.c                         |  3 +--
 contrib/coccinelle/string-list.cocci     | 26 ++++++++++++++++++++++
 contrib/coccinelle/tests/string-list.c   | 20 +++++++++++++++++
 contrib/coccinelle/tests/string-list.res | 18 +++++++++++++++
 notes-utils.c                            |  4 ++--
 notes.c                                  | 28 ++++++++++++++----------
 reflog-walk.c                            |  2 +-
 refs.c                                   |  4 ++--
 resolve-undo.c                           |  8 +++----
 revision.c                               |  4 ++--
 11 files changed, 95 insertions(+), 29 deletions(-)
 create mode 100644 contrib/coccinelle/string-list.cocci
 create mode 100644 contrib/coccinelle/tests/string-list.c
 create mode 100644 contrib/coccinelle/tests/string-list.res

Range-diff against v1:
1:  c89758491e7 ! 1:  61a62bdf8e9 string_list API users + cocci: use string_list_init_dup()
    @@ Commit message
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
    - ## contrib/coccinelle/string_list.cocci (new) ##
    + ## contrib/coccinelle/string-list.cocci (new) ##
     @@
     +@@
     +struct string_list *P;
    @@ contrib/coccinelle/string_list.cocci (new)
     +- (P)->strdup_strings = 1;
     ++ string_list_init_dup(P);
     
    - ## contrib/coccinelle/tests/string_list.c (new) ##
    + ## contrib/coccinelle/tests/string-list.c (new) ##
     @@
     +int init(void)
     +{
    @@ contrib/coccinelle/tests/string_list.c (new)
     +	list->strdup_strings = 1;
     +}
     
    - ## contrib/coccinelle/tests/string_list.res (new) ##
    + ## contrib/coccinelle/tests/string-list.res (new) ##
     @@
     +int init(void)
     +{
2:  5d8baa9cbc4 ! 2:  33e551a2f4c string-list API users: manually use string_list_init_*()
    @@ Metadata
     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Commit message ##
    -    string-list API users: manually use string_list_init_*()
    +    cocci: apply string_list.cocci with --disable-worth-trying-opt
     
    -    Convert various code that didn't use string_list_init_*() to do so, in
    -    cases where the only thing being allocated was the string list we can
    -    change from CALLOC_ARRAY() to ALLOC_ARRAY(), the string_list_init_*()
    -    function will zero out the memory.
    +    Apply the new string-list.cocci added in the preceding commit with
    +    --disable-worth-trying-opt. For optimization purposes we run spatch in
    +    a mode where even though we run it with --all-includes we'll miss some
    +    changes because we don't use --disable-worth-trying-opt.
     
    -    This covers cases that weren't matched by tho coccinelle rule in the
    -    preceding commit, which is conservative enough to care about the type
    -    of what we're modifying.
    +    This is because without that option it'll take a look at
    +    notes-utils.c, and conclude that it doesn't need to process
    +    it (irrelevant output excluded with "[...]"):
    +
    +            $ spatch --sp-file contrib/coccinelle/string-list.cocci --patch . notes-utils.c
    +            [...]
    +            (ONCE) Expected tokens string_list strdup_strings CALLOC_ARRAY
    +            Skipping: notes-utils.c
    +
    +    This is just one of the known (and probably some unknown) issues where
    +    our "make coccicheck" fails to include changes for whatever
    +    reason. That should be fixed more generally, but let's just fix this
    +    manually for now.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
    @@ notes-utils.c: struct notes_rewrite_cfg *init_copy_notes_for_rewrite(const char
      	c->refs_from_env = 0;
      	c->mode_from_env = 0;
      	if (rewrite_mode_env) {
    -
    - ## reflog-walk.c ##
    -@@ reflog-walk.c: struct reflog_walk_info {
    - void init_reflog_walk(struct reflog_walk_info **info)
    - {
    - 	CALLOC_ARRAY(*info, 1);
    --	(*info)->complete_reflogs.strdup_strings = 1;
    -+	string_list_init_dup(&((*info)->complete_reflogs));
    - }
    - 
    - void reflog_walk_info_release(struct reflog_walk_info *info)
-:  ----------- > 3:  62aab32ae77 reflog-walk.c: use string_list_init_dup()
-:  ----------- > 4:  2d858c49243 cocci: add "string_list" rule to swap "DUP" <-> "NODUP"
-:  ----------- > 5:  8c0ac6cbd96 string-list API users: don't tweak "strdup_strings" to free dupes
-:  ----------- > 6:  b0de7a63d1c notes.c: make "struct string_list display_notes_refs" non-static
-- 
2.37.1.1095.g64a1e8362fd

[PATCH v2 1/6] string_list API users + cocci: use string_list_init_dup()

[Ævar Arnfjörð Bjarmason]

Add a coccinelle rule to detect a particular misuse of the "struct
string_list" API. We have the *_INIT macros, but this code assumed
that a zero'd out "struct string_list" with a "strdup_string" set
would be the same as string_list_init_dup().

That assumption happens to be right, but let's instead use the helper
functions introduced in 183113a5ca9 (string_list: Add STRING_LIST_INIT
macro and make use of it., 2010-07-04).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 contrib/coccinelle/string-list.cocci     | 8 ++++++++
 contrib/coccinelle/tests/string-list.c   | 7 +++++++
 contrib/coccinelle/tests/string-list.res | 7 +++++++
 refs.c                                   | 4 ++--
 resolve-undo.c                           | 8 ++++----
 revision.c                               | 4 ++--
 6 files changed, 30 insertions(+), 8 deletions(-)
 create mode 100644 contrib/coccinelle/string-list.cocci
 create mode 100644 contrib/coccinelle/tests/string-list.c
 create mode 100644 contrib/coccinelle/tests/string-list.res

diff --git a/contrib/coccinelle/string-list.cocci b/contrib/coccinelle/string-list.cocci
new file mode 100644
index 00000000000..5d285d5732c
--- /dev/null
+++ b/contrib/coccinelle/string-list.cocci
@@ -0,0 +1,8 @@
+@@
+struct string_list *P;
+@@
+- CALLOC_ARRAY(P, 1);
++ ALLOC_ARRAY(P, 1);
+... when != P
+- (P)->strdup_strings = 1;
++ string_list_init_dup(P);
diff --git a/contrib/coccinelle/tests/string-list.c b/contrib/coccinelle/tests/string-list.c
new file mode 100644
index 00000000000..e77822b7682
--- /dev/null
+++ b/contrib/coccinelle/tests/string-list.c
@@ -0,0 +1,7 @@
+int init(void)
+{
+	struct string_list *list;
+
+	CALLOC_ARRAY(list, 1);
+	list->strdup_strings = 1;
+}
diff --git a/contrib/coccinelle/tests/string-list.res b/contrib/coccinelle/tests/string-list.res
new file mode 100644
index 00000000000..7e666f5bf48
--- /dev/null
+++ b/contrib/coccinelle/tests/string-list.res
@@ -0,0 +1,7 @@
+int init(void)
+{
+	struct string_list *list;
+
+	ALLOC_ARRAY(list, 1);
+	string_list_init_dup(list);
+}
diff --git a/refs.c b/refs.c
index 90bcb271687..83151a42b3a 100644
--- a/refs.c
+++ b/refs.c
@@ -1313,8 +1313,8 @@ int parse_hide_refs_config(const char *var, const char *value, const char *secti
 		while (len && ref[len - 1] == '/')
 			ref[--len] = '\0';
 		if (!hide_refs) {
-			CALLOC_ARRAY(hide_refs, 1);
-			hide_refs->strdup_strings = 1;
+			ALLOC_ARRAY(hide_refs, 1);
+			string_list_init_dup(hide_refs);
 		}
 		string_list_append(hide_refs, ref);
 	}
diff --git a/resolve-undo.c b/resolve-undo.c
index e81096e2d45..e66b8306fe0 100644
--- a/resolve-undo.c
+++ b/resolve-undo.c
@@ -15,8 +15,8 @@ void record_resolve_undo(struct index_state *istate, struct cache_entry *ce)
 		return;
 
 	if (!istate->resolve_undo) {
-		CALLOC_ARRAY(resolve_undo, 1);
-		resolve_undo->strdup_strings = 1;
+		ALLOC_ARRAY(resolve_undo, 1);
+		string_list_init_dup(resolve_undo);
 		istate->resolve_undo = resolve_undo;
 	}
 	resolve_undo = istate->resolve_undo;
@@ -57,8 +57,8 @@ struct string_list *resolve_undo_read(const char *data, unsigned long size)
 	int i;
 	const unsigned rawsz = the_hash_algo->rawsz;
 
-	CALLOC_ARRAY(resolve_undo, 1);
-	resolve_undo->strdup_strings = 1;
+	ALLOC_ARRAY(resolve_undo, 1);
+	string_list_init_dup(resolve_undo);
 
 	while (size) {
 		struct string_list_item *lost;
diff --git a/revision.c b/revision.c
index 0c6e26cd9c8..e44af92cacc 100644
--- a/revision.c
+++ b/revision.c
@@ -1578,8 +1578,8 @@ void clear_ref_exclusion(struct string_list **ref_excludes_p)
 void add_ref_exclusion(struct string_list **ref_excludes_p, const char *exclude)
 {
 	if (!*ref_excludes_p) {
-		CALLOC_ARRAY(*ref_excludes_p, 1);
-		(*ref_excludes_p)->strdup_strings = 1;
+		ALLOC_ARRAY(*ref_excludes_p, 1);
+		string_list_init_dup(*ref_excludes_p);
 	}
 	string_list_append(*ref_excludes_p, exclude);
 }
-- 
2.37.1.1095.g64a1e8362fd

[PATCH v2 2/6] cocci: apply string_list.cocci with --disable-worth-trying-opt

[Ævar Arnfjörð Bjarmason]

Apply the new string-list.cocci added in the preceding commit with
--disable-worth-trying-opt. For optimization purposes we run spatch in
a mode where even though we run it with --all-includes we'll miss some
changes because we don't use --disable-worth-trying-opt.

This is because without that option it'll take a look at
notes-utils.c, and conclude that it doesn't need to process
it (irrelevant output excluded with "[...]"):

	$ spatch --sp-file contrib/coccinelle/string-list.cocci --patch . notes-utils.c
	[...]
	(ONCE) Expected tokens string_list strdup_strings CALLOC_ARRAY
	Skipping: notes-utils.c

This is just one of the known (and probably some unknown) issues where
our "make coccicheck" fails to include changes for whatever
reason. That should be fixed more generally, but let's just fix this
manually for now.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 notes-utils.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/notes-utils.c b/notes-utils.c
index d7d18e30f5a..73559d24ec8 100644
--- a/notes-utils.c
+++ b/notes-utils.c
@@ -129,8 +129,8 @@ struct notes_rewrite_cfg *init_copy_notes_for_rewrite(const char *cmd)
 	c->cmd = cmd;
 	c->enabled = 1;
 	c->combine = combine_notes_concatenate;
-	CALLOC_ARRAY(c->refs, 1);
-	c->refs->strdup_strings = 1;
+	ALLOC_ARRAY(c->refs, 1);
+	string_list_init_dup(c->refs);
 	c->refs_from_env = 0;
 	c->mode_from_env = 0;
 	if (rewrite_mode_env) {
-- 
2.37.1.1095.g64a1e8362fd

[PATCH v2 3/6] reflog-walk.c: use string_list_init_dup()

[Ævar Arnfjörð Bjarmason]

Change init_reflog_walk() to use the helper function added in
183113a5ca9 (string_list: Add STRING_LIST_INIT macro and make use of
it., 2010-07-04) rather than assuming that the "struct string_list"
part of "struct reflog_walk_info" can be memset() to 0, followed by
flipping ".strdup_strings = 1".

As explained in a preceding commit that can be assumed now if we peek
behind the guts of the "struct string_list", but there's no reason to
break the encapsulation here.

This does not match the newly added "struct string_list" rule for
changing CALLOC_ARRAY() followed by ".strdup_strings = 1" to
"ALLOC_ARRAY()" and "string_list_init_dup()" because here the
"CALLOC_ARRAY()" is allocating more than just the "struct
string_list".

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 reflog-walk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/reflog-walk.c b/reflog-walk.c
index 7aa6595a51f..2b17408f9a4 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -120,7 +120,7 @@ struct reflog_walk_info {
 void init_reflog_walk(struct reflog_walk_info **info)
 {
 	CALLOC_ARRAY(*info, 1);
-	(*info)->complete_reflogs.strdup_strings = 1;
+	string_list_init_dup(&((*info)->complete_reflogs));
 }
 
 void reflog_walk_info_release(struct reflog_walk_info *info)
-- 
2.37.1.1095.g64a1e8362fd

[PATCH v2 4/6] cocci: add "string_list" rule to swap "DUP" <-> "NODUP"

[Ævar Arnfjörð Bjarmason]

Add a coccinelle rule to swap "NODUP" and "DUP" initialization in
cases such as [1], which as this change shows produces an identical
change.

We happened to have only one change in-tree that matched this
criteria, but now we're more certain of that, and will convert these
sorts of cases in the future.

1. https://lore.kernel.org/git/xmqq7d471dns.fsf@gitster.g/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/remote.c                         |  3 +--
 contrib/coccinelle/string-list.cocci     | 18 ++++++++++++++++++
 contrib/coccinelle/tests/string-list.c   | 13 +++++++++++++
 contrib/coccinelle/tests/string-list.res | 11 +++++++++++
 4 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/builtin/remote.c b/builtin/remote.c
index d9b8746cb3c..c713463d89d 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -1229,10 +1229,9 @@ static int get_one_entry(struct remote *remote, void *priv)
 
 static int show_all(void)
 {
-	struct string_list list = STRING_LIST_INIT_NODUP;
+	struct string_list list = STRING_LIST_INIT_DUP;
 	int result;
 
-	list.strdup_strings = 1;
 	result = for_each_remote(get_one_entry, &list);
 
 	if (!result) {
diff --git a/contrib/coccinelle/string-list.cocci b/contrib/coccinelle/string-list.cocci
index 5d285d5732c..63bb2abc93a 100644
--- a/contrib/coccinelle/string-list.cocci
+++ b/contrib/coccinelle/string-list.cocci
@@ -6,3 +6,21 @@ struct string_list *P;
 ... when != P
 - (P)->strdup_strings = 1;
 + string_list_init_dup(P);
+
+@@
+type T;
+identifier I;
+constant INIT_NODUP =~ "^STRING_LIST_INIT_NODUP$";
+constant INIT_DUP =~ "^STRING_LIST_INIT_DUP$";
+@@
+(
+- T I = INIT_NODUP;
++ T I = STRING_LIST_INIT_DUP;
+... when != &I
+- I.strdup_strings = 1;
+|
+- T I = INIT_DUP;
++ T I = STRING_LIST_INIT_NODUP;
+... when != &I
+- I.strdup_strings = 0;
+)
diff --git a/contrib/coccinelle/tests/string-list.c b/contrib/coccinelle/tests/string-list.c
index e77822b7682..1821ed4ebb4 100644
--- a/contrib/coccinelle/tests/string-list.c
+++ b/contrib/coccinelle/tests/string-list.c
@@ -1,7 +1,20 @@
 int init(void)
 {
 	struct string_list *list;
+	struct string_list list2 = STRING_LIST_INIT_NODUP;
+	struct string_list list3 = STRING_LIST_INIT_DUP;
+	struct string_list list4 = STRING_LIST_INIT_NODUP;
+	struct string_list list5 = STRING_LIST_INIT_DUP;
 
 	CALLOC_ARRAY(list, 1);
+
+	/* Exclude these */
+	string_list_append(&list4, "str");
+	string_list_append_nodup(&list5, xstrdup("str"));
+
 	list->strdup_strings = 1;
+	list2.strdup_strings = 1;
+	list3.strdup_strings = 0;
+	list4.strdup_strings = 1;
+	list5.strdup_strings = 0;
 }
diff --git a/contrib/coccinelle/tests/string-list.res b/contrib/coccinelle/tests/string-list.res
index 7e666f5bf48..58b3733dec2 100644
--- a/contrib/coccinelle/tests/string-list.res
+++ b/contrib/coccinelle/tests/string-list.res
@@ -1,7 +1,18 @@
 int init(void)
 {
 	struct string_list *list;
+	struct string_list list2 = STRING_LIST_INIT_DUP;
+	struct string_list list3 = STRING_LIST_INIT_NODUP;
+	struct string_list list4 = STRING_LIST_INIT_NODUP;
+	struct string_list list5 = STRING_LIST_INIT_DUP;
 
 	ALLOC_ARRAY(list, 1);
+
+	/* Exclude these */
+	string_list_append(&list4, "str");
+	string_list_append_nodup(&list5, xstrdup("str"));
+
 	string_list_init_dup(list);
+	list4.strdup_strings = 1;
+	list5.strdup_strings = 0;
 }
-- 
2.37.1.1095.g64a1e8362fd

[PATCH v2 5/6] string-list API users: don't tweak "strdup_strings" to free dupes

[Ævar Arnfjörð Bjarmason]

Change code added in [1] and [2] used in notes.c and bisect.c to
initialize a "struct string_list" as "DUP" and append with "nodup",
rather than doing it the other way around.

Settings up a "struct string_list" as "NODUP" and then manually
freeing its items by flipping the "strdup_strings" breaks the
encapsulation of the "struct string_list". It's also both more verbose
than the alternative, and not as safe. If we miss one of the codepaths
that appends to the list we'll end up freeing a constant string.

It's better to declare it as "dup", and then when we insert into the
list declare that the particular string we're inserting should be
owned by the "struct string_list" with string_list_append_nodup(). The
worst case with that API use is that we'll miss a caller, end up
double-dup-ing the string, and have a memory leak as a result.

1. 92e0d42539a (revision.c: make --no-notes reset --notes list,
   2011-03-29)
2. fb71a329964 (bisect--helper: `bisect_clean_state` shell function in
   C, 2017-09-29)

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 bisect.c | 7 +++----
 notes.c  | 8 ++------
 2 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/bisect.c b/bisect.c
index b63669cc9d7..8412feb1f69 100644
--- a/bisect.c
+++ b/bisect.c
@@ -1159,7 +1159,7 @@ static int mark_for_removal(const char *refname, const struct object_id *oid,
 {
 	struct string_list *refs = cb_data;
 	char *ref = xstrfmt("refs/bisect%s", refname);
-	string_list_append(refs, ref);
+	string_list_append_nodup(refs, ref);
 	return 0;
 }
 
@@ -1168,11 +1168,10 @@ int bisect_clean_state(void)
 	int result = 0;
 
 	/* There may be some refs packed during bisection */
-	struct string_list refs_for_removal = STRING_LIST_INIT_NODUP;
+	struct string_list refs_for_removal = STRING_LIST_INIT_DUP;
 	for_each_ref_in("refs/bisect", mark_for_removal, (void *) &refs_for_removal);
-	string_list_append(&refs_for_removal, xstrdup("BISECT_HEAD"));
+	string_list_append_nodup(&refs_for_removal, xstrdup("BISECT_HEAD"));
 	result = delete_refs("bisect: remove", &refs_for_removal, REF_NO_DEREF);
-	refs_for_removal.strdup_strings = 1;
 	string_list_clear(&refs_for_removal, 0);
 	unlink_or_warn(git_path_bisect_expected_rev());
 	unlink_or_warn(git_path_bisect_ancestors_ok());
diff --git a/notes.c b/notes.c
index 7452e71cc8d..acc35b580b6 100644
--- a/notes.c
+++ b/notes.c
@@ -1064,19 +1064,15 @@ void enable_ref_display_notes(struct display_notes_opt *opt, int *show_notes,
 	struct strbuf buf = STRBUF_INIT;
 	strbuf_addstr(&buf, ref);
 	expand_notes_ref(&buf);
-	string_list_append(&opt->extra_notes_refs,
-			strbuf_detach(&buf, NULL));
+	string_list_append_nodup(&opt->extra_notes_refs,
+				 strbuf_detach(&buf, NULL));
 	*show_notes = 1;
 }
 
 void disable_display_notes(struct display_notes_opt *opt, int *show_notes)
 {
 	opt->use_default_notes = -1;
-	/* we have been strdup'ing ourselves, so trick
-	 * string_list into free()ing strings */
-	opt->extra_notes_refs.strdup_strings = 1;
 	string_list_clear(&opt->extra_notes_refs, 0);
-	opt->extra_notes_refs.strdup_strings = 0;
 	*show_notes = 0;
 }
 
-- 
2.37.1.1095.g64a1e8362fd

[PATCH v2 6/6] notes.c: make "struct string_list display_notes_refs" non-static

[Ævar Arnfjörð Bjarmason]

Change the "struct string_list" added in 894a9d333e9 (Support showing
notes from more than one notes tree, 2010-03-12) to be non-static. Now
we don't need to flip-flop from "NODUP" to ".strdup_strings = 1".

In 2721ce21e43 (use string_list initializer consistently, 2016-06-13)
the statically initialized variable was made to use
"STRING_LIST_INIT_NODUP". That was correct in the narrow sense, but
didn't make much sense when this code was viewed as a whole. We'd set
it to "NODUP" just to set it to "DUP" when we'd use it.

This change could be smaller in just changing the "NODUP" to a "DUP",
but let's prove to ourselves and the compiler that this data is only
ever used when we enter load_display_notes(). We can just allocate it
in that function, and pass it to the notes_display_config() callback.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 notes.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/notes.c b/notes.c
index acc35b580b6..272fcb270a1 100644
--- a/notes.c
+++ b/notes.c
@@ -75,7 +75,6 @@ struct non_note {
 
 struct notes_tree default_notes_tree;
 
-static struct string_list display_notes_refs = STRING_LIST_INIT_NODUP;
 static struct notes_tree **display_notes_trees;
 
 static void load_subtree(struct notes_tree *t, struct leaf_node *subtree,
@@ -967,14 +966,19 @@ void string_list_add_refs_from_colon_sep(struct string_list *list,
 	free(globs_copy);
 }
 
+struct notes_display_config_cb {
+	struct string_list *list;
+	int *load_refs;
+};
+
 static int notes_display_config(const char *k, const char *v, void *cb)
 {
-	int *load_refs = cb;
+	struct notes_display_config_cb *data = cb;
 
-	if (*load_refs && !strcmp(k, "notes.displayref")) {
+	if (*data->load_refs && !strcmp(k, "notes.displayref")) {
 		if (!v)
 			return config_error_nonbool(k);
-		string_list_add_refs_by_glob(&display_notes_refs, v);
+		string_list_add_refs_by_glob(data->list, v);
 	}
 
 	return 0;
@@ -1080,7 +1084,11 @@ void load_display_notes(struct display_notes_opt *opt)
 {
 	char *display_ref_env;
 	int load_config_refs = 0;
-	display_notes_refs.strdup_strings = 1;
+	struct string_list display_notes_refs = STRING_LIST_INIT_DUP;
+	struct notes_display_config_cb cb_data = {
+		.list = &display_notes_refs,
+		.load_refs = &load_config_refs,
+	};
 
 	assert(!display_notes_trees);
 
@@ -1096,7 +1104,7 @@ void load_display_notes(struct display_notes_opt *opt)
 			load_config_refs = 1;
 	}
 
-	git_config(notes_display_config, &load_config_refs);
+	git_config(notes_display_config, &cb_data);
 
 	if (opt) {
 		struct string_list_item *item;
-- 
2.37.1.1095.g64a1e8362fd