{"thread":{"id":"54687","subject":"[PATCH 0/4] grep: retire `init_grep_defaults()`","startedAt":"2020-11-21T18:31:37Z","lastAt":"2020-12-01T04:47:03Z","messageCount":26,"participants":["Martin Ågren","Jeff King","Junio C Hamano","Johannes Schindelin","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"410462","messageId":"cover.1605972564.git.martin.agren@gmail.com","threadId":"54687","inReplyTo":null,"subject":"[PATCH 0/4] grep: retire `init_grep_defaults()`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-11-21T18:31:06Z","receivedAt":"2020-11-21T18:31:37Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Users of the grep machinery need to call `init_grep_defaults()` to\npopulate some defaults. There's a comment: \"We could let the compiler do\nthis, but without C99 initializers the code gets unwieldy and \nunreadable, so...\".\n\nWe have such initializers now, so we can simplify accordingly.\n\nMartin Ågren (4):\n  grep: don't set up a \"default\" repo for grep\n  grep: use designated initializers for `grep_defaults`\n  grep: simplify color setup\n  MyFirstObjectWalk: drop `init_walken_defaults()`\n\n Documentation/MyFirstObjectWalk.txt | 34 +------------\n grep.h                              |  1 -\n builtin/grep.c                      |  1 -\n builtin/log.c                       |  1 -\n grep.c                              | 74 ++++++++++-------------------\n revision.c                          |  1 -\n 6 files changed, 27 insertions(+), 85 deletions(-)\n\n-- \n2.29.2.454.gaff20da3a2\n\n"},{"id":"410463","messageId":"02c9c38f53cc6b1faa0b58e846f9f1f36c6a3df1.1605972564.git.martin.agren@gmail.com","threadId":"54687","inReplyTo":"cover.1605972564.git.martin.agren@gmail.com","subject":"[PATCH 1/4] grep: don't set up a \"default\" repo for grep","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-11-21T18:31:07Z","receivedAt":"2020-11-21T18:31:41Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"`init_grep_defaults()` fills a `static struct grep_opt grep_defaults`.\nThis struct is then used by `grep_init()` as a blueprint for other such\nstructs. Notably, `grep_init()` takes a `struct repo *` and assigns it\ninto the target struct.\n\nAs a result, it is unnecessary for us to take a `struct repo *` in\n`init_grep_defaults()` as well. We assign it into the default struct and\nnever look at it again. And in light of how we return early if we have\nalready set up the default struct, it's not just unnecessary, but is\nalso a bit confusing: If we are called twice and with different repos,\nis it a bug or a feature that we ignore the second repo?\n\nDrop the repo parameter for `init_grep_defaults()`.\n\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\n---\n Documentation/MyFirstObjectWalk.txt | 2 +-\n grep.h                              | 2 +-\n builtin/grep.c                      | 2 +-\n builtin/log.c                       | 2 +-\n grep.c                              | 3 +--\n revision.c                          | 2 +-\n 6 files changed, 6 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/MyFirstObjectWalk.txt b/Documentation/MyFirstObjectWalk.txt\nindex c3f2d1a831..85434d1938 100644\n--- a/Documentation/MyFirstObjectWalk.txt\n+++ b/Documentation/MyFirstObjectWalk.txt\n@@ -394,7 +394,7 @@ First some setup. Add `init_grep_defaults()` to `init_walken_defaults()` and add\n ----\n static void init_walken_defaults(void)\n {\n-\tinit_grep_defaults(the_repository);\n+\tinit_grep_defaults();\n }\n \n ...\ndiff --git a/grep.h b/grep.h\nindex 9115db8515..1c5478f381 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -170,7 +170,7 @@ struct grep_opt {\n \tvoid *output_priv;\n };\n \n-void init_grep_defaults(struct repository *);\n+void init_grep_defaults(void);\n int grep_config(const char *var, const char *value, void *);\n void grep_init(struct grep_opt *, struct repository *repo, const char *prefix);\n void grep_destroy(void);\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex e58e57504c..2b96efa8c2 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -950,7 +950,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\tOPT_END()\n \t};\n \n-\tinit_grep_defaults(the_repository);\n+\tinit_grep_defaults();\n \tgit_config(grep_cmd_config, NULL);\n \tgrep_init(&opt, the_repository, prefix);\n \ndiff --git a/builtin/log.c b/builtin/log.c\nindex 49eb8f6431..eee4beca4d 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -131,7 +131,7 @@ static int log_line_range_callback(const struct option *option, const char *arg,\n \n static void init_log_defaults(void)\n {\n-\tinit_grep_defaults(the_repository);\n+\tinit_grep_defaults();\n \tinit_diff_ui_defaults();\n \n \tdecoration_style = auto_decoration_style();\ndiff --git a/grep.c b/grep.c\nindex 54af9f813e..b351449f7f 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -57,7 +57,7 @@ static void color_set(char *dst, const char *color_bytes)\n  * We could let the compiler do this, but without C99 initializers\n  * the code gets unwieldy and unreadable, so...\n  */\n-void init_grep_defaults(struct repository *repo)\n+void init_grep_defaults(void)\n {\n \tstruct grep_opt *opt = &grep_defaults;\n \tstatic int run_once;\n@@ -67,7 +67,6 @@ void init_grep_defaults(struct repository *repo)\n \trun_once++;\n \n \tmemset(opt, 0, sizeof(*opt));\n-\topt->repo = repo;\n \topt->relative = 1;\n \topt->pathname = 1;\n \topt->max_depth = -1;\ndiff --git a/revision.c b/revision.c\nindex aa62212040..f35ea1db11 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1834,7 +1834,7 @@ void repo_init_revisions(struct repository *r,\n \trevs->commit_format = CMIT_FMT_DEFAULT;\n \trevs->expand_tabs_in_log_default = 8;\n \n-\tinit_grep_defaults(revs->repo);\n+\tinit_grep_defaults();\n \tgrep_init(&revs->grep_filter, revs->repo, prefix);\n \trevs->grep_filter.status_only = 1;\n \n-- \n2.29.2.454.gaff20da3a2\n\n"},{"id":"410464","messageId":"5aad7bb31f955a6adf41febfd16f2f483294f939.1605972564.git.martin.agren@gmail.com","threadId":"54687","inReplyTo":"cover.1605972564.git.martin.agren@gmail.com","subject":"[PATCH 2/4] grep: use designated initializers for `grep_defaults`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-11-21T18:31:08Z","receivedAt":"2020-11-21T18:31:44Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"In 15fabd1bbd (\"builtin/grep.c: make configuration callback more\nreusable\", 2012-10-09), we learned to fill a `static struct grep_opt\ngrep_defaults` which we can use as a blueprint for other such structs.\n\nAt the time, we didn't consider designated initializers to be widely\nuseable, but these days, we do. (See, e.g., cbc0f81d96 (\"strbuf: use\ndesignated initializers in STRBUF_INIT\", 2017-07-10).)\n\nUse designated initializers to let the compiler set up the struct and so\nthat we don't need to remember to call `init_grep_defaults()`.\n\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\n---\n If we've messed up and our color strings are too large for our buffers,\n we will no longer hit a BUG within `color_set()`, but we should be able\n to rely on the compiler detecting the truncation. We'll probably be\n *better* off than before, since the compiler will know exactly how\n large the buffer is.\n\n Documentation/MyFirstObjectWalk.txt | 10 +----\n grep.h                              |  1 -\n builtin/grep.c                      |  1 -\n builtin/log.c                       |  1 -\n grep.c                              | 64 +++++++++++------------------\n revision.c                          |  1 -\n 6 files changed, 26 insertions(+), 52 deletions(-)\n\ndiff --git a/Documentation/MyFirstObjectWalk.txt b/Documentation/MyFirstObjectWalk.txt\nindex 85434d1938..7f4bffc4dd 100644\n--- a/Documentation/MyFirstObjectWalk.txt\n+++ b/Documentation/MyFirstObjectWalk.txt\n@@ -388,17 +388,9 @@ Next, let's try to filter the commits we see based on their author. This is\n equivalent to running `git log --author=<pattern>`. We can add a filter by\n modifying `rev_info.grep_filter`, which is a `struct grep_opt`.\n \n-First some setup. Add `init_grep_defaults()` to `init_walken_defaults()` and add\n-`grep_config()` to `git_walken_config()`:\n+First some setup. Add `grep_config()` to `git_walken_config()`:\n \n ----\n-static void init_walken_defaults(void)\n-{\n-\tinit_grep_defaults();\n-}\n-\n-...\n-\n static int git_walken_config(const char *var, const char *value, void *cb)\n {\n \tgrep_config(var, value, cb);\ndiff --git a/grep.h b/grep.h\nindex 1c5478f381..b5c4e223a8 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -170,7 +170,6 @@ struct grep_opt {\n \tvoid *output_priv;\n };\n \n-void init_grep_defaults(void);\n int grep_config(const char *var, const char *value, void *);\n void grep_init(struct grep_opt *, struct repository *repo, const char *prefix);\n void grep_destroy(void);\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 2b96efa8c2..ca259af441 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -950,7 +950,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\tOPT_END()\n \t};\n \n-\tinit_grep_defaults();\n \tgit_config(grep_cmd_config, NULL);\n \tgrep_init(&opt, the_repository, prefix);\n \ndiff --git a/builtin/log.c b/builtin/log.c\nindex eee4beca4d..cf41714fb0 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -131,7 +131,6 @@ static int log_line_range_callback(const struct option *option, const char *arg,\n \n static void init_log_defaults(void)\n {\n-\tinit_grep_defaults();\n \tinit_diff_ui_defaults();\n \n \tdecoration_style = auto_decoration_style();\ndiff --git a/grep.c b/grep.c\nindex b351449f7f..8f2009ec9f 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -14,7 +14,31 @@ static int grep_source_load(struct grep_source *gs);\n static int grep_source_is_binary(struct grep_source *gs,\n \t\t\t\t struct index_state *istate);\n \n-static struct grep_opt grep_defaults;\n+static void std_output(struct grep_opt *opt, const void *buf, size_t size)\n+{\n+\tfwrite(buf, size, 1, stdout);\n+}\n+\n+static struct grep_opt grep_defaults = {\n+\t.relative = 1,\n+\t.pathname = 1,\n+\t.max_depth = -1,\n+\t.pattern_type_option = GREP_PATTERN_TYPE_UNSPECIFIED,\n+\t.colors = {\n+\t\t[GREP_COLOR_CONTEXT] = \"\",\n+\t\t[GREP_COLOR_FILENAME] = \"\",\n+\t\t[GREP_COLOR_FUNCTION] = \"\",\n+\t\t[GREP_COLOR_LINENO] = \"\",\n+\t\t[GREP_COLOR_COLUMNNO] = \"\",\n+\t\t[GREP_COLOR_MATCH_CONTEXT] = GIT_COLOR_BOLD_RED,\n+\t\t[GREP_COLOR_MATCH_SELECTED] = GIT_COLOR_BOLD_RED,\n+\t\t[GREP_COLOR_SELECTED] = \"\",\n+\t\t[GREP_COLOR_SEP] = GIT_COLOR_CYAN,\n+\t},\n+\t.only_matching = 0,\n+\t.color = -1,\n+\t.output = std_output,\n+};\n \n #ifdef USE_LIBPCRE2\n static pcre2_general_context *pcre2_global_context;\n@@ -42,49 +66,11 @@ static const char *color_grep_slots[] = {\n \t[GREP_COLOR_SEP]\t    = \"separator\",\n };\n \n-static void std_output(struct grep_opt *opt, const void *buf, size_t size)\n-{\n-\tfwrite(buf, size, 1, stdout);\n-}\n-\n static void color_set(char *dst, const char *color_bytes)\n {\n \txsnprintf(dst, COLOR_MAXLEN, \"%s\", color_bytes);\n }\n \n-/*\n- * Initialize the grep_defaults template with hardcoded defaults.\n- * We could let the compiler do this, but without C99 initializers\n- * the code gets unwieldy and unreadable, so...\n- */\n-void init_grep_defaults(void)\n-{\n-\tstruct grep_opt *opt = &grep_defaults;\n-\tstatic int run_once;\n-\n-\tif (run_once)\n-\t\treturn;\n-\trun_once++;\n-\n-\tmemset(opt, 0, sizeof(*opt));\n-\topt->relative = 1;\n-\topt->pathname = 1;\n-\topt->max_depth = -1;\n-\topt->pattern_type_option = GREP_PATTERN_TYPE_UNSPECIFIED;\n-\tcolor_set(opt->colors[GREP_COLOR_CONTEXT], \"\");\n-\tcolor_set(opt->colors[GREP_COLOR_FILENAME], \"\");\n-\tcolor_set(opt->colors[GREP_COLOR_FUNCTION], \"\");\n-\tcolor_set(opt->colors[GREP_COLOR_LINENO], \"\");\n-\tcolor_set(opt->colors[GREP_COLOR_COLUMNNO], \"\");\n-\tcolor_set(opt->colors[GREP_COLOR_MATCH_CONTEXT], GIT_COLOR_BOLD_RED);\n-\tcolor_set(opt->colors[GREP_COLOR_MATCH_SELECTED], GIT_COLOR_BOLD_RED);\n-\tcolor_set(opt->colors[GREP_COLOR_SELECTED], \"\");\n-\tcolor_set(opt->colors[GREP_COLOR_SEP], GIT_COLOR_CYAN);\n-\topt->only_matching = 0;\n-\topt->color = -1;\n-\topt->output = std_output;\n-}\n-\n static int parse_pattern_type_arg(const char *opt, const char *arg)\n {\n \tif (!strcmp(arg, \"default\"))\ndiff --git a/revision.c b/revision.c\nindex f35ea1db11..963868f699 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1834,7 +1834,6 @@ void repo_init_revisions(struct repository *r,\n \trevs->commit_format = CMIT_FMT_DEFAULT;\n \trevs->expand_tabs_in_log_default = 8;\n \n-\tinit_grep_defaults();\n \tgrep_init(&revs->grep_filter, revs->repo, prefix);\n \trevs->grep_filter.status_only = 1;\n \n-- \n2.29.2.454.gaff20da3a2\n\n"},{"id":"410465","messageId":"d6e43bc540bc682bb46d54e579a7101d0d2c462d.1605972564.git.martin.agren@gmail.com","threadId":"54687","inReplyTo":"cover.1605972564.git.martin.agren@gmail.com","subject":"[PATCH 3/4] grep: simplify color setup","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-11-21T18:31:09Z","receivedAt":"2020-11-21T18:31:46Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"The previous commit left us with only one user of the one-line wrapper\n`color_set()`. We could inline it, but note how we're `xsnprintf()`-ing\nall the entries in one array into another array of the same type. We\nmight as well just `memcpy()` everything into place.\n\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\n---\n Cc-ing Peff, who initially introduced this helper. After having inlined\n the function into the for loop, it seemed better to just copy the whole\n array. Happy to hear arguments against.\n\n Come to think of it, I suppose we could copy the whole struct and not\n just the color array. Hmmm...\n\n grep.c | 9 +--------\n 1 file changed, 1 insertion(+), 8 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 8f2009ec9f..9597cec67e 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -66,11 +66,6 @@ static const char *color_grep_slots[] = {\n \t[GREP_COLOR_SEP]\t    = \"separator\",\n };\n \n-static void color_set(char *dst, const char *color_bytes)\n-{\n-\txsnprintf(dst, COLOR_MAXLEN, \"%s\", color_bytes);\n-}\n-\n static int parse_pattern_type_arg(const char *opt, const char *arg)\n {\n \tif (!strcmp(arg, \"default\"))\n@@ -158,7 +153,6 @@ int grep_config(const char *var, const char *value, void *cb)\n void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix)\n {\n \tstruct grep_opt *def = &grep_defaults;\n-\tint i;\n \n #if defined(USE_LIBPCRE2)\n \tif (!pcre2_global_context)\n@@ -189,8 +183,7 @@ void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix\n \topt->relative = def->relative;\n \topt->output = def->output;\n \n-\tfor (i = 0; i < NR_GREP_COLORS; i++)\n-\t\tcolor_set(opt->colors[i], def->colors[i]);\n+\tmemcpy(opt->colors, def->colors, sizeof(def->colors));\n }\n \n void grep_destroy(void)\n-- \n2.29.2.454.gaff20da3a2\n\n"},{"id":"410466","messageId":"ca4523ce9e79654f2a51c0fee4909d84bd1f2bba.1605972564.git.martin.agren@gmail.com","threadId":"54687","inReplyTo":"cover.1605972564.git.martin.agren@gmail.com","subject":"[PATCH 4/4] MyFirstObjectWalk: drop `init_walken_defaults()`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-11-21T18:31:10Z","receivedAt":"2020-11-21T18:31:48Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"In a recent commit, we stopped calling `init_grep_defaults()` from this\nfunction. Thus, by the end of the tutorial, we still haven't added any\ncontents to this function. Let's remove it for simplicity.\n\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\n---\n Documentation/MyFirstObjectWalk.txt | 24 ------------------------\n 1 file changed, 24 deletions(-)\n\ndiff --git a/Documentation/MyFirstObjectWalk.txt b/Documentation/MyFirstObjectWalk.txt\nindex 7f4bffc4dd..2d10eea7a9 100644\n--- a/Documentation/MyFirstObjectWalk.txt\n+++ b/Documentation/MyFirstObjectWalk.txt\n@@ -182,30 +182,6 @@ its `init_log_defaults()` sets its own state (`decoration_style`) and asks\n `grep` and `diff` to initialize themselves by calling each of their\n initialization functions.\n \n-For our first example within `git walken`, we don't intend to use any other\n-components within Git, and we don't have any configuration to do.  However, we\n-may want to add some later, so for now, we can add an empty placeholder. Create\n-a new function in `builtin/walken.c`:\n-\n-----\n-static void init_walken_defaults(void)\n-{\n-\t/*\n-\t * We don't actually need the same components `git log` does; leave this\n-\t * empty for now.\n-\t */\n-}\n-----\n-\n-Make sure to add a line invoking it inside of `cmd_walken()`.\n-\n-----\n-int cmd_walken(int argc, const char **argv, const char *prefix)\n-{\n-\tinit_walken_defaults();\n-}\n-----\n-\n ==== Configuring From `.gitconfig`\n \n Next, we should have a look at any relevant configuration settings (i.e.,\n-- \n2.29.2.454.gaff20da3a2\n\n"},{"id":"410471","messageId":"20201121202310.GA972561@coredump.intra.peff.net","threadId":"54687","inReplyTo":"d6e43bc540bc682bb46d54e579a7101d0d2c462d.1605972564.git.martin.agren@gmail.com","subject":"Re: [PATCH 3/4] grep: simplify color setup","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-11-21T20:23:10Z","receivedAt":"2020-11-21T20:23:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Nov 21, 2020 at 07:31:09PM +0100, Martin Ågren wrote:\n\n> The previous commit left us with only one user of the one-line wrapper\n> `color_set()`. We could inline it, but note how we're `xsnprintf()`-ing\n> all the entries in one array into another array of the same type. We\n> might as well just `memcpy()` everything into place.\n> \n> Signed-off-by: Martin Ågren <martin.agren@gmail.com>\n> ---\n>  Cc-ing Peff, who initially introduced this helper. After having inlined\n>  the function into the for loop, it seemed better to just copy the whole\n>  array. Happy to hear arguments against.\n\nNo, this is way better than the existing code. I introduced it to get\naway from strcpy(), but this is better still. But...\n\n>  Come to think of it, I suppose we could copy the whole struct and not\n>  just the color array. Hmmm...\n\nYes, this seems even better. If our goal is just to start our new\ngrep_opt the same as grep_defaults, then a single-line struct copy\n(whether through assignment or memcpy) is even clearer and more\nmaintainable.\n\n-Peff\n"},{"id":"410472","messageId":"CAN0heSpNYpCGqgaa9LABG1T1LRa9jbGxvUucSSvrJG9ztHpXOg@mail.gmail.com","threadId":"54687","inReplyTo":"20201121202310.GA972561@coredump.intra.peff.net","subject":"Re: [PATCH 3/4] grep: simplify color setup","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-11-21T20:52:33Z","receivedAt":"2020-11-21T20:52:52Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Sat, 21 Nov 2020 at 21:23, Jeff King <peff@peff.net> wrote:\n>\n> On Sat, Nov 21, 2020 at 07:31:09PM +0100, Martin Ågren wrote:\n>> >  Cc-ing Peff, who initially introduced this helper. After having inlined\n> >  the function into the for loop, it seemed better to just copy the whole\n> >  array. Happy to hear arguments against.\n>\n> No, this is way better than the existing code. I introduced it to get\n> away from strcpy(), but this is better still. But...\n>\n> >  Come to think of it, I suppose we could copy the whole struct and not\n> >  just the color array. Hmmm...\n>\n> Yes, this seems even better. If our goal is just to start our new\n> grep_opt the same as grep_defaults, then a single-line struct copy\n> (whether through assignment or memcpy) is even clearer and more\n> maintainable.\n\nOk, thanks for the encouraging words. I couldn't keep myself from\nthinking that we're doing this for some weird ... performance reason?\nThanks for taking me out of that thought.\n\nI'll hold off for a while in case there's more feedback, then look into\nreplacing this patch with a more aggressive copy of the whole struct.\n\nMartin\n"},{"id":"410476","messageId":"xmqq4klis4zt.fsf@gitster.c.googlers.com","threadId":"54687","inReplyTo":"20201121202310.GA972561@coredump.intra.peff.net","subject":"Re: [PATCH 3/4] grep: simplify color setup","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-21T22:46:14Z","receivedAt":"2020-11-21T22:46:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Sat, Nov 21, 2020 at 07:31:09PM +0100, Martin Ågren wrote:\n>\n>> The previous commit left us with only one user of the one-line wrapper\n>> `color_set()`. We could inline it, but note how we're `xsnprintf()`-ing\n>> all the entries in one array into another array of the same type. We\n>> might as well just `memcpy()` everything into place.\n>> \n>> Signed-off-by: Martin Ågren <martin.agren@gmail.com>\n>> ---\n>>  Cc-ing Peff, who initially introduced this helper. After having inlined\n>>  the function into the for loop, it seemed better to just copy the whole\n>>  array. Happy to hear arguments against.\n>\n> No, this is way better than the existing code. I introduced it to get\n> away from strcpy(), but this is better still. But...\n\nYes, the copy in this patch looks eminently sensible.\n\n>>  Come to think of it, I suppose we could copy the whole struct and not\n>>  just the color array. Hmmm...\n>\n> Yes, this seems even better. If our goal is just to start our new\n> grep_opt the same as grep_defaults, then a single-line struct copy\n> (whether through assignment or memcpy) is even clearer and more\n> maintainable.\n\n... until such a time when typeof(grep_defaults) gains a field with\na non-const pointer value that we'd rather not to share amongst\ninstances of the type, at which point it no longer is clear win from\nmaintainability's point of view.\n"},{"id":"410519","messageId":"nycvar.QRO.7.76.6.2011231159340.56@tvgsbejvaqbjf.bet","threadId":"54687","inReplyTo":"cover.1605972564.git.martin.agren@gmail.com","subject":"Re: [PATCH 0/4] grep: retire `init_grep_defaults()`","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-11-23T11:03:54Z","receivedAt":"2020-11-23T11:04:10Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Martin,\n\nOn Sat, 21 Nov 2020, Martin Ågren wrote:\n\n> Users of the grep machinery need to call `init_grep_defaults()` to\n> populate some defaults. There's a comment: \"We could let the compiler do\n> this, but without C99 initializers the code gets unwieldy and\n> unreadable, so...\".\n>\n> We have such initializers now, so we can simplify accordingly.\n\nWhen I read this at first, I feared that you would change a\n`pthread_init_mutex()` to using `PTHREAD_MUTEX_INITIALIZER` instead. On\nthe face of it, that would look good on paper, but it unfortunately falls\nflat for us because there is no Win32 equivalent for the static\ninitializer: we emulate mutexes via `CRITICAL_SECTION`s, and those need to\nbe initialized by running a function.\n\nTo my surprise and relief, your patches do not do that; The only mutex in\n`grep.c` (`grep_attr_mutex`) is not touched.\n\nI just wanted to mention this here, in case anybody else had the same\nidea.\n\nThanks,\nDscho\n"},{"id":"410637","messageId":"X7yuE1M9GpnL0VbT@coredump.intra.peff.net","threadId":"54687","inReplyTo":"xmqq4klis4zt.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 3/4] grep: simplify color setup","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-11-24T06:54:11Z","receivedAt":"2020-11-24T06:54:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Nov 21, 2020 at 02:46:14PM -0800, Junio C Hamano wrote:\n\n> >>  Come to think of it, I suppose we could copy the whole struct and not\n> >>  just the color array. Hmmm...\n> >\n> > Yes, this seems even better. If our goal is just to start our new\n> > grep_opt the same as grep_defaults, then a single-line struct copy\n> > (whether through assignment or memcpy) is even clearer and more\n> > maintainable.\n> \n> ... until such a time when typeof(grep_defaults) gains a field with\n> a non-const pointer value that we'd rather not to share amongst\n> instances of the type, at which point it no longer is clear win from\n> maintainability's point of view.\n\nI don't think we are any worse off. Either way we need to add a\nspecial-case deep copy to the function (which should definitely happen\nin only one place; we do not want bare struct copies sprinkled around\nthe code).\n\n-Peff\n"},{"id":"410707","messageId":"cover.1606251357.git.martin.agren@gmail.com","threadId":"54687","inReplyTo":"cover.1605972564.git.martin.agren@gmail.com","subject":"[PATCH v2 0/4] grep: simplify \"grep defaults\" handling","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-11-24T21:04:12Z","receivedAt":"2020-11-24T21:04:49Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"This is v2 of my series [1] to simplify the setup and use of the `struct\ngrep_opt grep_defaults`. The only difference compared to v1 is in the\nthird patch which now drops more code in favor of copying the whole\nstruct in one go.\n\n[1] https://lore.kernel.org/git/cover.1605972564.git.martin.agren@gmail.com/\n\nMartin Ågren (4):\n  grep: don't set up a \"default\" repo for grep\n  grep: use designated initializers for `grep_defaults`\n  grep: copy struct in one fell swoop\n  MyFirstObjectWalk: drop `init_walken_defaults()`\n\n Documentation/MyFirstObjectWalk.txt | 34 +----------\n grep.h                              |  1 -\n builtin/grep.c                      |  1 -\n builtin/log.c                       |  1 -\n grep.c                              | 90 +++++++++--------------------\n revision.c                          |  1 -\n 6 files changed, 28 insertions(+), 100 deletions(-)\n\n-- \n2.29.2.454.gaff20da3a2\n\n"},{"id":"410708","messageId":"56c00645c55f0e2cb3d21b19e397f449968b869d.1606251357.git.martin.agren@gmail.com","threadId":"54687","inReplyTo":"cover.1606251357.git.martin.agren@gmail.com","subject":"[PATCH v2 1/4] grep: don't set up a \"default\" repo for grep","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-11-24T21:04:13Z","receivedAt":"2020-11-24T21:04:52Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"`init_grep_defaults()` fills a `static struct grep_opt grep_defaults`.\nThis struct is then used by `grep_init()` as a blueprint for other such\nstructs. Notably, `grep_init()` takes a `struct repo *` and assigns it\ninto the target struct.\n\nAs a result, it is unnecessary for us to take a `struct repo *` in\n`init_grep_defaults()` as well. We assign it into the default struct and\nnever look at it again. And in light of how we return early if we have\nalready set up the default struct, it's not just unnecessary, but is\nalso a bit confusing: If we are called twice and with different repos,\nis it a bug or a feature that we ignore the second repo?\n\nDrop the repo parameter for `init_grep_defaults()`.\n\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\n---\n Documentation/MyFirstObjectWalk.txt | 2 +-\n grep.h                              | 2 +-\n builtin/grep.c                      | 2 +-\n builtin/log.c                       | 2 +-\n grep.c                              | 3 +--\n revision.c                          | 2 +-\n 6 files changed, 6 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/MyFirstObjectWalk.txt b/Documentation/MyFirstObjectWalk.txt\nindex c3f2d1a831..85434d1938 100644\n--- a/Documentation/MyFirstObjectWalk.txt\n+++ b/Documentation/MyFirstObjectWalk.txt\n@@ -394,7 +394,7 @@ First some setup. Add `init_grep_defaults()` to `init_walken_defaults()` and add\n ----\n static void init_walken_defaults(void)\n {\n-\tinit_grep_defaults(the_repository);\n+\tinit_grep_defaults();\n }\n \n ...\ndiff --git a/grep.h b/grep.h\nindex 9115db8515..1c5478f381 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -170,7 +170,7 @@ struct grep_opt {\n \tvoid *output_priv;\n };\n \n-void init_grep_defaults(struct repository *);\n+void init_grep_defaults(void);\n int grep_config(const char *var, const char *value, void *);\n void grep_init(struct grep_opt *, struct repository *repo, const char *prefix);\n void grep_destroy(void);\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex e58e57504c..2b96efa8c2 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -950,7 +950,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\tOPT_END()\n \t};\n \n-\tinit_grep_defaults(the_repository);\n+\tinit_grep_defaults();\n \tgit_config(grep_cmd_config, NULL);\n \tgrep_init(&opt, the_repository, prefix);\n \ndiff --git a/builtin/log.c b/builtin/log.c\nindex 49eb8f6431..eee4beca4d 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -131,7 +131,7 @@ static int log_line_range_callback(const struct option *option, const char *arg,\n \n static void init_log_defaults(void)\n {\n-\tinit_grep_defaults(the_repository);\n+\tinit_grep_defaults();\n \tinit_diff_ui_defaults();\n \n \tdecoration_style = auto_decoration_style();\ndiff --git a/grep.c b/grep.c\nindex 54af9f813e..b351449f7f 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -57,7 +57,7 @@ static void color_set(char *dst, const char *color_bytes)\n  * We could let the compiler do this, but without C99 initializers\n  * the code gets unwieldy and unreadable, so...\n  */\n-void init_grep_defaults(struct repository *repo)\n+void init_grep_defaults(void)\n {\n \tstruct grep_opt *opt = &grep_defaults;\n \tstatic int run_once;\n@@ -67,7 +67,6 @@ void init_grep_defaults(struct repository *repo)\n \trun_once++;\n \n \tmemset(opt, 0, sizeof(*opt));\n-\topt->repo = repo;\n \topt->relative = 1;\n \topt->pathname = 1;\n \topt->max_depth = -1;\ndiff --git a/revision.c b/revision.c\nindex aa62212040..f35ea1db11 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1834,7 +1834,7 @@ void repo_init_revisions(struct repository *r,\n \trevs->commit_format = CMIT_FMT_DEFAULT;\n \trevs->expand_tabs_in_log_default = 8;\n \n-\tinit_grep_defaults(revs->repo);\n+\tinit_grep_defaults();\n \tgrep_init(&revs->grep_filter, revs->repo, prefix);\n \trevs->grep_filter.status_only = 1;\n \n-- \n2.29.2.454.gaff20da3a2\n\n"},{"id":"410709","messageId":"c33837f0828acad539e7faef4c36768949d1ac56.1606251358.git.martin.agren@gmail.com","threadId":"54687","inReplyTo":"cover.1606251357.git.martin.agren@gmail.com","subject":"[PATCH v2 2/4] grep: use designated initializers for `grep_defaults`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-11-24T21:04:14Z","receivedAt":"2020-11-24T21:04:55Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"In 15fabd1bbd (\"builtin/grep.c: make configuration callback more\nreusable\", 2012-10-09), we learned to fill a `static struct grep_opt\ngrep_defaults` which we can use as a blueprint for other such structs.\n\nAt the time, we didn't consider designated initializers to be widely\nuseable, but these days, we do. (See, e.g., cbc0f81d96 (\"strbuf: use\ndesignated initializers in STRBUF_INIT\", 2017-07-10).)\n\nUse designated initializers to let the compiler set up the struct and so\nthat we don't need to remember to call `init_grep_defaults()`.\n\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\n---\n Documentation/MyFirstObjectWalk.txt | 10 +----\n grep.h                              |  1 -\n builtin/grep.c                      |  1 -\n builtin/log.c                       |  1 -\n grep.c                              | 64 +++++++++++------------------\n revision.c                          |  1 -\n 6 files changed, 26 insertions(+), 52 deletions(-)\n\ndiff --git a/Documentation/MyFirstObjectWalk.txt b/Documentation/MyFirstObjectWalk.txt\nindex 85434d1938..7f4bffc4dd 100644\n--- a/Documentation/MyFirstObjectWalk.txt\n+++ b/Documentation/MyFirstObjectWalk.txt\n@@ -388,17 +388,9 @@ Next, let's try to filter the commits we see based on their author. This is\n equivalent to running `git log --author=<pattern>`. We can add a filter by\n modifying `rev_info.grep_filter`, which is a `struct grep_opt`.\n \n-First some setup. Add `init_grep_defaults()` to `init_walken_defaults()` and add\n-`grep_config()` to `git_walken_config()`:\n+First some setup. Add `grep_config()` to `git_walken_config()`:\n \n ----\n-static void init_walken_defaults(void)\n-{\n-\tinit_grep_defaults();\n-}\n-\n-...\n-\n static int git_walken_config(const char *var, const char *value, void *cb)\n {\n \tgrep_config(var, value, cb);\ndiff --git a/grep.h b/grep.h\nindex 1c5478f381..b5c4e223a8 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -170,7 +170,6 @@ struct grep_opt {\n \tvoid *output_priv;\n };\n \n-void init_grep_defaults(void);\n int grep_config(const char *var, const char *value, void *);\n void grep_init(struct grep_opt *, struct repository *repo, const char *prefix);\n void grep_destroy(void);\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 2b96efa8c2..ca259af441 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -950,7 +950,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\tOPT_END()\n \t};\n \n-\tinit_grep_defaults();\n \tgit_config(grep_cmd_config, NULL);\n \tgrep_init(&opt, the_repository, prefix);\n \ndiff --git a/builtin/log.c b/builtin/log.c\nindex eee4beca4d..cf41714fb0 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -131,7 +131,6 @@ static int log_line_range_callback(const struct option *option, const char *arg,\n \n static void init_log_defaults(void)\n {\n-\tinit_grep_defaults();\n \tinit_diff_ui_defaults();\n \n \tdecoration_style = auto_decoration_style();\ndiff --git a/grep.c b/grep.c\nindex b351449f7f..8f2009ec9f 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -14,7 +14,31 @@ static int grep_source_load(struct grep_source *gs);\n static int grep_source_is_binary(struct grep_source *gs,\n \t\t\t\t struct index_state *istate);\n \n-static struct grep_opt grep_defaults;\n+static void std_output(struct grep_opt *opt, const void *buf, size_t size)\n+{\n+\tfwrite(buf, size, 1, stdout);\n+}\n+\n+static struct grep_opt grep_defaults = {\n+\t.relative = 1,\n+\t.pathname = 1,\n+\t.max_depth = -1,\n+\t.pattern_type_option = GREP_PATTERN_TYPE_UNSPECIFIED,\n+\t.colors = {\n+\t\t[GREP_COLOR_CONTEXT] = \"\",\n+\t\t[GREP_COLOR_FILENAME] = \"\",\n+\t\t[GREP_COLOR_FUNCTION] = \"\",\n+\t\t[GREP_COLOR_LINENO] = \"\",\n+\t\t[GREP_COLOR_COLUMNNO] = \"\",\n+\t\t[GREP_COLOR_MATCH_CONTEXT] = GIT_COLOR_BOLD_RED,\n+\t\t[GREP_COLOR_MATCH_SELECTED] = GIT_COLOR_BOLD_RED,\n+\t\t[GREP_COLOR_SELECTED] = \"\",\n+\t\t[GREP_COLOR_SEP] = GIT_COLOR_CYAN,\n+\t},\n+\t.only_matching = 0,\n+\t.color = -1,\n+\t.output = std_output,\n+};\n \n #ifdef USE_LIBPCRE2\n static pcre2_general_context *pcre2_global_context;\n@@ -42,49 +66,11 @@ static const char *color_grep_slots[] = {\n \t[GREP_COLOR_SEP]\t    = \"separator\",\n };\n \n-static void std_output(struct grep_opt *opt, const void *buf, size_t size)\n-{\n-\tfwrite(buf, size, 1, stdout);\n-}\n-\n static void color_set(char *dst, const char *color_bytes)\n {\n \txsnprintf(dst, COLOR_MAXLEN, \"%s\", color_bytes);\n }\n \n-/*\n- * Initialize the grep_defaults template with hardcoded defaults.\n- * We could let the compiler do this, but without C99 initializers\n- * the code gets unwieldy and unreadable, so...\n- */\n-void init_grep_defaults(void)\n-{\n-\tstruct grep_opt *opt = &grep_defaults;\n-\tstatic int run_once;\n-\n-\tif (run_once)\n-\t\treturn;\n-\trun_once++;\n-\n-\tmemset(opt, 0, sizeof(*opt));\n-\topt->relative = 1;\n-\topt->pathname = 1;\n-\topt->max_depth = -1;\n-\topt->pattern_type_option = GREP_PATTERN_TYPE_UNSPECIFIED;\n-\tcolor_set(opt->colors[GREP_COLOR_CONTEXT], \"\");\n-\tcolor_set(opt->colors[GREP_COLOR_FILENAME], \"\");\n-\tcolor_set(opt->colors[GREP_COLOR_FUNCTION], \"\");\n-\tcolor_set(opt->colors[GREP_COLOR_LINENO], \"\");\n-\tcolor_set(opt->colors[GREP_COLOR_COLUMNNO], \"\");\n-\tcolor_set(opt->colors[GREP_COLOR_MATCH_CONTEXT], GIT_COLOR_BOLD_RED);\n-\tcolor_set(opt->colors[GREP_COLOR_MATCH_SELECTED], GIT_COLOR_BOLD_RED);\n-\tcolor_set(opt->colors[GREP_COLOR_SELECTED], \"\");\n-\tcolor_set(opt->colors[GREP_COLOR_SEP], GIT_COLOR_CYAN);\n-\topt->only_matching = 0;\n-\topt->color = -1;\n-\topt->output = std_output;\n-}\n-\n static int parse_pattern_type_arg(const char *opt, const char *arg)\n {\n \tif (!strcmp(arg, \"default\"))\ndiff --git a/revision.c b/revision.c\nindex f35ea1db11..963868f699 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1834,7 +1834,6 @@ void repo_init_revisions(struct repository *r,\n \trevs->commit_format = CMIT_FMT_DEFAULT;\n \trevs->expand_tabs_in_log_default = 8;\n \n-\tinit_grep_defaults();\n \tgrep_init(&revs->grep_filter, revs->repo, prefix);\n \trevs->grep_filter.status_only = 1;\n \n-- \n2.29.2.454.gaff20da3a2\n\n"},{"id":"410710","messageId":"359355fb4eff6d99cb1baad9b72ff96e7dcda51d.1606251358.git.martin.agren@gmail.com","threadId":"54687","inReplyTo":"cover.1606251357.git.martin.agren@gmail.com","subject":"[PATCH v2 3/4] grep: copy struct in one fell swoop","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-11-24T21:04:15Z","receivedAt":"2020-11-24T21:04:57Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"We have a `struct grep_opt` with our defaults which we then copy into\nthe caller's struct. Rather than zeroing the target struct and copying\neach element one by one, just copy everything at once. This leaves the\ncode simpler and more maintainable.\n\nWe don't have any ownership issues with what we're copying now and can\njust greedily copy the whole thing. If and when we do need to handle\nsuch elements (`char *`?), we must and can handle it appropriately. This\ncommit doesn't really change that.\n\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\n---\n grep.c | 25 ++-----------------------\n 1 file changed, 2 insertions(+), 23 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 8f2009ec9f..7d740452cd 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -66,11 +66,6 @@ static const char *color_grep_slots[] = {\n \t[GREP_COLOR_SEP]\t    = \"separator\",\n };\n \n-static void color_set(char *dst, const char *color_bytes)\n-{\n-\txsnprintf(dst, COLOR_MAXLEN, \"%s\", color_bytes);\n-}\n-\n static int parse_pattern_type_arg(const char *opt, const char *arg)\n {\n \tif (!strcmp(arg, \"default\"))\n@@ -157,9 +152,6 @@ int grep_config(const char *var, const char *value, void *cb)\n  */\n void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix)\n {\n-\tstruct grep_opt *def = &grep_defaults;\n-\tint i;\n-\n #if defined(USE_LIBPCRE2)\n \tif (!pcre2_global_context)\n \t\tpcre2_global_context = pcre2_general_context_create(\n@@ -171,26 +163,13 @@ void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix\n \tpcre_free = free;\n #endif\n \n-\tmemset(opt, 0, sizeof(*opt));\n+\t*opt = grep_defaults;\n+\n \topt->repo = repo;\n \topt->prefix = prefix;\n \topt->prefix_length = (prefix && *prefix) ? strlen(prefix) : 0;\n \topt->pattern_tail = &opt->pattern_list;\n \topt->header_tail = &opt->header_list;\n-\n-\topt->only_matching = def->only_matching;\n-\topt->color = def->color;\n-\topt->extended_regexp_option = def->extended_regexp_option;\n-\topt->pattern_type_option = def->pattern_type_option;\n-\topt->linenum = def->linenum;\n-\topt->columnnum = def->columnnum;\n-\topt->max_depth = def->max_depth;\n-\topt->pathname = def->pathname;\n-\topt->relative = def->relative;\n-\topt->output = def->output;\n-\n-\tfor (i = 0; i < NR_GREP_COLORS; i++)\n-\t\tcolor_set(opt->colors[i], def->colors[i]);\n }\n \n void grep_destroy(void)\n-- \n2.29.2.454.gaff20da3a2\n\n"},{"id":"410711","messageId":"c2af3639239329675f61bb06380fdc77d0d4eb3b.1606251358.git.martin.agren@gmail.com","threadId":"54687","inReplyTo":"cover.1606251357.git.martin.agren@gmail.com","subject":"[PATCH v2 4/4] MyFirstObjectWalk: drop `init_walken_defaults()`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-11-24T21:04:16Z","receivedAt":"2020-11-24T21:04:58Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"In a recent commit, we stopped calling `init_grep_defaults()` from this\nfunction. Thus, by the end of the tutorial, we still haven't added any\ncontents to this function. Let's remove it for simplicity.\n\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\n---\n Documentation/MyFirstObjectWalk.txt | 24 ------------------------\n 1 file changed, 24 deletions(-)\n\ndiff --git a/Documentation/MyFirstObjectWalk.txt b/Documentation/MyFirstObjectWalk.txt\nindex 7f4bffc4dd..2d10eea7a9 100644\n--- a/Documentation/MyFirstObjectWalk.txt\n+++ b/Documentation/MyFirstObjectWalk.txt\n@@ -182,30 +182,6 @@ its `init_log_defaults()` sets its own state (`decoration_style`) and asks\n `grep` and `diff` to initialize themselves by calling each of their\n initialization functions.\n \n-For our first example within `git walken`, we don't intend to use any other\n-components within Git, and we don't have any configuration to do.  However, we\n-may want to add some later, so for now, we can add an empty placeholder. Create\n-a new function in `builtin/walken.c`:\n-\n-----\n-static void init_walken_defaults(void)\n-{\n-\t/*\n-\t * We don't actually need the same components `git log` does; leave this\n-\t * empty for now.\n-\t */\n-}\n-----\n-\n-Make sure to add a line invoking it inside of `cmd_walken()`.\n-\n-----\n-int cmd_walken(int argc, const char **argv, const char *prefix)\n-{\n-\tinit_walken_defaults();\n-}\n-----\n-\n ==== Configuring From `.gitconfig`\n \n Next, we should have a look at any relevant configuration settings (i.e.,\n-- \n2.29.2.454.gaff20da3a2\n\n"},{"id":"410726","messageId":"xmqqsg8ygza7.fsf@gitster.c.googlers.com","threadId":"54687","inReplyTo":"359355fb4eff6d99cb1baad9b72ff96e7dcda51d.1606251358.git.martin.agren@gmail.com","subject":"Re: [PATCH v2 3/4] grep: copy struct in one fell swoop","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-24T22:34:08Z","receivedAt":"2020-11-24T22:34:34Z","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> We have a `struct grep_opt` with our defaults which we then copy into\n> the caller's struct. Rather than zeroing the target struct and copying\n> each element one by one, just copy everything at once. This leaves the\n> code simpler and more maintainable.\n>\n> We don't have any ownership issues with what we're copying now and can\n> just greedily copy the whole thing. If and when we do need to handle\n> such elements (`char *`?), we must and can handle it appropriately.\n\nThat is correct, but ...\n\n> This\n> commit doesn't really change that.\n\n... I suspect this is not.\n\nIn the original code, those who are adding a new field would notice\nthat it is not copied over to the new instance (because they didn't\nadd anything to grep_init() to copy the field) and at that point\nthey must stop and think how the new field need to be copied.\n\nThe structure assignment of the outer shell done in this patch means\nthey are robbed of the opportunity to stop and think, because most\nof the time it \"works\" out of the box.  I'd feel safer if we left a\nclue to future developers if we were to do your clean-up, perhaps\nlike:\n\ndiff --git c/grep.h w/grep.h\nindex b5c4e223a8..388d226da3 100644\n--- c/grep.h\n+++ w/grep.h\n@@ -115,6 +115,14 @@ struct grep_expr {\n \t} u;\n };\n \n+/*\n+ * grep_config() initializes one \"default\" instance of this type, and\n+ * it is copied by grep_init() to be used by each individual\n+ * invocation.  When adding a new field to this structure that is\n+ * populated from the configuration, be sure to think about ownership\n+ * (i.e. a shallow copy may not be what you want for the type of your\n+ * newly added field).\n+ */\n struct grep_opt {\n \tstruct grep_pat *pattern_list;\n \tstruct grep_pat **pattern_tail;\n\n"},{"id":"410764","messageId":"CAN0heSoM+qQe8BdKHVpqhA0RAqzyyL3Qr98G=O8kD504diruCg@mail.gmail.com","threadId":"54687","inReplyTo":"xmqqsg8ygza7.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 3/4] grep: copy struct in one fell swoop","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-11-25T06:25:07Z","receivedAt":"2020-11-25T06:25:41Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Tue, 24 Nov 2020 at 23:34, Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Martin Ågren <martin.agren@gmail.com> writes:\n>\n> > We don't have any ownership issues with what we're copying now and can\n> > just greedily copy the whole thing. If and when we do need to handle\n> > such elements (`char *`?), we must and can handle it appropriately.\n>\n> That is correct, but ...\n>\n> > This\n> > commit doesn't really change that.\n>\n> ... I suspect this is not.\n>\n> In the original code, those who are adding a new field would notice\n> that it is not copied over to the new instance (because they didn't\n> add anything to grep_init() to copy the field) and at that point\n> they must stop and think how the new field need to be copied.\n>\n> The structure assignment of the outer shell done in this patch means\n> they are robbed of the opportunity to stop and think, because most\n> of the time it \"works\" out of the box.  I'd feel safer if we left a\n> clue to future developers if we were to do your clean-up, perhaps\n> like:\n>\n> diff --git c/grep.h w/grep.h\n> index b5c4e223a8..388d226da3 100644\n> --- c/grep.h\n> +++ w/grep.h\n> @@ -115,6 +115,14 @@ struct grep_expr {\n>         } u;\n>  };\n>\n> +/*\n> + * grep_config() initializes one \"default\" instance of this type, and\n> + * it is copied by grep_init() to be used by each individual\n> + * invocation.  When adding a new field to this structure that is\n> + * populated from the configuration, be sure to think about ownership\n> + * (i.e. a shallow copy may not be what you want for the type of your\n> + * newly added field).\n> + */\n>  struct grep_opt {\n>         struct grep_pat *pattern_list;\n>         struct grep_pat **pattern_tail;\n\nOk, that makes sense. Maybe put it in `grep_config()` though? We can add\nanything we want to to this struct and initialize it from the command\nline. It's when we start pre-filling it in `grep_config()` that we need\nto think about this. What do you think? We could also do both of\ncourse to really hedge our bets...\n\n  /*\n   * The instance of grep_opt that we set up here is copied by\n   * grep_init() to be used by each individual invocation.\n   * When populating a new field of this structure here,\n   * be sure to think about ownership (i.e. a shallow copy in\n   * grep_init() may not be what you want).\n   */\n\nThanks\nMartin\n"},{"id":"410767","messageId":"xmqqlfepg9dq.fsf@gitster.c.googlers.com","threadId":"54687","inReplyTo":"CAN0heSoM+qQe8BdKHVpqhA0RAqzyyL3Qr98G=O8kD504diruCg@mail.gmail.com","subject":"Re: [PATCH v2 3/4] grep: copy struct in one fell swoop","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-25T07:53:37Z","receivedAt":"2020-11-25T07:53:44Z","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> On Tue, 24 Nov 2020 at 23:34, Junio C Hamano <gitster@pobox.com> wrote:\n>>\n>> +/*\n>> + * grep_config() initializes one \"default\" instance of this type, and\n>> + * it is copied by grep_init() to be used by each individual\n>> + * invocation.  When adding a new field to this structure that is\n>> + * populated from the configuration, be sure to think about ownership\n>> + * (i.e. a shallow copy may not be what you want for the type of your\n>> + * newly added field).\n>> + */\n>>  struct grep_opt {\n>>         struct grep_pat *pattern_list;\n>>         struct grep_pat **pattern_tail;\n>\n> Ok, that makes sense. Maybe put it in `grep_config()` though? We can add\n> anything we want to to this struct and initialize it from the command\n> line. It's when we start pre-filling it in `grep_config()` that we need\n> to think about this. What do you think? We could also do both of\n> course to really hedge our bets...\n\nI agree with you that it would be the most helpful to have the\ncomment near grep_config(), as that function is what defines the\ndesign of populating the singleton to be copied by the instance\nused by individual invocation.\n\n>   /*\n>    * The instance of grep_opt that we set up here is copied by\n>    * grep_init() to be used by each individual invocation.\n>    * When populating a new field of this structure here,\n>    * be sure to think about ownership (i.e. a shallow copy in\n>    * grep_init() may not be what you want).\n>    */\n\nI find the text near the end of both my version and yours a bit\nunsatisfying.  One thing I care about is not to mislead readers to\nthink that the way grep_init() copies the singleton template is\ncorrect and sacred and they need to design their data structure to\nbe compatible with the shallow copying.  We'd want it to be clear\nthat it is expected that they will deep copy the field, and release\nit once individual invocation is done, when they need a new field\nthat won't work well with shallow copying.  Perhaps \"may not be wnat\nyou want\" is explicit enough, but I dunno.\n\nThanks.\n\n\n\n"},{"id":"410776","messageId":"87wny9okg6.fsf@evledraar.gmail.com","threadId":"54687","inReplyTo":"cover.1606251357.git.martin.agren@gmail.com","subject":"Re: [PATCH v2 0/4] grep: simplify \"grep defaults\" handling","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2020-11-25T09:27:21Z","receivedAt":"2020-11-25T09:27:41Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Nov 24 2020, Martin Ågren wrote:\n\n> This is v2 of my series [1] to simplify the setup and use of the `struct\n> grep_opt grep_defaults`. The only difference compared to v1 is in the\n> third patch which now drops more code in favor of copying the whole\n> struct in one go.\n\nIt's good to see this change, I remembered having to juggle these values\na while back in:\n\n    git log -p --author=Ævar --grep=memset -- grep.c\n\nThanks.\n"},{"id":"410901","messageId":"CAN0heSp2L4Q0pcn8V5hAKzXF6szZgpGqncLNYZOq5tmM7EucOQ@mail.gmail.com","threadId":"54687","inReplyTo":"xmqqlfepg9dq.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 3/4] grep: copy struct in one fell swoop","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-11-26T20:25:57Z","receivedAt":"2020-11-26T20:27:05Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Wed, 25 Nov 2020 at 08:53, Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Martin Ågren <martin.agren@gmail.com> writes:\n>\n> >   /*\n> >    * The instance of grep_opt that we set up here is copied by\n> >    * grep_init() to be used by each individual invocation.\n> >    * When populating a new field of this structure here,\n> >    * be sure to think about ownership (i.e. a shallow copy in\n> >    * grep_init() may not be what you want).\n> >    */\n>\n> I find the text near the end of both my version and yours a bit\n> unsatisfying.  One thing I care about is not to mislead readers to\n> think that the way grep_init() copies the singleton template is\n> correct and sacred and they need to design their data structure to\n> be compatible with the shallow copying.  We'd want it to be clear\n> that it is expected that they will deep copy the field, and release\n> it once individual invocation is done, when they need a new field\n> that won't work well with shallow copying.  Perhaps \"may not be wnat\n> you want\" is explicit enough, but I dunno.\n\nI understand your concern. Here's what I'm considering using:\n\n  /*\n   * The instance of grep_opt that we set up here is copied by\n   * grep_init() to be used by each individual invocation.\n   * When populating a new field of this structure here, be\n   * sure to think about ownership -- e.g., you might need to\n   * override the shallow copy in grep_init() with a deep copy.\n   */\n\nI'm not going into the releasing of those resources, but hopefully\nthat part can be left to the reader.\n\nOther suggestions welcome, of course. I hope to get around to rerolling\nthis on the weekend.\n\nMartin\n"},{"id":"410982","messageId":"1d3878799f8260968ea9f6a75a92c4daca1da133.1606679312.git.martin.agren@gmail.com","threadId":"54687","inReplyTo":"cover.1606679312.git.martin.agren@gmail.com","subject":"[PATCH v3 1/4] grep: don't set up a \"default\" repo for grep","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-11-29T19:52:19Z","receivedAt":"2020-11-29T19:53:53Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"`init_grep_defaults()` fills a `static struct grep_opt grep_defaults`.\nThis struct is then used by `grep_init()` as a blueprint for other such\nstructs. Notably, `grep_init()` takes a `struct repo *` and assigns it\ninto the target struct.\n\nAs a result, it is unnecessary for us to take a `struct repo *` in\n`init_grep_defaults()` as well. We assign it into the default struct and\nnever look at it again. And in light of how we return early if we have\nalready set up the default struct, it's not just unnecessary, but is\nalso a bit confusing: If we are called twice and with different repos,\nis it a bug or a feature that we ignore the second repo?\n\nDrop the repo parameter for `init_grep_defaults()`.\n\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n Documentation/MyFirstObjectWalk.txt | 2 +-\n grep.h                              | 2 +-\n builtin/grep.c                      | 2 +-\n builtin/log.c                       | 2 +-\n grep.c                              | 3 +--\n revision.c                          | 2 +-\n 6 files changed, 6 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/MyFirstObjectWalk.txt b/Documentation/MyFirstObjectWalk.txt\nindex c3f2d1a831..85434d1938 100644\n--- a/Documentation/MyFirstObjectWalk.txt\n+++ b/Documentation/MyFirstObjectWalk.txt\n@@ -394,7 +394,7 @@ First some setup. Add `init_grep_defaults()` to `init_walken_defaults()` and add\n ----\n static void init_walken_defaults(void)\n {\n-\tinit_grep_defaults(the_repository);\n+\tinit_grep_defaults();\n }\n \n ...\ndiff --git a/grep.h b/grep.h\nindex 9115db8515..1c5478f381 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -170,7 +170,7 @@ struct grep_opt {\n \tvoid *output_priv;\n };\n \n-void init_grep_defaults(struct repository *);\n+void init_grep_defaults(void);\n int grep_config(const char *var, const char *value, void *);\n void grep_init(struct grep_opt *, struct repository *repo, const char *prefix);\n void grep_destroy(void);\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex e58e57504c..2b96efa8c2 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -950,7 +950,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\tOPT_END()\n \t};\n \n-\tinit_grep_defaults(the_repository);\n+\tinit_grep_defaults();\n \tgit_config(grep_cmd_config, NULL);\n \tgrep_init(&opt, the_repository, prefix);\n \ndiff --git a/builtin/log.c b/builtin/log.c\nindex 49eb8f6431..eee4beca4d 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -131,7 +131,7 @@ static int log_line_range_callback(const struct option *option, const char *arg,\n \n static void init_log_defaults(void)\n {\n-\tinit_grep_defaults(the_repository);\n+\tinit_grep_defaults();\n \tinit_diff_ui_defaults();\n \n \tdecoration_style = auto_decoration_style();\ndiff --git a/grep.c b/grep.c\nindex 54af9f813e..b351449f7f 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -57,7 +57,7 @@ static void color_set(char *dst, const char *color_bytes)\n  * We could let the compiler do this, but without C99 initializers\n  * the code gets unwieldy and unreadable, so...\n  */\n-void init_grep_defaults(struct repository *repo)\n+void init_grep_defaults(void)\n {\n \tstruct grep_opt *opt = &grep_defaults;\n \tstatic int run_once;\n@@ -67,7 +67,6 @@ void init_grep_defaults(struct repository *repo)\n \trun_once++;\n \n \tmemset(opt, 0, sizeof(*opt));\n-\topt->repo = repo;\n \topt->relative = 1;\n \topt->pathname = 1;\n \topt->max_depth = -1;\ndiff --git a/revision.c b/revision.c\nindex aa62212040..f35ea1db11 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1834,7 +1834,7 @@ void repo_init_revisions(struct repository *r,\n \trevs->commit_format = CMIT_FMT_DEFAULT;\n \trevs->expand_tabs_in_log_default = 8;\n \n-\tinit_grep_defaults(revs->repo);\n+\tinit_grep_defaults();\n \tgrep_init(&revs->grep_filter, revs->repo, prefix);\n \trevs->grep_filter.status_only = 1;\n \n-- \n2.29.2.454.gaff20da3a2\n\n"},{"id":"410983","messageId":"96313423a75fa8d88b6ecd5a15c21a7fbaf9e9be.1606679312.git.martin.agren@gmail.com","threadId":"54687","inReplyTo":"cover.1606679312.git.martin.agren@gmail.com","subject":"[PATCH v3 2/4] grep: use designated initializers for `grep_defaults`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-11-29T19:52:20Z","receivedAt":"2020-11-29T19:53:53Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"In 15fabd1bbd (\"builtin/grep.c: make configuration callback more\nreusable\", 2012-10-09), we learned to fill a `static struct grep_opt\ngrep_defaults` which we can use as a blueprint for other such structs.\n\nAt the time, we didn't consider designated initializers to be widely\nuseable, but these days, we do. (See, e.g., cbc0f81d96 (\"strbuf: use\ndesignated initializers in STRBUF_INIT\", 2017-07-10).)\n\nUse designated initializers to let the compiler set up the struct and so\nthat we don't need to remember to call `init_grep_defaults()`.\n\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n Documentation/MyFirstObjectWalk.txt | 10 +----\n grep.h                              |  1 -\n builtin/grep.c                      |  1 -\n builtin/log.c                       |  1 -\n grep.c                              | 64 +++++++++++------------------\n revision.c                          |  1 -\n 6 files changed, 26 insertions(+), 52 deletions(-)\n\ndiff --git a/Documentation/MyFirstObjectWalk.txt b/Documentation/MyFirstObjectWalk.txt\nindex 85434d1938..7f4bffc4dd 100644\n--- a/Documentation/MyFirstObjectWalk.txt\n+++ b/Documentation/MyFirstObjectWalk.txt\n@@ -388,17 +388,9 @@ Next, let's try to filter the commits we see based on their author. This is\n equivalent to running `git log --author=<pattern>`. We can add a filter by\n modifying `rev_info.grep_filter`, which is a `struct grep_opt`.\n \n-First some setup. Add `init_grep_defaults()` to `init_walken_defaults()` and add\n-`grep_config()` to `git_walken_config()`:\n+First some setup. Add `grep_config()` to `git_walken_config()`:\n \n ----\n-static void init_walken_defaults(void)\n-{\n-\tinit_grep_defaults();\n-}\n-\n-...\n-\n static int git_walken_config(const char *var, const char *value, void *cb)\n {\n \tgrep_config(var, value, cb);\ndiff --git a/grep.h b/grep.h\nindex 1c5478f381..b5c4e223a8 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -170,7 +170,6 @@ struct grep_opt {\n \tvoid *output_priv;\n };\n \n-void init_grep_defaults(void);\n int grep_config(const char *var, const char *value, void *);\n void grep_init(struct grep_opt *, struct repository *repo, const char *prefix);\n void grep_destroy(void);\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 2b96efa8c2..ca259af441 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -950,7 +950,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\tOPT_END()\n \t};\n \n-\tinit_grep_defaults();\n \tgit_config(grep_cmd_config, NULL);\n \tgrep_init(&opt, the_repository, prefix);\n \ndiff --git a/builtin/log.c b/builtin/log.c\nindex eee4beca4d..cf41714fb0 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -131,7 +131,6 @@ static int log_line_range_callback(const struct option *option, const char *arg,\n \n static void init_log_defaults(void)\n {\n-\tinit_grep_defaults();\n \tinit_diff_ui_defaults();\n \n \tdecoration_style = auto_decoration_style();\ndiff --git a/grep.c b/grep.c\nindex b351449f7f..8f2009ec9f 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -14,7 +14,31 @@ static int grep_source_load(struct grep_source *gs);\n static int grep_source_is_binary(struct grep_source *gs,\n \t\t\t\t struct index_state *istate);\n \n-static struct grep_opt grep_defaults;\n+static void std_output(struct grep_opt *opt, const void *buf, size_t size)\n+{\n+\tfwrite(buf, size, 1, stdout);\n+}\n+\n+static struct grep_opt grep_defaults = {\n+\t.relative = 1,\n+\t.pathname = 1,\n+\t.max_depth = -1,\n+\t.pattern_type_option = GREP_PATTERN_TYPE_UNSPECIFIED,\n+\t.colors = {\n+\t\t[GREP_COLOR_CONTEXT] = \"\",\n+\t\t[GREP_COLOR_FILENAME] = \"\",\n+\t\t[GREP_COLOR_FUNCTION] = \"\",\n+\t\t[GREP_COLOR_LINENO] = \"\",\n+\t\t[GREP_COLOR_COLUMNNO] = \"\",\n+\t\t[GREP_COLOR_MATCH_CONTEXT] = GIT_COLOR_BOLD_RED,\n+\t\t[GREP_COLOR_MATCH_SELECTED] = GIT_COLOR_BOLD_RED,\n+\t\t[GREP_COLOR_SELECTED] = \"\",\n+\t\t[GREP_COLOR_SEP] = GIT_COLOR_CYAN,\n+\t},\n+\t.only_matching = 0,\n+\t.color = -1,\n+\t.output = std_output,\n+};\n \n #ifdef USE_LIBPCRE2\n static pcre2_general_context *pcre2_global_context;\n@@ -42,49 +66,11 @@ static const char *color_grep_slots[] = {\n \t[GREP_COLOR_SEP]\t    = \"separator\",\n };\n \n-static void std_output(struct grep_opt *opt, const void *buf, size_t size)\n-{\n-\tfwrite(buf, size, 1, stdout);\n-}\n-\n static void color_set(char *dst, const char *color_bytes)\n {\n \txsnprintf(dst, COLOR_MAXLEN, \"%s\", color_bytes);\n }\n \n-/*\n- * Initialize the grep_defaults template with hardcoded defaults.\n- * We could let the compiler do this, but without C99 initializers\n- * the code gets unwieldy and unreadable, so...\n- */\n-void init_grep_defaults(void)\n-{\n-\tstruct grep_opt *opt = &grep_defaults;\n-\tstatic int run_once;\n-\n-\tif (run_once)\n-\t\treturn;\n-\trun_once++;\n-\n-\tmemset(opt, 0, sizeof(*opt));\n-\topt->relative = 1;\n-\topt->pathname = 1;\n-\topt->max_depth = -1;\n-\topt->pattern_type_option = GREP_PATTERN_TYPE_UNSPECIFIED;\n-\tcolor_set(opt->colors[GREP_COLOR_CONTEXT], \"\");\n-\tcolor_set(opt->colors[GREP_COLOR_FILENAME], \"\");\n-\tcolor_set(opt->colors[GREP_COLOR_FUNCTION], \"\");\n-\tcolor_set(opt->colors[GREP_COLOR_LINENO], \"\");\n-\tcolor_set(opt->colors[GREP_COLOR_COLUMNNO], \"\");\n-\tcolor_set(opt->colors[GREP_COLOR_MATCH_CONTEXT], GIT_COLOR_BOLD_RED);\n-\tcolor_set(opt->colors[GREP_COLOR_MATCH_SELECTED], GIT_COLOR_BOLD_RED);\n-\tcolor_set(opt->colors[GREP_COLOR_SELECTED], \"\");\n-\tcolor_set(opt->colors[GREP_COLOR_SEP], GIT_COLOR_CYAN);\n-\topt->only_matching = 0;\n-\topt->color = -1;\n-\topt->output = std_output;\n-}\n-\n static int parse_pattern_type_arg(const char *opt, const char *arg)\n {\n \tif (!strcmp(arg, \"default\"))\ndiff --git a/revision.c b/revision.c\nindex f35ea1db11..963868f699 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1834,7 +1834,6 @@ void repo_init_revisions(struct repository *r,\n \trevs->commit_format = CMIT_FMT_DEFAULT;\n \trevs->expand_tabs_in_log_default = 8;\n \n-\tinit_grep_defaults();\n \tgrep_init(&revs->grep_filter, revs->repo, prefix);\n \trevs->grep_filter.status_only = 1;\n \n-- \n2.29.2.454.gaff20da3a2\n\n"},{"id":"410984","messageId":"cover.1606679312.git.martin.agren@gmail.com","threadId":"54687","inReplyTo":"cover.1606251357.git.martin.agren@gmail.com","subject":"[PATCH v3 0/4] grep: simplify \"grep defaults\" handling","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-11-29T19:52:18Z","receivedAt":"2020-11-29T19:53:53Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"This is v3 of my series [1] [2] to simplify the setup and use of the\n`struct grep_opt grep_defaults`. The only difference compared to v2 is\nan added comment in the third patch.\n\n[1] https://lore.kernel.org/git/cover.1605972564.git.martin.agren@gmail.com/\n[2] https://lore.kernel.org/git/cover.1606251357.git.martin.agren@gmail.com/\n\nMartin Ågren (4):\n  grep: don't set up a \"default\" repo for grep\n  grep: use designated initializers for `grep_defaults`\n  grep: copy struct in one fell swoop\n  MyFirstObjectWalk: drop `init_walken_defaults()`\n\n Documentation/MyFirstObjectWalk.txt | 34 +---------\n grep.h                              |  1 -\n builtin/grep.c                      |  1 -\n builtin/log.c                       |  1 -\n grep.c                              | 98 +++++++++++------------------\n revision.c                          |  1 -\n 6 files changed, 36 insertions(+), 100 deletions(-)\n\n-- \n2.29.2.454.gaff20da3a2\n\n"},{"id":"410985","messageId":"77371dee2f13723cf8f766793b07241ca458adbd.1606679312.git.martin.agren@gmail.com","threadId":"54687","inReplyTo":"cover.1606679312.git.martin.agren@gmail.com","subject":"[PATCH v3 3/4] grep: copy struct in one fell swoop","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-11-29T19:52:21Z","receivedAt":"2020-11-29T19:53:53Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"We have a `struct grep_opt` with our defaults which we then copy into\nthe caller's struct. Rather than zeroing the target struct and copying\neach element one by one, just copy everything at once. This leaves the\ncode simpler and more maintainable.\n\nWe don't have any ownership issues with what we're copying now and can\njust greedily copy the whole thing. If and when we do need to handle\nsuch elements (`char *`?), we must and can handle it appropriately. Make\nsure to leave a comment to our future selves.\n\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\n---\n grep.c | 33 ++++++++++-----------------------\n 1 file changed, 10 insertions(+), 23 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 8f2009ec9f..efeb6dc58d 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -66,11 +66,6 @@ static const char *color_grep_slots[] = {\n \t[GREP_COLOR_SEP]\t    = \"separator\",\n };\n \n-static void color_set(char *dst, const char *color_bytes)\n-{\n-\txsnprintf(dst, COLOR_MAXLEN, \"%s\", color_bytes);\n-}\n-\n static int parse_pattern_type_arg(const char *opt, const char *arg)\n {\n \tif (!strcmp(arg, \"default\"))\n@@ -100,6 +95,14 @@ int grep_config(const char *var, const char *value, void *cb)\n \tif (userdiff_config(var, value) < 0)\n \t\treturn -1;\n \n+\t/*\n+\t * The instance of grep_opt that we set up here is copied by\n+\t * grep_init() to be used by each individual invocation.\n+\t * When populating a new field of this structure here, be\n+\t * sure to think about ownership -- e.g., you might need to\n+\t * override the shallow copy in grep_init() with a deep copy.\n+\t */\n+\n \tif (!strcmp(var, \"grep.extendedregexp\")) {\n \t\topt->extended_regexp_option = git_config_bool(var, value);\n \t\treturn 0;\n@@ -157,9 +160,6 @@ int grep_config(const char *var, const char *value, void *cb)\n  */\n void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix)\n {\n-\tstruct grep_opt *def = &grep_defaults;\n-\tint i;\n-\n #if defined(USE_LIBPCRE2)\n \tif (!pcre2_global_context)\n \t\tpcre2_global_context = pcre2_general_context_create(\n@@ -171,26 +171,13 @@ void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix\n \tpcre_free = free;\n #endif\n \n-\tmemset(opt, 0, sizeof(*opt));\n+\t*opt = grep_defaults;\n+\n \topt->repo = repo;\n \topt->prefix = prefix;\n \topt->prefix_length = (prefix && *prefix) ? strlen(prefix) : 0;\n \topt->pattern_tail = &opt->pattern_list;\n \topt->header_tail = &opt->header_list;\n-\n-\topt->only_matching = def->only_matching;\n-\topt->color = def->color;\n-\topt->extended_regexp_option = def->extended_regexp_option;\n-\topt->pattern_type_option = def->pattern_type_option;\n-\topt->linenum = def->linenum;\n-\topt->columnnum = def->columnnum;\n-\topt->max_depth = def->max_depth;\n-\topt->pathname = def->pathname;\n-\topt->relative = def->relative;\n-\topt->output = def->output;\n-\n-\tfor (i = 0; i < NR_GREP_COLORS; i++)\n-\t\tcolor_set(opt->colors[i], def->colors[i]);\n }\n \n void grep_destroy(void)\n-- \n2.29.2.454.gaff20da3a2\n\n"},{"id":"410986","messageId":"977a7b6eca28e50d3a7377164228031b96b19ef0.1606679312.git.martin.agren@gmail.com","threadId":"54687","inReplyTo":"cover.1606679312.git.martin.agren@gmail.com","subject":"[PATCH v3 4/4] MyFirstObjectWalk: drop `init_walken_defaults()`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2020-11-29T19:52:22Z","receivedAt":"2020-11-29T19:53:53Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"In a recent commit, we stopped calling `init_grep_defaults()` from this\nfunction. Thus, by the end of the tutorial, we still haven't added any\ncontents to this function. Let's remove it for simplicity.\n\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n Documentation/MyFirstObjectWalk.txt | 24 ------------------------\n 1 file changed, 24 deletions(-)\n\ndiff --git a/Documentation/MyFirstObjectWalk.txt b/Documentation/MyFirstObjectWalk.txt\nindex 7f4bffc4dd..2d10eea7a9 100644\n--- a/Documentation/MyFirstObjectWalk.txt\n+++ b/Documentation/MyFirstObjectWalk.txt\n@@ -182,30 +182,6 @@ its `init_log_defaults()` sets its own state (`decoration_style`) and asks\n `grep` and `diff` to initialize themselves by calling each of their\n initialization functions.\n \n-For our first example within `git walken`, we don't intend to use any other\n-components within Git, and we don't have any configuration to do.  However, we\n-may want to add some later, so for now, we can add an empty placeholder. Create\n-a new function in `builtin/walken.c`:\n-\n-----\n-static void init_walken_defaults(void)\n-{\n-\t/*\n-\t * We don't actually need the same components `git log` does; leave this\n-\t * empty for now.\n-\t */\n-}\n-----\n-\n-Make sure to add a line invoking it inside of `cmd_walken()`.\n-\n-----\n-int cmd_walken(int argc, const char **argv, const char *prefix)\n-{\n-\tinit_walken_defaults();\n-}\n-----\n-\n ==== Configuring From `.gitconfig`\n \n Next, we should have a look at any relevant configuration settings (i.e.,\n-- \n2.29.2.454.gaff20da3a2\n\n"},{"id":"411035","messageId":"xmqqk0u2rv5a.fsf@gitster.c.googlers.com","threadId":"54687","inReplyTo":"cover.1606679312.git.martin.agren@gmail.com","subject":"Re: [PATCH v3 0/4] grep: simplify \"grep defaults\" handling","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-01T04:46:09Z","receivedAt":"2020-12-01T04:47:03Z","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> This is v3 of my series [1] [2] to simplify the setup and use of the\n> `struct grep_opt grep_defaults`. The only difference compared to v2 is\n> an added comment in the third patch.\n\nThanks, will queue.\n"}]}