[PATCH 0/5] *.[ch]: don't duplicate *_init() and *_INIT logic

[PATCH 0/5] *.[ch]: don't duplicate *_init() and *_INIT logic

[Ævar Arnfjörð Bjarmason]

This series implements a suggestion by Jeff King to use an idiom for
*_init() functions that avoids duplicating what we've declared in the
corresponding *_INIT macros. See
https://lore.kernel.org/git/YNytp0JAIaQih0Y4@coredump.intra.peff.net/

Ævar Arnfjörð Bjarmason (5):
  *.h: move some *_INIT to designated initializers
  *.c *_init(): define in terms of corresponding *_INIT macro
  dir.[ch]: replace dir_init() with DIR_INIT
  string-list.[ch]: add a string_list_init_{nodup,dup}()
  string-list.h users: change to use *_{nodup,dup}()

 apply.c                |  6 +++---
 archive.c              |  2 +-
 builtin/add.c          |  3 +--
 builtin/check-ignore.c |  3 +--
 builtin/clean.c        |  6 ++----
 builtin/grep.c         |  3 +--
 builtin/ls-files.c     |  3 +--
 builtin/stash.c        |  3 +--
 config.c               |  2 +-
 credential.c           |  4 ++--
 credential.h           |  4 +++-
 dir.c                  |  9 ++-------
 dir.h                  |  4 ++--
 entry.c                |  4 ++--
 json-writer.c          |  6 ++----
 json-writer.h          |  5 ++++-
 merge-ort.c            |  4 ++--
 merge-recursive.c      |  4 ++--
 merge.c                |  3 +--
 refs/packed-backend.c  |  2 +-
 run-command.c          |  5 ++---
 run-command.h          |  5 ++++-
 strbuf.c               |  4 ++--
 string-list.c          | 18 ++++++++++++++++--
 string-list.h          | 15 +++++++++++----
 strmap.c               |  3 ++-
 strvec.c               |  5 ++---
 transport.c            |  2 +-
 wt-status.c            |  3 +--
 29 files changed, 76 insertions(+), 64 deletions(-)

-- 
2.32.0.623.ge833f40cd87

[PATCH 1/5] *.h: move some *_INIT to designated initializers

[Ævar Arnfjörð Bjarmason]

Move *_INIT macros I'll use in a subsequent commits to designated
initializers. This isn't required for those follow-up changes, but
since I'm changing things in this are let's use the modern pattern
over the old one while we're at it.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 credential.h  | 4 +++-
 json-writer.h | 5 ++++-
 run-command.h | 5 ++++-
 string-list.h | 4 ++--
 4 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/credential.h b/credential.h
index c0e17e3554f..f430e77fea4 100644
--- a/credential.h
+++ b/credential.h
@@ -128,7 +128,9 @@ struct credential {
 	char *path;
 };
 
-#define CREDENTIAL_INIT { STRING_LIST_INIT_DUP }
+#define CREDENTIAL_INIT { \
+	.helpers = STRING_LIST_INIT_DUP, \
+}
 
 /* Initialize a credential structure, setting all fields to empty. */
 void credential_init(struct credential *);
diff --git a/json-writer.h b/json-writer.h
index 83906b09c17..209355e0f12 100644
--- a/json-writer.h
+++ b/json-writer.h
@@ -64,7 +64,10 @@ struct json_writer
 	unsigned int pretty:1;
 };
 
-#define JSON_WRITER_INIT { STRBUF_INIT, STRBUF_INIT, 0, 0 }
+#define JSON_WRITER_INIT { \
+	.json = STRBUF_INIT, \
+	.open_stack = STRBUF_INIT, \
+}
 
 void jw_init(struct json_writer *jw);
 void jw_release(struct json_writer *jw);
diff --git a/run-command.h b/run-command.h
index d08414a92e7..62a922d23fb 100644
--- a/run-command.h
+++ b/run-command.h
@@ -141,7 +141,10 @@ struct child_process {
 	void *clean_on_exit_handler_cbdata;
 };
 
-#define CHILD_PROCESS_INIT { NULL, STRVEC_INIT, STRVEC_INIT }
+#define CHILD_PROCESS_INIT { \
+	.args = STRVEC_INIT, \
+	.env_array = STRVEC_INIT, \
+}
 
 /**
  * The functions: child_process_init, start_command, finish_command,
diff --git a/string-list.h b/string-list.h
index 6c5d274126a..521b9c0748d 100644
--- a/string-list.h
+++ b/string-list.h
@@ -91,8 +91,8 @@ struct string_list {
 	compare_strings_fn cmp; /* NULL uses strcmp() */
 };
 
-#define STRING_LIST_INIT_NODUP { NULL, 0, 0, 0, NULL }
-#define STRING_LIST_INIT_DUP   { NULL, 0, 0, 1, NULL }
+#define STRING_LIST_INIT_NODUP { 0 }
+#define STRING_LIST_INIT_DUP   { .strdup_strings = 1 }
 
 /* General functions which work with both sorted and unsorted lists. */
 
-- 
2.32.0.623.ge833f40cd87

Re: [PATCH 1/5] *.h: move some *_INIT to designated initializers

[Martin Ågren]

On Thu, 1 Jul 2021 at 12:54, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>
> Move *_INIT macros I'll use in a subsequent commits to designated
> initializers. This isn't required for those follow-up changes, but
> since I'm changing things in this are let's use the modern pattern
> over the old one while we're at it.

s/are/area/, maybe even s/area/area,/ on top.

> -#define CREDENTIAL_INIT { STRING_LIST_INIT_DUP }
> +#define CREDENTIAL_INIT { \
> +       .helpers = STRING_LIST_INIT_DUP, \
> +}

I've verified that all of these designated initializers assign the exact
same fields before and after this commit, e.g., `helpers` actually is
what comes first in the struct.

> -#define STRING_LIST_INIT_NODUP { NULL, 0, 0, 0, NULL }
> -#define STRING_LIST_INIT_DUP   { NULL, 0, 0, 1, NULL }
> +#define STRING_LIST_INIT_NODUP { 0 }
> +#define STRING_LIST_INIT_DUP   { .strdup_strings = 1 }

This "NODUP" one is a bit of an odd case though. You don't actually
change it to use designated initializers, as the patch says. Instead you
change it in a slightly different way. In a sense, you assign the
literal zero not to strdup_strings, but to the first field, which is a
pointer, where I would have expected NULL.

I think there's been some recent-ish "we can assign `{ 0 }` even if the
first field is a pointer, because it's idiomatic and works out fine"
even if assigning 0 to a pointer looks a bit odd. So it might not be
wrong as such, but it doesn't match what the commit message claims, and
I think it would be more clear to use `{ .strdup_strings = 0 }` here:
We're explicitly interested in *not* duplicating the strings. It's not
just some default "let's leave everything as zero/NULL".

Martin

Re: [PATCH 1/5] *.h: move some *_INIT to designated initializers

[Junio C Hamano]

Martin Ågren <martin.agren@gmail.com> writes:

>> -#define STRING_LIST_INIT_NODUP { NULL, 0, 0, 0, NULL }
>> +#define STRING_LIST_INIT_NODUP { 0 }
>
> This "NODUP" one is a bit of an odd case though. You don't actually
> change it to use designated initializers, as the patch says. Instead you
> change it in a slightly different way. In a sense, you assign the
> literal zero not to strdup_strings, but to the first field, which is a
> pointer, where I would have expected NULL.
>
> I think there's been some recent-ish "we can assign `{ 0 }` even if the
> first field is a pointer, because it's idiomatic and works out fine"
> even if assigning 0 to a pointer looks a bit odd. So it might not be
> wrong as such, but it doesn't match what the commit message claims, and
> I think it would be more clear to use `{ .strdup_strings = 0 }` here:
> We're explicitly interested in *not* duplicating the strings. It's not
> just some default "let's leave everything as zero/NULL".

Well reasoned.  I agree that it is not wrong per-se, and the
solution to use designated initializer is indeed in line with the
theme of the topic.

Re: [PATCH 1/5] *.h: move some *_INIT to designated initializers

[Ævar Arnfjörð Bjarmason]

On Thu, Jul 01 2021, Martin Ågren wrote:

> On Thu, 1 Jul 2021 at 12:54, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>>
>> Move *_INIT macros I'll use in a subsequent commits to designated
>> initializers. This isn't required for those follow-up changes, but
>> since I'm changing things in this are let's use the modern pattern
>> over the old one while we're at it.
>
> s/are/area/, maybe even s/area/area,/ on top.

Will fix.

>> -#define CREDENTIAL_INIT { STRING_LIST_INIT_DUP }
>> +#define CREDENTIAL_INIT { \
>> +       .helpers = STRING_LIST_INIT_DUP, \
>> +}
>
> I've verified that all of these designated initializers assign the exact
> same fields before and after this commit, e.g., `helpers` actually is
> what comes first in the struct.

Thanks!

>> -#define STRING_LIST_INIT_NODUP { NULL, 0, 0, 0, NULL }
>> -#define STRING_LIST_INIT_DUP   { NULL, 0, 0, 1, NULL }
>> +#define STRING_LIST_INIT_NODUP { 0 }
>> +#define STRING_LIST_INIT_DUP   { .strdup_strings = 1 }
>
> This "NODUP" one is a bit of an odd case though. You don't actually
> change it to use designated initializers, as the patch says. Instead you
> change it in a slightly different way. In a sense, you assign the
> literal zero not to strdup_strings, but to the first field, which is a
> pointer, where I would have expected NULL.
>
> I think there's been some recent-ish "we can assign `{ 0 }` even if the
> first field is a pointer, because it's idiomatic and works out fine"
> even if assigning 0 to a pointer looks a bit odd. So it might not be
> wrong as such, but it doesn't match what the commit message claims, and
> I think it would be more clear to use `{ .strdup_strings = 0 }` here:
> We're explicitly interested in *not* duplicating the strings. It's not
> just some default "let's leave everything as zero/NULL".

Yes I'm really conflating a few different things here:

 1. Whether to use designated initializers.

 2. Whether to skip explicit initialization, i.e. not enumerate
    everything, and have the ones not mentioned initializes as if they
    had static storage duration.

 3. Using the 0 null pointer constant instead of NULL while doing #2.

 4. Using our in-codebase idiom of only setting any boolean flags during
    initialization if we're setting them to true, 

So I think in this case just keeping it as "{ 0 }" makes the most
sense. Even better would be just a "{}", but that's a GNU-only
extension.

I think "{ NULL }" is just a distraction, i.e. it's better to use "{ 0
}" consistently everywhere. C guarantees the same-ness of the two, and
the point is to initialize the struct as if though it were done with a
memset() to 0, not mentally keep track of whether the first member is an
int, pointer or char * (in one other case we use '\0').

[PATCH 2/5] *.c *_init(): define in terms of corresponding *_INIT macro

[Ævar Arnfjörð Bjarmason]

Change the common patter in the codebase of duplicating the
initialization logic between an *_INIT macro and a
corresponding *_init() function to use the macro as the canonical
source of truth.

Now we no longer need to keep the function up-to-date with the macro
version. This implements a suggestion by Jeff King who found that
under -O2 [1] modern compilers will init new version in place without
the extra copy[1]. The performance of a single *_init() won't matter
in most cases, but even if it does we're going to be producing
efficient machine code to perform these operations.

1. https://lore.kernel.org/git/YNyrDxUO1PlGJvCn@coredump.intra.peff.net/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 credential.c  | 4 ++--
 json-writer.c | 6 ++----
 run-command.c | 5 ++---
 strbuf.c      | 4 ++--
 strmap.c      | 3 ++-
 strvec.c      | 5 ++---
 6 files changed, 12 insertions(+), 15 deletions(-)

diff --git a/credential.c b/credential.c
index e5202fbef26..3c05c7c6691 100644
--- a/credential.c
+++ b/credential.c
@@ -10,8 +10,8 @@
 
 void credential_init(struct credential *c)
 {
-	memset(c, 0, sizeof(*c));
-	c->helpers.strdup_strings = 1;
+	struct credential blank = CREDENTIAL_INIT;
+	memcpy(c, &blank, sizeof(*c));
 }
 
 void credential_clear(struct credential *c)
diff --git a/json-writer.c b/json-writer.c
index aadb9dbddc3..f1cfd8fa8c6 100644
--- a/json-writer.c
+++ b/json-writer.c
@@ -3,10 +3,8 @@
 
 void jw_init(struct json_writer *jw)
 {
-	strbuf_init(&jw->json, 0);
-	strbuf_init(&jw->open_stack, 0);
-	jw->need_comma = 0;
-	jw->pretty = 0;
+	struct json_writer blank = JSON_WRITER_INIT;
+	memcpy(jw, &blank, sizeof(*jw));;
 }
 
 void jw_release(struct json_writer *jw)
diff --git a/run-command.c b/run-command.c
index be6bc128cd9..8750df16d89 100644
--- a/run-command.c
+++ b/run-command.c
@@ -11,9 +11,8 @@
 
 void child_process_init(struct child_process *child)
 {
-	memset(child, 0, sizeof(*child));
-	strvec_init(&child->args);
-	strvec_init(&child->env_array);
+	struct child_process blank = CHILD_PROCESS_INIT;
+	memcpy(child, &blank, sizeof(*child));
 }
 
 void child_process_clear(struct child_process *child)
diff --git a/strbuf.c b/strbuf.c
index 4df30b45494..c8a5789694c 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -52,8 +52,8 @@ char strbuf_slopbuf[1];
 
 void strbuf_init(struct strbuf *sb, size_t hint)
 {
-	sb->alloc = sb->len = 0;
-	sb->buf = strbuf_slopbuf;
+	struct strbuf blank = STRBUF_INIT;
+	memcpy(sb, &blank, sizeof(*sb));
 	if (hint)
 		strbuf_grow(sb, hint);
 }
diff --git a/strmap.c b/strmap.c
index 4fb9f6100ec..ee486357082 100644
--- a/strmap.c
+++ b/strmap.c
@@ -25,7 +25,8 @@ static struct strmap_entry *find_strmap_entry(struct strmap *map,
 
 void strmap_init(struct strmap *map)
 {
-	strmap_init_with_options(map, NULL, 1);
+	struct strmap blank = STRMAP_INIT;
+	memcpy(map, &blank, sizeof(*map));
 }
 
 void strmap_init_with_options(struct strmap *map,
diff --git a/strvec.c b/strvec.c
index 21dce0a7a4d..61a76ce6cb9 100644
--- a/strvec.c
+++ b/strvec.c
@@ -6,9 +6,8 @@ const char *empty_strvec[] = { NULL };
 
 void strvec_init(struct strvec *array)
 {
-	array->v = empty_strvec;
-	array->nr = 0;
-	array->alloc = 0;
+	struct strvec blank = STRVEC_INIT;
+	memcpy(array, &blank, sizeof(*array));
 }
 
 static void strvec_push_nodup(struct strvec *array, const char *value)
-- 
2.32.0.623.ge833f40cd87

[PATCH 3/5] dir.[ch]: replace dir_init() with DIR_INIT

[Ævar Arnfjörð Bjarmason]

Remove the dir_init() function and replace it with a DIR_INIT
macro. In many cases in the codebase we need to initialize things with
a function for good reasons, e.g. needing to call another function on
initialization. The "dir_init()" function was not one such case, and
could trivially be replaced with a more idiomatic macro initialization
pattern.

The only place where we made use of its use of memset() was in
dir_clear() itself, which resets the contents of an an existing struct
pointer. Let's use the new "memcpy() a 'blank' struct on the stack"
idiom to do that reset.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/add.c          | 3 +--
 builtin/check-ignore.c | 3 +--
 builtin/clean.c        | 6 ++----
 builtin/grep.c         | 3 +--
 builtin/ls-files.c     | 3 +--
 builtin/stash.c        | 3 +--
 dir.c                  | 9 ++-------
 dir.h                  | 4 ++--
 merge.c                | 3 +--
 wt-status.c            | 3 +--
 10 files changed, 13 insertions(+), 27 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index b773b5a4993..09e684585d9 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -470,7 +470,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 {
 	int exit_status = 0;
 	struct pathspec pathspec;
-	struct dir_struct dir;
+	struct dir_struct dir = DIR_INIT;
 	int flags;
 	int add_new_files;
 	int require_pathspec;
@@ -577,7 +577,6 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 	die_in_unpopulated_submodule(&the_index, prefix);
 	die_path_inside_submodule(&the_index, &pathspec);
 
-	dir_init(&dir);
 	if (add_new_files) {
 		int baselen;
 
diff --git a/builtin/check-ignore.c b/builtin/check-ignore.c
index 81234552b7f..21912569650 100644
--- a/builtin/check-ignore.c
+++ b/builtin/check-ignore.c
@@ -153,7 +153,7 @@ static int check_ignore_stdin_paths(struct dir_struct *dir, const char *prefix)
 int cmd_check_ignore(int argc, const char **argv, const char *prefix)
 {
 	int num_ignored;
-	struct dir_struct dir;
+	struct dir_struct dir = DIR_INIT;
 
 	git_config(git_default_config, NULL);
 
@@ -182,7 +182,6 @@ int cmd_check_ignore(int argc, const char **argv, const char *prefix)
 	if (!no_index && read_cache() < 0)
 		die(_("index file corrupt"));
 
-	dir_init(&dir);
 	setup_standard_excludes(&dir);
 
 	if (stdin_paths) {
diff --git a/builtin/clean.c b/builtin/clean.c
index 4944cf440b4..98a2860409b 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -641,7 +641,7 @@ static int clean_cmd(void)
 
 static int filter_by_patterns_cmd(void)
 {
-	struct dir_struct dir;
+	struct dir_struct dir = DIR_INIT;
 	struct strbuf confirm = STRBUF_INIT;
 	struct strbuf **ignore_list;
 	struct string_list_item *item;
@@ -665,7 +665,6 @@ static int filter_by_patterns_cmd(void)
 		if (!confirm.len)
 			break;
 
-		dir_init(&dir);
 		pl = add_pattern_list(&dir, EXC_CMDL, "manual exclude");
 		ignore_list = strbuf_split_max(&confirm, ' ', 0);
 
@@ -890,7 +889,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 	int ignored_only = 0, config_set = 0, errors = 0, gone = 1;
 	int rm_flags = REMOVE_DIR_KEEP_NESTED_GIT;
 	struct strbuf abs_path = STRBUF_INIT;
-	struct dir_struct dir;
+	struct dir_struct dir = DIR_INIT;
 	struct pathspec pathspec;
 	struct strbuf buf = STRBUF_INIT;
 	struct string_list exclude_list = STRING_LIST_INIT_NODUP;
@@ -921,7 +920,6 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, options, builtin_clean_usage,
 			     0);
 
-	dir_init(&dir);
 	if (!interactive && !dry_run && !force) {
 		if (config_set)
 			die(_("clean.requireForce set to true and neither -i, -n, nor -f given; "
diff --git a/builtin/grep.c b/builtin/grep.c
index ab8822e68f4..7d2f8e5adb6 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -704,10 +704,9 @@ static int grep_objects(struct grep_opt *opt, const struct pathspec *pathspec,
 static int grep_directory(struct grep_opt *opt, const struct pathspec *pathspec,
 			  int exc_std, int use_index)
 {
-	struct dir_struct dir;
+	struct dir_struct dir = DIR_INIT;
 	int i, hit = 0;
 
-	dir_init(&dir);
 	if (!use_index)
 		dir.flags |= DIR_NO_GITLINKS;
 	if (exc_std)
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 45cc3b23dd6..29a26ad8ae4 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -608,7 +608,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 {
 	int require_work_tree = 0, show_tag = 0, i;
 	char *max_prefix;
-	struct dir_struct dir;
+	struct dir_struct dir = DIR_INIT;
 	struct pattern_list *pl;
 	struct string_list exclude_list = STRING_LIST_INIT_NODUP;
 	struct option builtin_ls_files_options[] = {
@@ -678,7 +678,6 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 	if (argc == 2 && !strcmp(argv[1], "-h"))
 		usage_with_options(ls_files_usage, builtin_ls_files_options);
 
-	dir_init(&dir);
 	prefix = cmd_prefix;
 	if (prefix)
 		prefix_len = strlen(prefix);
diff --git a/builtin/stash.c b/builtin/stash.c
index 9c72e4b1257..8f42360ca91 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -991,9 +991,8 @@ static int get_untracked_files(const struct pathspec *ps, int include_untracked,
 {
 	int i;
 	int found = 0;
-	struct dir_struct dir;
+	struct dir_struct dir = DIR_INIT;
 
-	dir_init(&dir);
 	if (include_untracked != INCLUDE_ALL_FILES)
 		setup_standard_excludes(&dir);
 
diff --git a/dir.c b/dir.c
index ebe5ec046e0..313e9324597 100644
--- a/dir.c
+++ b/dir.c
@@ -53,12 +53,6 @@ static enum path_treatment read_directory_recursive(struct dir_struct *dir,
 	int check_only, int stop_at_first_file, const struct pathspec *pathspec);
 static int resolve_dtype(int dtype, struct index_state *istate,
 			 const char *path, int len);
-
-void dir_init(struct dir_struct *dir)
-{
-	memset(dir, 0, sizeof(*dir));
-}
-
 struct dirent *readdir_skip_dot_and_dotdot(DIR *dirp)
 {
 	struct dirent *e;
@@ -3105,6 +3099,7 @@ void dir_clear(struct dir_struct *dir)
 	struct exclude_list_group *group;
 	struct pattern_list *pl;
 	struct exclude_stack *stk;
+	struct dir_struct new = DIR_INIT;
 
 	for (i = EXC_CMDL; i <= EXC_FILE; i++) {
 		group = &dir->exclude_list_group[i];
@@ -3132,7 +3127,7 @@ void dir_clear(struct dir_struct *dir)
 	}
 	strbuf_release(&dir->basebuf);
 
-	dir_init(dir);
+	memcpy(dir, &new, sizeof(*dir));
 }
 
 struct ondisk_untracked_cache {
diff --git a/dir.h b/dir.h
index e3db9b9ec65..8d0ddd8f18d 100644
--- a/dir.h
+++ b/dir.h
@@ -342,6 +342,8 @@ struct dir_struct {
 	unsigned visited_directories;
 };
 
+#define DIR_INIT { 0 }
+
 struct dirent *readdir_skip_dot_and_dotdot(DIR *dirp);
 
 /*Count the number of slashes for string s*/
@@ -367,8 +369,6 @@ int match_pathspec(struct index_state *istate,
 int report_path_error(const char *ps_matched, const struct pathspec *pathspec);
 int within_depth(const char *name, int namelen, int depth, int max_depth);
 
-void dir_init(struct dir_struct *dir);
-
 int fill_directory(struct dir_struct *dir,
 		   struct index_state *istate,
 		   const struct pathspec *pathspec);
diff --git a/merge.c b/merge.c
index 5fb88af1025..6e736881d90 100644
--- a/merge.c
+++ b/merge.c
@@ -53,7 +53,7 @@ int checkout_fast_forward(struct repository *r,
 	struct unpack_trees_options opts;
 	struct tree_desc t[MAX_UNPACK_TREES];
 	int i, nr_trees = 0;
-	struct dir_struct dir;
+	struct dir_struct dir = DIR_INIT;
 	struct lock_file lock_file = LOCK_INIT;
 
 	refresh_index(r->index, REFRESH_QUIET, NULL, NULL, NULL);
@@ -80,7 +80,6 @@ int checkout_fast_forward(struct repository *r,
 	}
 
 	memset(&opts, 0, sizeof(opts));
-	dir_init(&dir);
 	if (overwrite_ignore) {
 		dir.flags |= DIR_SHOW_IGNORED;
 		setup_standard_excludes(&dir);
diff --git a/wt-status.c b/wt-status.c
index 42b67357169..b5a3e1cc252 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -699,14 +699,13 @@ static void wt_status_collect_changes_initial(struct wt_status *s)
 static void wt_status_collect_untracked(struct wt_status *s)
 {
 	int i;
-	struct dir_struct dir;
+	struct dir_struct dir = DIR_INIT;
 	uint64_t t_begin = getnanotime();
 	struct index_state *istate = s->repo->index;
 
 	if (!s->show_untracked_files)
 		return;
 
-	dir_init(&dir);
 	if (s->show_untracked_files != SHOW_ALL_UNTRACKED_FILES)
 		dir.flags |=
 			DIR_SHOW_OTHER_DIRECTORIES | DIR_HIDE_EMPTY_DIRECTORIES;
-- 
2.32.0.623.ge833f40cd87

[PATCH 4/5] string-list.[ch]: add a string_list_init_{nodup,dup}()

[Ævar Arnfjörð Bjarmason]

In order to use the new "memcpy() a 'blank' struct on the stack"
pattern for string_list_init(), and to make the macro initialization
consistent with the function initialization introduce two new
string_list_init_{nodup,dup}() functions. These are like the old
string_list_init() when called with a false and true second argument,
respectively.

I think this not only makes things more consistent, but also easier to
read. I often had to lookup what the ", 0)" or ", 1)" in these
invocations meant, now it's right there in the function name, and
corresponds to the macros.

A subsequent commit will convert existing API users to this pattern,
but as this is a very common API let's leave a compatibility function
in place for later removal. This intermediate state also proves that
the compatibility function works.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 string-list.c | 18 ++++++++++++++++--
 string-list.h | 11 +++++++++--
 2 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/string-list.c b/string-list.c
index a917955fbd8..43576ad1265 100644
--- a/string-list.c
+++ b/string-list.c
@@ -1,10 +1,24 @@
 #include "cache.h"
 #include "string-list.h"
 
+void string_list_init_nodup(struct string_list *list)
+{
+	struct string_list blank = STRING_LIST_INIT_NODUP;
+	memcpy(list, &blank, sizeof(*list));
+}
+
+void string_list_init_dup(struct string_list *list)
+{
+	struct string_list blank = STRING_LIST_INIT_DUP;
+	memcpy(list, &blank, sizeof(*list));
+}
+
 void string_list_init(struct string_list *list, int strdup_strings)
 {
-	memset(list, 0, sizeof(*list));
-	list->strdup_strings = strdup_strings;
+	if (strdup_strings)
+		string_list_init_dup(list);
+	else
+		string_list_init_nodup(list);
 }
 
 /* if there is no exact match, point to the index where the entry could be
diff --git a/string-list.h b/string-list.h
index 521b9c0748d..0d6b4692396 100644
--- a/string-list.h
+++ b/string-list.h
@@ -97,8 +97,15 @@ struct string_list {
 /* General functions which work with both sorted and unsorted lists. */
 
 /**
- * Initialize the members of the string_list, set `strdup_strings`
- * member according to the value of the second parameter.
+ * Initialize the members of a string_list pointer in the same way as
+ * the corresponding `STRING_LIST_INIT_NODUP` and
+ * `STRING_LIST_INIT_DUP` macros.
+ */
+void string_list_init_nodup(struct string_list *list);
+void string_list_init_dup(struct string_list *list);
+
+/**
+ * TODO remove: For compatibility with any in-flight older API users
  */
 void string_list_init(struct string_list *list, int strdup_strings);
 
-- 
2.32.0.623.ge833f40cd87

[PATCH 5/5] string-list.h users: change to use *_{nodup,dup}()

[Ævar Arnfjörð Bjarmason]

Change all in-tree users of the string_list_init(LIST, BOOL) API to
use string_list_init_{nodup,dup}(LIST) instead.

As noted in the preceding commit let's leave the now-unused
string_list_init() wrapper in-place for any in-flight users, it can be
removed at some later date.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 apply.c               | 6 +++---
 archive.c             | 2 +-
 config.c              | 2 +-
 entry.c               | 4 ++--
 merge-ort.c           | 4 ++--
 merge-recursive.c     | 4 ++--
 refs/packed-backend.c | 2 +-
 transport.c           | 2 +-
 8 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/apply.c b/apply.c
index 853d3ed385a..44bc31d6eb5 100644
--- a/apply.c
+++ b/apply.c
@@ -101,9 +101,9 @@ int init_apply_state(struct apply_state *state,
 	state->ws_error_action = warn_on_ws_error;
 	state->ws_ignore_action = ignore_ws_none;
 	state->linenr = 1;
-	string_list_init(&state->fn_table, 0);
-	string_list_init(&state->limit_by_name, 0);
-	string_list_init(&state->symlink_changes, 0);
+	string_list_init_nodup(&state->fn_table);
+	string_list_init_nodup(&state->limit_by_name);
+	string_list_init_nodup(&state->symlink_changes);
 	strbuf_init(&state->root, 0);
 
 	git_apply_config();
diff --git a/archive.c b/archive.c
index ff2bb54f622..3c266d1d7bf 100644
--- a/archive.c
+++ b/archive.c
@@ -645,7 +645,7 @@ int write_archive(int argc, const char **argv, const char *prefix,
 	args.pretty_ctx = &ctx;
 	args.repo = repo;
 	args.prefix = prefix;
-	string_list_init(&args.extra_files, 1);
+	string_list_init_dup(&args.extra_files);
 	argc = parse_archive_args(argc, argv, &ar, &args, name_hint, remote);
 	if (!startup_info->have_repository) {
 		/*
diff --git a/config.c b/config.c
index f9c400ad306..2edb282b43d 100644
--- a/config.c
+++ b/config.c
@@ -2072,7 +2072,7 @@ static int configset_add_value(struct config_set *cs, const char *key, const cha
 		e = xmalloc(sizeof(*e));
 		hashmap_entry_init(&e->ent, strhash(key));
 		e->key = xstrdup(key);
-		string_list_init(&e->value_list, 1);
+		string_list_init_dup(&e->value_list);
 		hashmap_add(&cs->config_hash, &e->ent);
 	}
 	si = string_list_append_nodup(&e->value_list, xstrdup_or_null(value));
diff --git a/entry.c b/entry.c
index 711ee0693c7..125fabdbd52 100644
--- a/entry.c
+++ b/entry.c
@@ -143,8 +143,8 @@ void enable_delayed_checkout(struct checkout *state)
 	if (!state->delayed_checkout) {
 		state->delayed_checkout = xmalloc(sizeof(*state->delayed_checkout));
 		state->delayed_checkout->state = CE_CAN_DELAY;
-		string_list_init(&state->delayed_checkout->filters, 0);
-		string_list_init(&state->delayed_checkout->paths, 0);
+		string_list_init_nodup(&state->delayed_checkout->filters);
+		string_list_init_nodup(&state->delayed_checkout->paths);
 	}
 }
 
diff --git a/merge-ort.c b/merge-ort.c
index b954f7184a5..d7ae8d0558f 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -1836,7 +1836,7 @@ static void compute_collisions(struct strmap *collisions,
 			free(new_path);
 		} else {
 			CALLOC_ARRAY(collision_info, 1);
-			string_list_init(&collision_info->source_files, 0);
+			string_list_init_nodup(&collision_info->source_files);
 			strmap_put(collisions, new_path, collision_info);
 		}
 		string_list_insert(&collision_info->source_files,
@@ -3942,7 +3942,7 @@ static void merge_start(struct merge_options *opt, struct merge_result *result)
 	 */
 	strmap_init_with_options(&opt->priv->paths, NULL, 0);
 	strmap_init_with_options(&opt->priv->conflicted, NULL, 0);
-	string_list_init(&opt->priv->paths_to_free, 0);
+	string_list_init_nodup(&opt->priv->paths_to_free);
 
 	/*
 	 * keys & strbufs in output will sometimes need to outlive "paths",
diff --git a/merge-recursive.c b/merge-recursive.c
index d146bb116f7..be473f854be 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -121,7 +121,7 @@ static void dir_rename_entry_init(struct dir_rename_entry *entry,
 	entry->dir = directory;
 	entry->non_unique_new_dir = 0;
 	strbuf_init(&entry->new_dir, 0);
-	string_list_init(&entry->possible_new_dirs, 0);
+	string_list_init_nodup(&entry->possible_new_dirs);
 }
 
 struct collision_entry {
@@ -3703,7 +3703,7 @@ static int merge_start(struct merge_options *opt, struct tree *head)
 	}
 
 	CALLOC_ARRAY(opt->priv, 1);
-	string_list_init(&opt->priv->df_conflict_file_set, 1);
+	string_list_init_dup(&opt->priv->df_conflict_file_set);
 	return 0;
 }
 
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index dfecdbc1db6..5f50def076c 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -1425,7 +1425,7 @@ static int packed_transaction_prepare(struct ref_store *ref_store,
 	 */
 
 	CALLOC_ARRAY(data, 1);
-	string_list_init(&data->updates, 0);
+	string_list_init_nodup(&data->updates);
 
 	transaction->backend_data = data;
 
diff --git a/transport.c b/transport.c
index 50f5830eb6b..25f77ce6ab2 100644
--- a/transport.c
+++ b/transport.c
@@ -1052,7 +1052,7 @@ struct transport *transport_get(struct remote *remote, const char *url)
 	struct transport *ret = xcalloc(1, sizeof(*ret));
 
 	ret->progress = isatty(2);
-	string_list_init(&ret->pack_lockfiles, 1);
+	string_list_init_dup(&ret->pack_lockfiles);
 
 	if (!remote)
 		BUG("No remote provided to transport_get()");
-- 
2.32.0.623.ge833f40cd87

Re: [PATCH 0/5] *.[ch]: don't duplicate *_init() and *_INIT logic

[Jeff King]

On Thu, Jul 01, 2021 at 12:51:24PM +0200, Ævar Arnfjörð Bjarmason wrote:

> This series implements a suggestion by Jeff King to use an idiom for
> *_init() functions that avoids duplicating what we've declared in the
> corresponding *_INIT macros. See
> https://lore.kernel.org/git/YNytp0JAIaQih0Y4@coredump.intra.peff.net/

These all look pretty good to me. My comments are minor enough that I'll
include them here rather than individual replies:

> Ævar Arnfjörð Bjarmason (5):
>   *.h: move some *_INIT to designated initializers

I agree with Martin's suggestion to prefer the designated initializer
for STRING_LIST_INIT_NODUP. The consistency makes it easier to read.

>   *.c *_init(): define in terms of corresponding *_INIT macro

Yep, nice.

>   dir.[ch]: replace dir_init() with DIR_INIT

I like this. Since there is a dir_clear(), I guess somebody could argue
that lacking an init function makes things less consistent/balanced. But
if we don't need it, I'm happy to drop it.

>   string-list.[ch]: add a string_list_init_{nodup,dup}()

I like this. I agree the current "0/1" in the callers is kind of
inscrutable. It does make things harder if we wanted to add more
options, but that is already true due to the macro initializers.

>   string-list.h users: change to use *_{nodup,dup}()

Nice. IMHO you should drop the convenience wrapper here. Leaving it
means we'll have to circle back later and fix up topics in flight (and
it's not atomic; when we finally drop it, there may be more topics in
flight then!).

If it were somehow complicated to convert callers, that might be more
justified. But doing a quick conversion (which is easily detected by the
compiler) in the merge commit seems like less work than having to
revisit the topic later.

-Peff