{"thread":{"id":"56036","subject":"[PATCH 0/5] *.[ch]: don't duplicate *_init() and *_INIT logic","startedAt":"2021-07-01T10:51:36Z","lastAt":"2021-07-01T16:28:58Z","messageCount":10,"participants":["Ævar Arnfjörð Bjarmason","Martin Ågren","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"428879","messageId":"cover-0.5-00000000000-20210701T104855Z-avarab@gmail.com","threadId":"56036","inReplyTo":null,"subject":"[PATCH 0/5] *.[ch]: don't duplicate *_init() and *_INIT logic","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-01T10:51:24Z","receivedAt":"2021-07-01T10:51:36Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This series implements a suggestion by Jeff King to use an idiom for\n*_init() functions that avoids duplicating what we've declared in the\ncorresponding *_INIT macros. See\nhttps://lore.kernel.org/git/YNytp0JAIaQih0Y4@coredump.intra.peff.net/\n\nÆvar Arnfjörð Bjarmason (5):\n  *.h: move some *_INIT to designated initializers\n  *.c *_init(): define in terms of corresponding *_INIT macro\n  dir.[ch]: replace dir_init() with DIR_INIT\n  string-list.[ch]: add a string_list_init_{nodup,dup}()\n  string-list.h users: change to use *_{nodup,dup}()\n\n apply.c                |  6 +++---\n archive.c              |  2 +-\n builtin/add.c          |  3 +--\n builtin/check-ignore.c |  3 +--\n builtin/clean.c        |  6 ++----\n builtin/grep.c         |  3 +--\n builtin/ls-files.c     |  3 +--\n builtin/stash.c        |  3 +--\n config.c               |  2 +-\n credential.c           |  4 ++--\n credential.h           |  4 +++-\n dir.c                  |  9 ++-------\n dir.h                  |  4 ++--\n entry.c                |  4 ++--\n json-writer.c          |  6 ++----\n json-writer.h          |  5 ++++-\n merge-ort.c            |  4 ++--\n merge-recursive.c      |  4 ++--\n merge.c                |  3 +--\n refs/packed-backend.c  |  2 +-\n run-command.c          |  5 ++---\n run-command.h          |  5 ++++-\n strbuf.c               |  4 ++--\n string-list.c          | 18 ++++++++++++++++--\n string-list.h          | 15 +++++++++++----\n strmap.c               |  3 ++-\n strvec.c               |  5 ++---\n transport.c            |  2 +-\n wt-status.c            |  3 +--\n 29 files changed, 76 insertions(+), 64 deletions(-)\n\n-- \n2.32.0.623.ge833f40cd87\n\n"},{"id":"428880","messageId":"patch-3.5-5f776976b81-20210701T104855Z-avarab@gmail.com","threadId":"56036","inReplyTo":"cover-0.5-00000000000-20210701T104855Z-avarab@gmail.com","subject":"[PATCH 3/5] dir.[ch]: replace dir_init() with DIR_INIT","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-01T10:51:27Z","receivedAt":"2021-07-01T10:51:40Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Remove the dir_init() function and replace it with a DIR_INIT\nmacro. In many cases in the codebase we need to initialize things with\na function for good reasons, e.g. needing to call another function on\ninitialization. The \"dir_init()\" function was not one such case, and\ncould trivially be replaced with a more idiomatic macro initialization\npattern.\n\nThe only place where we made use of its use of memset() was in\ndir_clear() itself, which resets the contents of an an existing struct\npointer. Let's use the new \"memcpy() a 'blank' struct on the stack\"\nidiom to do that reset.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n builtin/add.c          | 3 +--\n builtin/check-ignore.c | 3 +--\n builtin/clean.c        | 6 ++----\n builtin/grep.c         | 3 +--\n builtin/ls-files.c     | 3 +--\n builtin/stash.c        | 3 +--\n dir.c                  | 9 ++-------\n dir.h                  | 4 ++--\n merge.c                | 3 +--\n wt-status.c            | 3 +--\n 10 files changed, 13 insertions(+), 27 deletions(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex b773b5a4993..09e684585d9 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -470,7 +470,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n {\n \tint exit_status = 0;\n \tstruct pathspec pathspec;\n-\tstruct dir_struct dir;\n+\tstruct dir_struct dir = DIR_INIT;\n \tint flags;\n \tint add_new_files;\n \tint require_pathspec;\n@@ -577,7 +577,6 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \tdie_in_unpopulated_submodule(&the_index, prefix);\n \tdie_path_inside_submodule(&the_index, &pathspec);\n \n-\tdir_init(&dir);\n \tif (add_new_files) {\n \t\tint baselen;\n \ndiff --git a/builtin/check-ignore.c b/builtin/check-ignore.c\nindex 81234552b7f..21912569650 100644\n--- a/builtin/check-ignore.c\n+++ b/builtin/check-ignore.c\n@@ -153,7 +153,7 @@ static int check_ignore_stdin_paths(struct dir_struct *dir, const char *prefix)\n int cmd_check_ignore(int argc, const char **argv, const char *prefix)\n {\n \tint num_ignored;\n-\tstruct dir_struct dir;\n+\tstruct dir_struct dir = DIR_INIT;\n \n \tgit_config(git_default_config, NULL);\n \n@@ -182,7 +182,6 @@ int cmd_check_ignore(int argc, const char **argv, const char *prefix)\n \tif (!no_index && read_cache() < 0)\n \t\tdie(_(\"index file corrupt\"));\n \n-\tdir_init(&dir);\n \tsetup_standard_excludes(&dir);\n \n \tif (stdin_paths) {\ndiff --git a/builtin/clean.c b/builtin/clean.c\nindex 4944cf440b4..98a2860409b 100644\n--- a/builtin/clean.c\n+++ b/builtin/clean.c\n@@ -641,7 +641,7 @@ static int clean_cmd(void)\n \n static int filter_by_patterns_cmd(void)\n {\n-\tstruct dir_struct dir;\n+\tstruct dir_struct dir = DIR_INIT;\n \tstruct strbuf confirm = STRBUF_INIT;\n \tstruct strbuf **ignore_list;\n \tstruct string_list_item *item;\n@@ -665,7 +665,6 @@ static int filter_by_patterns_cmd(void)\n \t\tif (!confirm.len)\n \t\t\tbreak;\n \n-\t\tdir_init(&dir);\n \t\tpl = add_pattern_list(&dir, EXC_CMDL, \"manual exclude\");\n \t\tignore_list = strbuf_split_max(&confirm, ' ', 0);\n \n@@ -890,7 +889,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \tint ignored_only = 0, config_set = 0, errors = 0, gone = 1;\n \tint rm_flags = REMOVE_DIR_KEEP_NESTED_GIT;\n \tstruct strbuf abs_path = STRBUF_INIT;\n-\tstruct dir_struct dir;\n+\tstruct dir_struct dir = DIR_INIT;\n \tstruct pathspec pathspec;\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct string_list exclude_list = STRING_LIST_INIT_NODUP;\n@@ -921,7 +920,6 @@ int cmd_clean(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix, options, builtin_clean_usage,\n \t\t\t     0);\n \n-\tdir_init(&dir);\n \tif (!interactive && !dry_run && !force) {\n \t\tif (config_set)\n \t\t\tdie(_(\"clean.requireForce set to true and neither -i, -n, nor -f given; \"\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex ab8822e68f4..7d2f8e5adb6 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -704,10 +704,9 @@ static int grep_objects(struct grep_opt *opt, const struct pathspec *pathspec,\n static int grep_directory(struct grep_opt *opt, const struct pathspec *pathspec,\n \t\t\t  int exc_std, int use_index)\n {\n-\tstruct dir_struct dir;\n+\tstruct dir_struct dir = DIR_INIT;\n \tint i, hit = 0;\n \n-\tdir_init(&dir);\n \tif (!use_index)\n \t\tdir.flags |= DIR_NO_GITLINKS;\n \tif (exc_std)\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex 45cc3b23dd6..29a26ad8ae4 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -608,7 +608,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)\n {\n \tint require_work_tree = 0, show_tag = 0, i;\n \tchar *max_prefix;\n-\tstruct dir_struct dir;\n+\tstruct dir_struct dir = DIR_INIT;\n \tstruct pattern_list *pl;\n \tstruct string_list exclude_list = STRING_LIST_INIT_NODUP;\n \tstruct option builtin_ls_files_options[] = {\n@@ -678,7 +678,6 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)\n \tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n \t\tusage_with_options(ls_files_usage, builtin_ls_files_options);\n \n-\tdir_init(&dir);\n \tprefix = cmd_prefix;\n \tif (prefix)\n \t\tprefix_len = strlen(prefix);\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex 9c72e4b1257..8f42360ca91 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -991,9 +991,8 @@ static int get_untracked_files(const struct pathspec *ps, int include_untracked,\n {\n \tint i;\n \tint found = 0;\n-\tstruct dir_struct dir;\n+\tstruct dir_struct dir = DIR_INIT;\n \n-\tdir_init(&dir);\n \tif (include_untracked != INCLUDE_ALL_FILES)\n \t\tsetup_standard_excludes(&dir);\n \ndiff --git a/dir.c b/dir.c\nindex ebe5ec046e0..313e9324597 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -53,12 +53,6 @@ static enum path_treatment read_directory_recursive(struct dir_struct *dir,\n \tint check_only, int stop_at_first_file, const struct pathspec *pathspec);\n static int resolve_dtype(int dtype, struct index_state *istate,\n \t\t\t const char *path, int len);\n-\n-void dir_init(struct dir_struct *dir)\n-{\n-\tmemset(dir, 0, sizeof(*dir));\n-}\n-\n struct dirent *readdir_skip_dot_and_dotdot(DIR *dirp)\n {\n \tstruct dirent *e;\n@@ -3105,6 +3099,7 @@ void dir_clear(struct dir_struct *dir)\n \tstruct exclude_list_group *group;\n \tstruct pattern_list *pl;\n \tstruct exclude_stack *stk;\n+\tstruct dir_struct new = DIR_INIT;\n \n \tfor (i = EXC_CMDL; i <= EXC_FILE; i++) {\n \t\tgroup = &dir->exclude_list_group[i];\n@@ -3132,7 +3127,7 @@ void dir_clear(struct dir_struct *dir)\n \t}\n \tstrbuf_release(&dir->basebuf);\n \n-\tdir_init(dir);\n+\tmemcpy(dir, &new, sizeof(*dir));\n }\n \n struct ondisk_untracked_cache {\ndiff --git a/dir.h b/dir.h\nindex e3db9b9ec65..8d0ddd8f18d 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -342,6 +342,8 @@ struct dir_struct {\n \tunsigned visited_directories;\n };\n \n+#define DIR_INIT { 0 }\n+\n struct dirent *readdir_skip_dot_and_dotdot(DIR *dirp);\n \n /*Count the number of slashes for string s*/\n@@ -367,8 +369,6 @@ int match_pathspec(struct index_state *istate,\n int report_path_error(const char *ps_matched, const struct pathspec *pathspec);\n int within_depth(const char *name, int namelen, int depth, int max_depth);\n \n-void dir_init(struct dir_struct *dir);\n-\n int fill_directory(struct dir_struct *dir,\n \t\t   struct index_state *istate,\n \t\t   const struct pathspec *pathspec);\ndiff --git a/merge.c b/merge.c\nindex 5fb88af1025..6e736881d90 100644\n--- a/merge.c\n+++ b/merge.c\n@@ -53,7 +53,7 @@ int checkout_fast_forward(struct repository *r,\n \tstruct unpack_trees_options opts;\n \tstruct tree_desc t[MAX_UNPACK_TREES];\n \tint i, nr_trees = 0;\n-\tstruct dir_struct dir;\n+\tstruct dir_struct dir = DIR_INIT;\n \tstruct lock_file lock_file = LOCK_INIT;\n \n \trefresh_index(r->index, REFRESH_QUIET, NULL, NULL, NULL);\n@@ -80,7 +80,6 @@ int checkout_fast_forward(struct repository *r,\n \t}\n \n \tmemset(&opts, 0, sizeof(opts));\n-\tdir_init(&dir);\n \tif (overwrite_ignore) {\n \t\tdir.flags |= DIR_SHOW_IGNORED;\n \t\tsetup_standard_excludes(&dir);\ndiff --git a/wt-status.c b/wt-status.c\nindex 42b67357169..b5a3e1cc252 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -699,14 +699,13 @@ static void wt_status_collect_changes_initial(struct wt_status *s)\n static void wt_status_collect_untracked(struct wt_status *s)\n {\n \tint i;\n-\tstruct dir_struct dir;\n+\tstruct dir_struct dir = DIR_INIT;\n \tuint64_t t_begin = getnanotime();\n \tstruct index_state *istate = s->repo->index;\n \n \tif (!s->show_untracked_files)\n \t\treturn;\n \n-\tdir_init(&dir);\n \tif (s->show_untracked_files != SHOW_ALL_UNTRACKED_FILES)\n \t\tdir.flags |=\n \t\t\tDIR_SHOW_OTHER_DIRECTORIES | DIR_HIDE_EMPTY_DIRECTORIES;\n-- \n2.32.0.623.ge833f40cd87\n\n"},{"id":"428881","messageId":"patch-2.5-955dbd1693d-20210701T104855Z-avarab@gmail.com","threadId":"56036","inReplyTo":"cover-0.5-00000000000-20210701T104855Z-avarab@gmail.com","subject":"[PATCH 2/5] *.c *_init(): define in terms of corresponding *_INIT macro","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-01T10:51:26Z","receivedAt":"2021-07-01T10:51:40Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the common patter in the codebase of duplicating the\ninitialization logic between an *_INIT macro and a\ncorresponding *_init() function to use the macro as the canonical\nsource of truth.\n\nNow we no longer need to keep the function up-to-date with the macro\nversion. This implements a suggestion by Jeff King who found that\nunder -O2 [1] modern compilers will init new version in place without\nthe extra copy[1]. The performance of a single *_init() won't matter\nin most cases, but even if it does we're going to be producing\nefficient machine code to perform these operations.\n\n1. https://lore.kernel.org/git/YNyrDxUO1PlGJvCn@coredump.intra.peff.net/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n credential.c  | 4 ++--\n json-writer.c | 6 ++----\n run-command.c | 5 ++---\n strbuf.c      | 4 ++--\n strmap.c      | 3 ++-\n strvec.c      | 5 ++---\n 6 files changed, 12 insertions(+), 15 deletions(-)\n\ndiff --git a/credential.c b/credential.c\nindex e5202fbef26..3c05c7c6691 100644\n--- a/credential.c\n+++ b/credential.c\n@@ -10,8 +10,8 @@\n \n void credential_init(struct credential *c)\n {\n-\tmemset(c, 0, sizeof(*c));\n-\tc->helpers.strdup_strings = 1;\n+\tstruct credential blank = CREDENTIAL_INIT;\n+\tmemcpy(c, &blank, sizeof(*c));\n }\n \n void credential_clear(struct credential *c)\ndiff --git a/json-writer.c b/json-writer.c\nindex aadb9dbddc3..f1cfd8fa8c6 100644\n--- a/json-writer.c\n+++ b/json-writer.c\n@@ -3,10 +3,8 @@\n \n void jw_init(struct json_writer *jw)\n {\n-\tstrbuf_init(&jw->json, 0);\n-\tstrbuf_init(&jw->open_stack, 0);\n-\tjw->need_comma = 0;\n-\tjw->pretty = 0;\n+\tstruct json_writer blank = JSON_WRITER_INIT;\n+\tmemcpy(jw, &blank, sizeof(*jw));;\n }\n \n void jw_release(struct json_writer *jw)\ndiff --git a/run-command.c b/run-command.c\nindex be6bc128cd9..8750df16d89 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -11,9 +11,8 @@\n \n void child_process_init(struct child_process *child)\n {\n-\tmemset(child, 0, sizeof(*child));\n-\tstrvec_init(&child->args);\n-\tstrvec_init(&child->env_array);\n+\tstruct child_process blank = CHILD_PROCESS_INIT;\n+\tmemcpy(child, &blank, sizeof(*child));\n }\n \n void child_process_clear(struct child_process *child)\ndiff --git a/strbuf.c b/strbuf.c\nindex 4df30b45494..c8a5789694c 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -52,8 +52,8 @@ char strbuf_slopbuf[1];\n \n void strbuf_init(struct strbuf *sb, size_t hint)\n {\n-\tsb->alloc = sb->len = 0;\n-\tsb->buf = strbuf_slopbuf;\n+\tstruct strbuf blank = STRBUF_INIT;\n+\tmemcpy(sb, &blank, sizeof(*sb));\n \tif (hint)\n \t\tstrbuf_grow(sb, hint);\n }\ndiff --git a/strmap.c b/strmap.c\nindex 4fb9f6100ec..ee486357082 100644\n--- a/strmap.c\n+++ b/strmap.c\n@@ -25,7 +25,8 @@ static struct strmap_entry *find_strmap_entry(struct strmap *map,\n \n void strmap_init(struct strmap *map)\n {\n-\tstrmap_init_with_options(map, NULL, 1);\n+\tstruct strmap blank = STRMAP_INIT;\n+\tmemcpy(map, &blank, sizeof(*map));\n }\n \n void strmap_init_with_options(struct strmap *map,\ndiff --git a/strvec.c b/strvec.c\nindex 21dce0a7a4d..61a76ce6cb9 100644\n--- a/strvec.c\n+++ b/strvec.c\n@@ -6,9 +6,8 @@ const char *empty_strvec[] = { NULL };\n \n void strvec_init(struct strvec *array)\n {\n-\tarray->v = empty_strvec;\n-\tarray->nr = 0;\n-\tarray->alloc = 0;\n+\tstruct strvec blank = STRVEC_INIT;\n+\tmemcpy(array, &blank, sizeof(*array));\n }\n \n static void strvec_push_nodup(struct strvec *array, const char *value)\n-- \n2.32.0.623.ge833f40cd87\n\n"},{"id":"428882","messageId":"patch-1.5-9a92e7c216f-20210701T104855Z-avarab@gmail.com","threadId":"56036","inReplyTo":"cover-0.5-00000000000-20210701T104855Z-avarab@gmail.com","subject":"[PATCH 1/5] *.h: move some *_INIT to designated initializers","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-01T10:51:25Z","receivedAt":"2021-07-01T10:51:41Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Move *_INIT macros I'll use in a subsequent commits to designated\ninitializers. This isn't required for those follow-up changes, but\nsince I'm changing things in this are let's use the modern pattern\nover the old one while we're at it.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n credential.h  | 4 +++-\n json-writer.h | 5 ++++-\n run-command.h | 5 ++++-\n string-list.h | 4 ++--\n 4 files changed, 13 insertions(+), 5 deletions(-)\n\ndiff --git a/credential.h b/credential.h\nindex c0e17e3554f..f430e77fea4 100644\n--- a/credential.h\n+++ b/credential.h\n@@ -128,7 +128,9 @@ struct credential {\n \tchar *path;\n };\n \n-#define CREDENTIAL_INIT { STRING_LIST_INIT_DUP }\n+#define CREDENTIAL_INIT { \\\n+\t.helpers = STRING_LIST_INIT_DUP, \\\n+}\n \n /* Initialize a credential structure, setting all fields to empty. */\n void credential_init(struct credential *);\ndiff --git a/json-writer.h b/json-writer.h\nindex 83906b09c17..209355e0f12 100644\n--- a/json-writer.h\n+++ b/json-writer.h\n@@ -64,7 +64,10 @@ struct json_writer\n \tunsigned int pretty:1;\n };\n \n-#define JSON_WRITER_INIT { STRBUF_INIT, STRBUF_INIT, 0, 0 }\n+#define JSON_WRITER_INIT { \\\n+\t.json = STRBUF_INIT, \\\n+\t.open_stack = STRBUF_INIT, \\\n+}\n \n void jw_init(struct json_writer *jw);\n void jw_release(struct json_writer *jw);\ndiff --git a/run-command.h b/run-command.h\nindex d08414a92e7..62a922d23fb 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -141,7 +141,10 @@ struct child_process {\n \tvoid *clean_on_exit_handler_cbdata;\n };\n \n-#define CHILD_PROCESS_INIT { NULL, STRVEC_INIT, STRVEC_INIT }\n+#define CHILD_PROCESS_INIT { \\\n+\t.args = STRVEC_INIT, \\\n+\t.env_array = STRVEC_INIT, \\\n+}\n \n /**\n  * The functions: child_process_init, start_command, finish_command,\ndiff --git a/string-list.h b/string-list.h\nindex 6c5d274126a..521b9c0748d 100644\n--- a/string-list.h\n+++ b/string-list.h\n@@ -91,8 +91,8 @@ struct string_list {\n \tcompare_strings_fn cmp; /* NULL uses strcmp() */\n };\n \n-#define STRING_LIST_INIT_NODUP { NULL, 0, 0, 0, NULL }\n-#define STRING_LIST_INIT_DUP   { NULL, 0, 0, 1, NULL }\n+#define STRING_LIST_INIT_NODUP { 0 }\n+#define STRING_LIST_INIT_DUP   { .strdup_strings = 1 }\n \n /* General functions which work with both sorted and unsorted lists. */\n \n-- \n2.32.0.623.ge833f40cd87\n\n"},{"id":"428883","messageId":"patch-5.5-e6a6557b680-20210701T104855Z-avarab@gmail.com","threadId":"56036","inReplyTo":"cover-0.5-00000000000-20210701T104855Z-avarab@gmail.com","subject":"[PATCH 5/5] string-list.h users: change to use *_{nodup,dup}()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-01T10:51:29Z","receivedAt":"2021-07-01T10:51:42Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change all in-tree users of the string_list_init(LIST, BOOL) API to\nuse string_list_init_{nodup,dup}(LIST) instead.\n\nAs noted in the preceding commit let's leave the now-unused\nstring_list_init() wrapper in-place for any in-flight users, it can be\nremoved at some later date.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n apply.c               | 6 +++---\n archive.c             | 2 +-\n config.c              | 2 +-\n entry.c               | 4 ++--\n merge-ort.c           | 4 ++--\n merge-recursive.c     | 4 ++--\n refs/packed-backend.c | 2 +-\n transport.c           | 2 +-\n 8 files changed, 13 insertions(+), 13 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex 853d3ed385a..44bc31d6eb5 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -101,9 +101,9 @@ int init_apply_state(struct apply_state *state,\n \tstate->ws_error_action = warn_on_ws_error;\n \tstate->ws_ignore_action = ignore_ws_none;\n \tstate->linenr = 1;\n-\tstring_list_init(&state->fn_table, 0);\n-\tstring_list_init(&state->limit_by_name, 0);\n-\tstring_list_init(&state->symlink_changes, 0);\n+\tstring_list_init_nodup(&state->fn_table);\n+\tstring_list_init_nodup(&state->limit_by_name);\n+\tstring_list_init_nodup(&state->symlink_changes);\n \tstrbuf_init(&state->root, 0);\n \n \tgit_apply_config();\ndiff --git a/archive.c b/archive.c\nindex ff2bb54f622..3c266d1d7bf 100644\n--- a/archive.c\n+++ b/archive.c\n@@ -645,7 +645,7 @@ int write_archive(int argc, const char **argv, const char *prefix,\n \targs.pretty_ctx = &ctx;\n \targs.repo = repo;\n \targs.prefix = prefix;\n-\tstring_list_init(&args.extra_files, 1);\n+\tstring_list_init_dup(&args.extra_files);\n \targc = parse_archive_args(argc, argv, &ar, &args, name_hint, remote);\n \tif (!startup_info->have_repository) {\n \t\t/*\ndiff --git a/config.c b/config.c\nindex f9c400ad306..2edb282b43d 100644\n--- a/config.c\n+++ b/config.c\n@@ -2072,7 +2072,7 @@ static int configset_add_value(struct config_set *cs, const char *key, const cha\n \t\te = xmalloc(sizeof(*e));\n \t\thashmap_entry_init(&e->ent, strhash(key));\n \t\te->key = xstrdup(key);\n-\t\tstring_list_init(&e->value_list, 1);\n+\t\tstring_list_init_dup(&e->value_list);\n \t\thashmap_add(&cs->config_hash, &e->ent);\n \t}\n \tsi = string_list_append_nodup(&e->value_list, xstrdup_or_null(value));\ndiff --git a/entry.c b/entry.c\nindex 711ee0693c7..125fabdbd52 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -143,8 +143,8 @@ void enable_delayed_checkout(struct checkout *state)\n \tif (!state->delayed_checkout) {\n \t\tstate->delayed_checkout = xmalloc(sizeof(*state->delayed_checkout));\n \t\tstate->delayed_checkout->state = CE_CAN_DELAY;\n-\t\tstring_list_init(&state->delayed_checkout->filters, 0);\n-\t\tstring_list_init(&state->delayed_checkout->paths, 0);\n+\t\tstring_list_init_nodup(&state->delayed_checkout->filters);\n+\t\tstring_list_init_nodup(&state->delayed_checkout->paths);\n \t}\n }\n \ndiff --git a/merge-ort.c b/merge-ort.c\nindex b954f7184a5..d7ae8d0558f 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -1836,7 +1836,7 @@ static void compute_collisions(struct strmap *collisions,\n \t\t\tfree(new_path);\n \t\t} else {\n \t\t\tCALLOC_ARRAY(collision_info, 1);\n-\t\t\tstring_list_init(&collision_info->source_files, 0);\n+\t\t\tstring_list_init_nodup(&collision_info->source_files);\n \t\t\tstrmap_put(collisions, new_path, collision_info);\n \t\t}\n \t\tstring_list_insert(&collision_info->source_files,\n@@ -3942,7 +3942,7 @@ static void merge_start(struct merge_options *opt, struct merge_result *result)\n \t */\n \tstrmap_init_with_options(&opt->priv->paths, NULL, 0);\n \tstrmap_init_with_options(&opt->priv->conflicted, NULL, 0);\n-\tstring_list_init(&opt->priv->paths_to_free, 0);\n+\tstring_list_init_nodup(&opt->priv->paths_to_free);\n \n \t/*\n \t * keys & strbufs in output will sometimes need to outlive \"paths\",\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex d146bb116f7..be473f854be 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -121,7 +121,7 @@ static void dir_rename_entry_init(struct dir_rename_entry *entry,\n \tentry->dir = directory;\n \tentry->non_unique_new_dir = 0;\n \tstrbuf_init(&entry->new_dir, 0);\n-\tstring_list_init(&entry->possible_new_dirs, 0);\n+\tstring_list_init_nodup(&entry->possible_new_dirs);\n }\n \n struct collision_entry {\n@@ -3703,7 +3703,7 @@ static int merge_start(struct merge_options *opt, struct tree *head)\n \t}\n \n \tCALLOC_ARRAY(opt->priv, 1);\n-\tstring_list_init(&opt->priv->df_conflict_file_set, 1);\n+\tstring_list_init_dup(&opt->priv->df_conflict_file_set);\n \treturn 0;\n }\n \ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex dfecdbc1db6..5f50def076c 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -1425,7 +1425,7 @@ static int packed_transaction_prepare(struct ref_store *ref_store,\n \t */\n \n \tCALLOC_ARRAY(data, 1);\n-\tstring_list_init(&data->updates, 0);\n+\tstring_list_init_nodup(&data->updates);\n \n \ttransaction->backend_data = data;\n \ndiff --git a/transport.c b/transport.c\nindex 50f5830eb6b..25f77ce6ab2 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -1052,7 +1052,7 @@ struct transport *transport_get(struct remote *remote, const char *url)\n \tstruct transport *ret = xcalloc(1, sizeof(*ret));\n \n \tret->progress = isatty(2);\n-\tstring_list_init(&ret->pack_lockfiles, 1);\n+\tstring_list_init_dup(&ret->pack_lockfiles);\n \n \tif (!remote)\n \t\tBUG(\"No remote provided to transport_get()\");\n-- \n2.32.0.623.ge833f40cd87\n\n"},{"id":"428884","messageId":"patch-4.5-b05a393b5f1-20210701T104855Z-avarab@gmail.com","threadId":"56036","inReplyTo":"cover-0.5-00000000000-20210701T104855Z-avarab@gmail.com","subject":"[PATCH 4/5] string-list.[ch]: add a string_list_init_{nodup,dup}()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-01T10:51:28Z","receivedAt":"2021-07-01T10:51:43Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"In order to use the new \"memcpy() a 'blank' struct on the stack\"\npattern for string_list_init(), and to make the macro initialization\nconsistent with the function initialization introduce two new\nstring_list_init_{nodup,dup}() functions. These are like the old\nstring_list_init() when called with a false and true second argument,\nrespectively.\n\nI think this not only makes things more consistent, but also easier to\nread. I often had to lookup what the \", 0)\" or \", 1)\" in these\ninvocations meant, now it's right there in the function name, and\ncorresponds to the macros.\n\nA subsequent commit will convert existing API users to this pattern,\nbut as this is a very common API let's leave a compatibility function\nin place for later removal. This intermediate state also proves that\nthe compatibility function works.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n string-list.c | 18 ++++++++++++++++--\n string-list.h | 11 +++++++++--\n 2 files changed, 25 insertions(+), 4 deletions(-)\n\ndiff --git a/string-list.c b/string-list.c\nindex a917955fbd8..43576ad1265 100644\n--- a/string-list.c\n+++ b/string-list.c\n@@ -1,10 +1,24 @@\n #include \"cache.h\"\n #include \"string-list.h\"\n \n+void string_list_init_nodup(struct string_list *list)\n+{\n+\tstruct string_list blank = STRING_LIST_INIT_NODUP;\n+\tmemcpy(list, &blank, sizeof(*list));\n+}\n+\n+void string_list_init_dup(struct string_list *list)\n+{\n+\tstruct string_list blank = STRING_LIST_INIT_DUP;\n+\tmemcpy(list, &blank, sizeof(*list));\n+}\n+\n void string_list_init(struct string_list *list, int strdup_strings)\n {\n-\tmemset(list, 0, sizeof(*list));\n-\tlist->strdup_strings = strdup_strings;\n+\tif (strdup_strings)\n+\t\tstring_list_init_dup(list);\n+\telse\n+\t\tstring_list_init_nodup(list);\n }\n \n /* if there is no exact match, point to the index where the entry could be\ndiff --git a/string-list.h b/string-list.h\nindex 521b9c0748d..0d6b4692396 100644\n--- a/string-list.h\n+++ b/string-list.h\n@@ -97,8 +97,15 @@ struct string_list {\n /* General functions which work with both sorted and unsorted lists. */\n \n /**\n- * Initialize the members of the string_list, set `strdup_strings`\n- * member according to the value of the second parameter.\n+ * Initialize the members of a string_list pointer in the same way as\n+ * the corresponding `STRING_LIST_INIT_NODUP` and\n+ * `STRING_LIST_INIT_DUP` macros.\n+ */\n+void string_list_init_nodup(struct string_list *list);\n+void string_list_init_dup(struct string_list *list);\n+\n+/**\n+ * TODO remove: For compatibility with any in-flight older API users\n  */\n void string_list_init(struct string_list *list, int strdup_strings);\n \n-- \n2.32.0.623.ge833f40cd87\n\n"},{"id":"428939","messageId":"CAN0heSpsg=bwX9gbYfeWYqUbuen-zArR69FWxNmdtcQW_RubzA@mail.gmail.com","threadId":"56036","inReplyTo":"patch-1.5-9a92e7c216f-20210701T104855Z-avarab@gmail.com","subject":"Re: [PATCH 1/5] *.h: move some *_INIT to designated initializers","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2021-07-01T15:00:02Z","receivedAt":"2021-07-01T15:00:19Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Thu, 1 Jul 2021 at 12:54, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:\n>\n> Move *_INIT macros I'll use in a subsequent commits to designated\n> initializers. This isn't required for those follow-up changes, but\n> since I'm changing things in this are let's use the modern pattern\n> over the old one while we're at it.\n\ns/are/area/, maybe even s/area/area,/ on top.\n\n> -#define CREDENTIAL_INIT { STRING_LIST_INIT_DUP }\n> +#define CREDENTIAL_INIT { \\\n> +       .helpers = STRING_LIST_INIT_DUP, \\\n> +}\n\nI've verified that all of these designated initializers assign the exact\nsame fields before and after this commit, e.g., `helpers` actually is\nwhat comes first in the struct.\n\n> -#define STRING_LIST_INIT_NODUP { NULL, 0, 0, 0, NULL }\n> -#define STRING_LIST_INIT_DUP   { NULL, 0, 0, 1, NULL }\n> +#define STRING_LIST_INIT_NODUP { 0 }\n> +#define STRING_LIST_INIT_DUP   { .strdup_strings = 1 }\n\nThis \"NODUP\" one is a bit of an odd case though. You don't actually\nchange it to use designated initializers, as the patch says. Instead you\nchange it in a slightly different way. In a sense, you assign the\nliteral zero not to strdup_strings, but to the first field, which is a\npointer, where I would have expected NULL.\n\nI think there's been some recent-ish \"we can assign `{ 0 }` even if the\nfirst field is a pointer, because it's idiomatic and works out fine\"\neven if assigning 0 to a pointer looks a bit odd. So it might not be\nwrong as such, but it doesn't match what the commit message claims, and\nI think it would be more clear to use `{ .strdup_strings = 0 }` here:\nWe're explicitly interested in *not* duplicating the strings. It's not\njust some default \"let's leave everything as zero/NULL\".\n\nMartin\n"},{"id":"428945","messageId":"xmqq8s2qrps1.fsf@gitster.g","threadId":"56036","inReplyTo":"CAN0heSpsg=bwX9gbYfeWYqUbuen-zArR69FWxNmdtcQW_RubzA@mail.gmail.com","subject":"Re: [PATCH 1/5] *.h: move some *_INIT to designated initializers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-01T15:56:46Z","receivedAt":"2021-07-01T15:56:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren <martin.agren@gmail.com> writes:\n\n>> -#define STRING_LIST_INIT_NODUP { NULL, 0, 0, 0, NULL }\n>> +#define STRING_LIST_INIT_NODUP { 0 }\n>\n> This \"NODUP\" one is a bit of an odd case though. You don't actually\n> change it to use designated initializers, as the patch says. Instead you\n> change it in a slightly different way. In a sense, you assign the\n> literal zero not to strdup_strings, but to the first field, which is a\n> pointer, where I would have expected NULL.\n>\n> I think there's been some recent-ish \"we can assign `{ 0 }` even if the\n> first field is a pointer, because it's idiomatic and works out fine\"\n> even if assigning 0 to a pointer looks a bit odd. So it might not be\n> wrong as such, but it doesn't match what the commit message claims, and\n> I think it would be more clear to use `{ .strdup_strings = 0 }` here:\n> We're explicitly interested in *not* duplicating the strings. It's not\n> just some default \"let's leave everything as zero/NULL\".\n\nWell reasoned.  I agree that it is not wrong per-se, and the\nsolution to use designated initializer is indeed in line with the\ntheme of the topic.\n\n"},{"id":"428966","messageId":"YN3pmS8q3dA3Gnd+@coredump.intra.peff.net","threadId":"56036","inReplyTo":"cover-0.5-00000000000-20210701T104855Z-avarab@gmail.com","subject":"Re: [PATCH 0/5] *.[ch]: don't duplicate *_init() and *_INIT logic","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-07-01T16:13:13Z","receivedAt":"2021-07-01T16:13:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 01, 2021 at 12:51:24PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> This series implements a suggestion by Jeff King to use an idiom for\n> *_init() functions that avoids duplicating what we've declared in the\n> corresponding *_INIT macros. See\n> https://lore.kernel.org/git/YNytp0JAIaQih0Y4@coredump.intra.peff.net/\n\nThese all look pretty good to me. My comments are minor enough that I'll\ninclude them here rather than individual replies:\n\n> Ævar Arnfjörð Bjarmason (5):\n>   *.h: move some *_INIT to designated initializers\n\nI agree with Martin's suggestion to prefer the designated initializer\nfor STRING_LIST_INIT_NODUP. The consistency makes it easier to read.\n\n>   *.c *_init(): define in terms of corresponding *_INIT macro\n\nYep, nice.\n\n>   dir.[ch]: replace dir_init() with DIR_INIT\n\nI like this. Since there is a dir_clear(), I guess somebody could argue\nthat lacking an init function makes things less consistent/balanced. But\nif we don't need it, I'm happy to drop it.\n\n>   string-list.[ch]: add a string_list_init_{nodup,dup}()\n\nI like this. I agree the current \"0/1\" in the callers is kind of\ninscrutable. It does make things harder if we wanted to add more\noptions, but that is already true due to the macro initializers.\n\n>   string-list.h users: change to use *_{nodup,dup}()\n\nNice. IMHO you should drop the convenience wrapper here. Leaving it\nmeans we'll have to circle back later and fix up topics in flight (and\nit's not atomic; when we finally drop it, there may be more topics in\nflight then!).\n\nIf it were somehow complicated to convert callers, that might be more\njustified. But doing a quick conversion (which is easily detected by the\ncompiler) in the merge commit seems like less work than having to\nrevisit the topic later.\n\n-Peff\n"},{"id":"428968","messageId":"87lf6qc81m.fsf@evledraar.gmail.com","threadId":"56036","inReplyTo":"CAN0heSpsg=bwX9gbYfeWYqUbuen-zArR69FWxNmdtcQW_RubzA@mail.gmail.com","subject":"Re: [PATCH 1/5] *.h: move some *_INIT to designated initializers","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-01T16:08:01Z","receivedAt":"2021-07-01T16:28:58Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Jul 01 2021, Martin Ågren wrote:\n\n> On Thu, 1 Jul 2021 at 12:54, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:\n>>\n>> Move *_INIT macros I'll use in a subsequent commits to designated\n>> initializers. This isn't required for those follow-up changes, but\n>> since I'm changing things in this are let's use the modern pattern\n>> over the old one while we're at it.\n>\n> s/are/area/, maybe even s/area/area,/ on top.\n\nWill fix.\n\n>> -#define CREDENTIAL_INIT { STRING_LIST_INIT_DUP }\n>> +#define CREDENTIAL_INIT { \\\n>> +       .helpers = STRING_LIST_INIT_DUP, \\\n>> +}\n>\n> I've verified that all of these designated initializers assign the exact\n> same fields before and after this commit, e.g., `helpers` actually is\n> what comes first in the struct.\n\nThanks!\n\n>> -#define STRING_LIST_INIT_NODUP { NULL, 0, 0, 0, NULL }\n>> -#define STRING_LIST_INIT_DUP   { NULL, 0, 0, 1, NULL }\n>> +#define STRING_LIST_INIT_NODUP { 0 }\n>> +#define STRING_LIST_INIT_DUP   { .strdup_strings = 1 }\n>\n> This \"NODUP\" one is a bit of an odd case though. You don't actually\n> change it to use designated initializers, as the patch says. Instead you\n> change it in a slightly different way. In a sense, you assign the\n> literal zero not to strdup_strings, but to the first field, which is a\n> pointer, where I would have expected NULL.\n>\n> I think there's been some recent-ish \"we can assign `{ 0 }` even if the\n> first field is a pointer, because it's idiomatic and works out fine\"\n> even if assigning 0 to a pointer looks a bit odd. So it might not be\n> wrong as such, but it doesn't match what the commit message claims, and\n> I think it would be more clear to use `{ .strdup_strings = 0 }` here:\n> We're explicitly interested in *not* duplicating the strings. It's not\n> just some default \"let's leave everything as zero/NULL\".\n\nYes I'm really conflating a few different things here:\n\n 1. Whether to use designated initializers.\n\n 2. Whether to skip explicit initialization, i.e. not enumerate\n    everything, and have the ones not mentioned initializes as if they\n    had static storage duration.\n\n 3. Using the 0 null pointer constant instead of NULL while doing #2.\n\n 4. Using our in-codebase idiom of only setting any boolean flags during\n    initialization if we're setting them to true, \n\nSo I think in this case just keeping it as \"{ 0 }\" makes the most\nsense. Even better would be just a \"{}\", but that's a GNU-only\nextension.\n\nI think \"{ NULL }\" is just a distraction, i.e. it's better to use \"{ 0\n}\" consistently everywhere. C guarantees the same-ness of the two, and\nthe point is to initialize the struct as if though it were done with a\nmemset() to 0, not mentally keep track of whether the first member is an\nint, pointer or char * (in one other case we use '\\0').\n"}]}