* [PATCH v2 1/6] string_list API users + cocci: use string_list_init_dup()
2022-07-21 12:00 ` [PATCH v2 0/6] string-list API user: fix API use, some with coccinelle Ævar Arnfjörð Bjarmason
@ 2022-07-21 12:00 ` Ævar Arnfjörð Bjarmason
2022-07-21 12:00 ` [PATCH v2 2/6] cocci: apply string_list.cocci with --disable-worth-trying-opt Ævar Arnfjörð Bjarmason
` (4 subsequent siblings)
5 siblings, 0 replies; 11+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-07-21 12:00 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Elijah Newren,
Æ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
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH v2 2/6] cocci: apply string_list.cocci with --disable-worth-trying-opt
2022-07-21 12:00 ` [PATCH v2 0/6] string-list API user: fix API use, some with coccinelle Ævar Arnfjörð Bjarmason
2022-07-21 12:00 ` [PATCH v2 1/6] string_list API users + cocci: use string_list_init_dup() Ævar Arnfjörð Bjarmason
@ 2022-07-21 12:00 ` Ævar Arnfjörð Bjarmason
2022-07-21 12:00 ` [PATCH v2 3/6] reflog-walk.c: use string_list_init_dup() Ævar Arnfjörð Bjarmason
` (3 subsequent siblings)
5 siblings, 0 replies; 11+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-07-21 12:00 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Elijah Newren,
Æ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
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH v2 3/6] reflog-walk.c: use string_list_init_dup()
2022-07-21 12:00 ` [PATCH v2 0/6] string-list API user: fix API use, some with coccinelle Ævar Arnfjörð Bjarmason
2022-07-21 12:00 ` [PATCH v2 1/6] string_list API users + cocci: use string_list_init_dup() Ævar Arnfjörð Bjarmason
2022-07-21 12:00 ` [PATCH v2 2/6] cocci: apply string_list.cocci with --disable-worth-trying-opt Ævar Arnfjörð Bjarmason
@ 2022-07-21 12:00 ` Ævar Arnfjörð Bjarmason
2022-07-21 12:00 ` [PATCH v2 4/6] cocci: add "string_list" rule to swap "DUP" <-> "NODUP" Ævar Arnfjörð Bjarmason
` (2 subsequent siblings)
5 siblings, 0 replies; 11+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-07-21 12:00 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Elijah Newren,
Æ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
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH v2 4/6] cocci: add "string_list" rule to swap "DUP" <-> "NODUP"
2022-07-21 12:00 ` [PATCH v2 0/6] string-list API user: fix API use, some with coccinelle Ævar Arnfjörð Bjarmason
` (2 preceding siblings ...)
2022-07-21 12:00 ` [PATCH v2 3/6] reflog-walk.c: use string_list_init_dup() Ævar Arnfjörð Bjarmason
@ 2022-07-21 12:00 ` Ævar Arnfjörð Bjarmason
2022-07-21 12:00 ` [PATCH v2 5/6] string-list API users: don't tweak "strdup_strings" to free dupes Ævar Arnfjörð Bjarmason
2022-07-21 12:00 ` [PATCH v2 6/6] notes.c: make "struct string_list display_notes_refs" non-static Ævar Arnfjörð Bjarmason
5 siblings, 0 replies; 11+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-07-21 12:00 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Elijah Newren,
Æ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
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH v2 5/6] string-list API users: don't tweak "strdup_strings" to free dupes
2022-07-21 12:00 ` [PATCH v2 0/6] string-list API user: fix API use, some with coccinelle Ævar Arnfjörð Bjarmason
` (3 preceding siblings ...)
2022-07-21 12:00 ` [PATCH v2 4/6] cocci: add "string_list" rule to swap "DUP" <-> "NODUP" Ævar Arnfjörð Bjarmason
@ 2022-07-21 12:00 ` Ævar Arnfjörð Bjarmason
2022-07-21 12:00 ` [PATCH v2 6/6] notes.c: make "struct string_list display_notes_refs" non-static Ævar Arnfjörð Bjarmason
5 siblings, 0 replies; 11+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-07-21 12:00 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Elijah Newren,
Æ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
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH v2 6/6] notes.c: make "struct string_list display_notes_refs" non-static
2022-07-21 12:00 ` [PATCH v2 0/6] string-list API user: fix API use, some with coccinelle Ævar Arnfjörð Bjarmason
` (4 preceding siblings ...)
2022-07-21 12:00 ` [PATCH v2 5/6] string-list API users: don't tweak "strdup_strings" to free dupes Ævar Arnfjörð Bjarmason
@ 2022-07-21 12:00 ` Ævar Arnfjörð Bjarmason
5 siblings, 0 replies; 11+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-07-21 12:00 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Elijah Newren,
Æ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
^ permalink raw reply related [flat|nested] 11+ messages in thread