[PATCH 0/4] grep: retire `init_grep_defaults()`

[PATCH 0/4] grep: retire `init_grep_defaults()`

[Martin Ågren]

Users of the grep machinery need to call `init_grep_defaults()` to
populate some defaults. There's a comment: "We could let the compiler do
this, but without C99 initializers the code gets unwieldy and 
unreadable, so...".

We have such initializers now, so we can simplify accordingly.

Martin Ågren (4):
  grep: don't set up a "default" repo for grep
  grep: use designated initializers for `grep_defaults`
  grep: simplify color setup
  MyFirstObjectWalk: drop `init_walken_defaults()`

 Documentation/MyFirstObjectWalk.txt | 34 +------------
 grep.h                              |  1 -
 builtin/grep.c                      |  1 -
 builtin/log.c                       |  1 -
 grep.c                              | 74 ++++++++++-------------------
 revision.c                          |  1 -
 6 files changed, 27 insertions(+), 85 deletions(-)

-- 
2.29.2.454.gaff20da3a2

[PATCH 1/4] grep: don't set up a "default" repo for grep

[Martin Ågren]

`init_grep_defaults()` fills a `static struct grep_opt grep_defaults`.
This struct is then used by `grep_init()` as a blueprint for other such
structs. Notably, `grep_init()` takes a `struct repo *` and assigns it
into the target struct.

As a result, it is unnecessary for us to take a `struct repo *` in
`init_grep_defaults()` as well. We assign it into the default struct and
never look at it again. And in light of how we return early if we have
already set up the default struct, it's not just unnecessary, but is
also a bit confusing: If we are called twice and with different repos,
is it a bug or a feature that we ignore the second repo?

Drop the repo parameter for `init_grep_defaults()`.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 Documentation/MyFirstObjectWalk.txt | 2 +-
 grep.h                              | 2 +-
 builtin/grep.c                      | 2 +-
 builtin/log.c                       | 2 +-
 grep.c                              | 3 +--
 revision.c                          | 2 +-
 6 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/Documentation/MyFirstObjectWalk.txt b/Documentation/MyFirstObjectWalk.txt
index c3f2d1a831..85434d1938 100644
--- a/Documentation/MyFirstObjectWalk.txt
+++ b/Documentation/MyFirstObjectWalk.txt
@@ -394,7 +394,7 @@ First some setup. Add `init_grep_defaults()` to `init_walken_defaults()` and add
 ----
 static void init_walken_defaults(void)
 {
-	init_grep_defaults(the_repository);
+	init_grep_defaults();
 }
 
 ...
diff --git a/grep.h b/grep.h
index 9115db8515..1c5478f381 100644
--- a/grep.h
+++ b/grep.h
@@ -170,7 +170,7 @@ struct grep_opt {
 	void *output_priv;
 };
 
-void init_grep_defaults(struct repository *);
+void init_grep_defaults(void);
 int grep_config(const char *var, const char *value, void *);
 void grep_init(struct grep_opt *, struct repository *repo, const char *prefix);
 void grep_destroy(void);
diff --git a/builtin/grep.c b/builtin/grep.c
index e58e57504c..2b96efa8c2 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -950,7 +950,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 
-	init_grep_defaults(the_repository);
+	init_grep_defaults();
 	git_config(grep_cmd_config, NULL);
 	grep_init(&opt, the_repository, prefix);
 
diff --git a/builtin/log.c b/builtin/log.c
index 49eb8f6431..eee4beca4d 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -131,7 +131,7 @@ static int log_line_range_callback(const struct option *option, const char *arg,
 
 static void init_log_defaults(void)
 {
-	init_grep_defaults(the_repository);
+	init_grep_defaults();
 	init_diff_ui_defaults();
 
 	decoration_style = auto_decoration_style();
diff --git a/grep.c b/grep.c
index 54af9f813e..b351449f7f 100644
--- a/grep.c
+++ b/grep.c
@@ -57,7 +57,7 @@ static void color_set(char *dst, const char *color_bytes)
  * We could let the compiler do this, but without C99 initializers
  * the code gets unwieldy and unreadable, so...
  */
-void init_grep_defaults(struct repository *repo)
+void init_grep_defaults(void)
 {
 	struct grep_opt *opt = &grep_defaults;
 	static int run_once;
@@ -67,7 +67,6 @@ void init_grep_defaults(struct repository *repo)
 	run_once++;
 
 	memset(opt, 0, sizeof(*opt));
-	opt->repo = repo;
 	opt->relative = 1;
 	opt->pathname = 1;
 	opt->max_depth = -1;
diff --git a/revision.c b/revision.c
index aa62212040..f35ea1db11 100644
--- a/revision.c
+++ b/revision.c
@@ -1834,7 +1834,7 @@ void repo_init_revisions(struct repository *r,
 	revs->commit_format = CMIT_FMT_DEFAULT;
 	revs->expand_tabs_in_log_default = 8;
 
-	init_grep_defaults(revs->repo);
+	init_grep_defaults();
 	grep_init(&revs->grep_filter, revs->repo, prefix);
 	revs->grep_filter.status_only = 1;
 
-- 
2.29.2.454.gaff20da3a2

[PATCH 2/4] grep: use designated initializers for `grep_defaults`

[Martin Ågren]

In 15fabd1bbd ("builtin/grep.c: make configuration callback more
reusable", 2012-10-09), we learned to fill a `static struct grep_opt
grep_defaults` which we can use as a blueprint for other such structs.

At the time, we didn't consider designated initializers to be widely
useable, but these days, we do. (See, e.g., cbc0f81d96 ("strbuf: use
designated initializers in STRBUF_INIT", 2017-07-10).)

Use designated initializers to let the compiler set up the struct and so
that we don't need to remember to call `init_grep_defaults()`.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 If we've messed up and our color strings are too large for our buffers,
 we will no longer hit a BUG within `color_set()`, but we should be able
 to rely on the compiler detecting the truncation. We'll probably be
 *better* off than before, since the compiler will know exactly how
 large the buffer is.

 Documentation/MyFirstObjectWalk.txt | 10 +----
 grep.h                              |  1 -
 builtin/grep.c                      |  1 -
 builtin/log.c                       |  1 -
 grep.c                              | 64 +++++++++++------------------
 revision.c                          |  1 -
 6 files changed, 26 insertions(+), 52 deletions(-)

diff --git a/Documentation/MyFirstObjectWalk.txt b/Documentation/MyFirstObjectWalk.txt
index 85434d1938..7f4bffc4dd 100644
--- a/Documentation/MyFirstObjectWalk.txt
+++ b/Documentation/MyFirstObjectWalk.txt
@@ -388,17 +388,9 @@ Next, let's try to filter the commits we see based on their author. This is
 equivalent to running `git log --author=<pattern>`. We can add a filter by
 modifying `rev_info.grep_filter`, which is a `struct grep_opt`.
 
-First some setup. Add `init_grep_defaults()` to `init_walken_defaults()` and add
-`grep_config()` to `git_walken_config()`:
+First some setup. Add `grep_config()` to `git_walken_config()`:
 
 ----
-static void init_walken_defaults(void)
-{
-	init_grep_defaults();
-}
-
-...
-
 static int git_walken_config(const char *var, const char *value, void *cb)
 {
 	grep_config(var, value, cb);
diff --git a/grep.h b/grep.h
index 1c5478f381..b5c4e223a8 100644
--- a/grep.h
+++ b/grep.h
@@ -170,7 +170,6 @@ struct grep_opt {
 	void *output_priv;
 };
 
-void init_grep_defaults(void);
 int grep_config(const char *var, const char *value, void *);
 void grep_init(struct grep_opt *, struct repository *repo, const char *prefix);
 void grep_destroy(void);
diff --git a/builtin/grep.c b/builtin/grep.c
index 2b96efa8c2..ca259af441 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -950,7 +950,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 
-	init_grep_defaults();
 	git_config(grep_cmd_config, NULL);
 	grep_init(&opt, the_repository, prefix);
 
diff --git a/builtin/log.c b/builtin/log.c
index eee4beca4d..cf41714fb0 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -131,7 +131,6 @@ static int log_line_range_callback(const struct option *option, const char *arg,
 
 static void init_log_defaults(void)
 {
-	init_grep_defaults();
 	init_diff_ui_defaults();
 
 	decoration_style = auto_decoration_style();
diff --git a/grep.c b/grep.c
index b351449f7f..8f2009ec9f 100644
--- a/grep.c
+++ b/grep.c
@@ -14,7 +14,31 @@ static int grep_source_load(struct grep_source *gs);
 static int grep_source_is_binary(struct grep_source *gs,
 				 struct index_state *istate);
 
-static struct grep_opt grep_defaults;
+static void std_output(struct grep_opt *opt, const void *buf, size_t size)
+{
+	fwrite(buf, size, 1, stdout);
+}
+
+static struct grep_opt grep_defaults = {
+	.relative = 1,
+	.pathname = 1,
+	.max_depth = -1,
+	.pattern_type_option = GREP_PATTERN_TYPE_UNSPECIFIED,
+	.colors = {
+		[GREP_COLOR_CONTEXT] = "",
+		[GREP_COLOR_FILENAME] = "",
+		[GREP_COLOR_FUNCTION] = "",
+		[GREP_COLOR_LINENO] = "",
+		[GREP_COLOR_COLUMNNO] = "",
+		[GREP_COLOR_MATCH_CONTEXT] = GIT_COLOR_BOLD_RED,
+		[GREP_COLOR_MATCH_SELECTED] = GIT_COLOR_BOLD_RED,
+		[GREP_COLOR_SELECTED] = "",
+		[GREP_COLOR_SEP] = GIT_COLOR_CYAN,
+	},
+	.only_matching = 0,
+	.color = -1,
+	.output = std_output,
+};
 
 #ifdef USE_LIBPCRE2
 static pcre2_general_context *pcre2_global_context;
@@ -42,49 +66,11 @@ static const char *color_grep_slots[] = {
 	[GREP_COLOR_SEP]	    = "separator",
 };
 
-static void std_output(struct grep_opt *opt, const void *buf, size_t size)
-{
-	fwrite(buf, size, 1, stdout);
-}
-
 static void color_set(char *dst, const char *color_bytes)
 {
 	xsnprintf(dst, COLOR_MAXLEN, "%s", color_bytes);
 }
 
-/*
- * Initialize the grep_defaults template with hardcoded defaults.
- * We could let the compiler do this, but without C99 initializers
- * the code gets unwieldy and unreadable, so...
- */
-void init_grep_defaults(void)
-{
-	struct grep_opt *opt = &grep_defaults;
-	static int run_once;
-
-	if (run_once)
-		return;
-	run_once++;
-
-	memset(opt, 0, sizeof(*opt));
-	opt->relative = 1;
-	opt->pathname = 1;
-	opt->max_depth = -1;
-	opt->pattern_type_option = GREP_PATTERN_TYPE_UNSPECIFIED;
-	color_set(opt->colors[GREP_COLOR_CONTEXT], "");
-	color_set(opt->colors[GREP_COLOR_FILENAME], "");
-	color_set(opt->colors[GREP_COLOR_FUNCTION], "");
-	color_set(opt->colors[GREP_COLOR_LINENO], "");
-	color_set(opt->colors[GREP_COLOR_COLUMNNO], "");
-	color_set(opt->colors[GREP_COLOR_MATCH_CONTEXT], GIT_COLOR_BOLD_RED);
-	color_set(opt->colors[GREP_COLOR_MATCH_SELECTED], GIT_COLOR_BOLD_RED);
-	color_set(opt->colors[GREP_COLOR_SELECTED], "");
-	color_set(opt->colors[GREP_COLOR_SEP], GIT_COLOR_CYAN);
-	opt->only_matching = 0;
-	opt->color = -1;
-	opt->output = std_output;
-}
-
 static int parse_pattern_type_arg(const char *opt, const char *arg)
 {
 	if (!strcmp(arg, "default"))
diff --git a/revision.c b/revision.c
index f35ea1db11..963868f699 100644
--- a/revision.c
+++ b/revision.c
@@ -1834,7 +1834,6 @@ void repo_init_revisions(struct repository *r,
 	revs->commit_format = CMIT_FMT_DEFAULT;
 	revs->expand_tabs_in_log_default = 8;
 
-	init_grep_defaults();
 	grep_init(&revs->grep_filter, revs->repo, prefix);
 	revs->grep_filter.status_only = 1;
 
-- 
2.29.2.454.gaff20da3a2

[PATCH 3/4] grep: simplify color setup

[Martin Ågren]

The previous commit left us with only one user of the one-line wrapper
`color_set()`. We could inline it, but note how we're `xsnprintf()`-ing
all the entries in one array into another array of the same type. We
might as well just `memcpy()` everything into place.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 Cc-ing Peff, who initially introduced this helper. After having inlined
 the function into the for loop, it seemed better to just copy the whole
 array. Happy to hear arguments against.

 Come to think of it, I suppose we could copy the whole struct and not
 just the color array. Hmmm...

 grep.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/grep.c b/grep.c
index 8f2009ec9f..9597cec67e 100644
--- a/grep.c
+++ b/grep.c
@@ -66,11 +66,6 @@ static const char *color_grep_slots[] = {
 	[GREP_COLOR_SEP]	    = "separator",
 };
 
-static void color_set(char *dst, const char *color_bytes)
-{
-	xsnprintf(dst, COLOR_MAXLEN, "%s", color_bytes);
-}
-
 static int parse_pattern_type_arg(const char *opt, const char *arg)
 {
 	if (!strcmp(arg, "default"))
@@ -158,7 +153,6 @@ int grep_config(const char *var, const char *value, void *cb)
 void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix)
 {
 	struct grep_opt *def = &grep_defaults;
-	int i;
 
 #if defined(USE_LIBPCRE2)
 	if (!pcre2_global_context)
@@ -189,8 +183,7 @@ void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix
 	opt->relative = def->relative;
 	opt->output = def->output;
 
-	for (i = 0; i < NR_GREP_COLORS; i++)
-		color_set(opt->colors[i], def->colors[i]);
+	memcpy(opt->colors, def->colors, sizeof(def->colors));
 }
 
 void grep_destroy(void)
-- 
2.29.2.454.gaff20da3a2

[PATCH 4/4] MyFirstObjectWalk: drop `init_walken_defaults()`

[Martin Ågren]

In a recent commit, we stopped calling `init_grep_defaults()` from this
function. Thus, by the end of the tutorial, we still haven't added any
contents to this function. Let's remove it for simplicity.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 Documentation/MyFirstObjectWalk.txt | 24 ------------------------
 1 file changed, 24 deletions(-)

diff --git a/Documentation/MyFirstObjectWalk.txt b/Documentation/MyFirstObjectWalk.txt
index 7f4bffc4dd..2d10eea7a9 100644
--- a/Documentation/MyFirstObjectWalk.txt
+++ b/Documentation/MyFirstObjectWalk.txt
@@ -182,30 +182,6 @@ its `init_log_defaults()` sets its own state (`decoration_style`) and asks
 `grep` and `diff` to initialize themselves by calling each of their
 initialization functions.
 
-For our first example within `git walken`, we don't intend to use any other
-components within Git, and we don't have any configuration to do.  However, we
-may want to add some later, so for now, we can add an empty placeholder. Create
-a new function in `builtin/walken.c`:
-
-----
-static void init_walken_defaults(void)
-{
-	/*
-	 * We don't actually need the same components `git log` does; leave this
-	 * empty for now.
-	 */
-}
-----
-
-Make sure to add a line invoking it inside of `cmd_walken()`.
-
-----
-int cmd_walken(int argc, const char **argv, const char *prefix)
-{
-	init_walken_defaults();
-}
-----
-
 ==== Configuring From `.gitconfig`
 
 Next, we should have a look at any relevant configuration settings (i.e.,
-- 
2.29.2.454.gaff20da3a2

Re: [PATCH 3/4] grep: simplify color setup

[Jeff King]

On Sat, Nov 21, 2020 at 07:31:09PM +0100, Martin Ågren wrote:

> The previous commit left us with only one user of the one-line wrapper
> `color_set()`. We could inline it, but note how we're `xsnprintf()`-ing
> all the entries in one array into another array of the same type. We
> might as well just `memcpy()` everything into place.
> 
> Signed-off-by: Martin Ågren <martin.agren@gmail.com>
> ---
>  Cc-ing Peff, who initially introduced this helper. After having inlined
>  the function into the for loop, it seemed better to just copy the whole
>  array. Happy to hear arguments against.

No, this is way better than the existing code. I introduced it to get
away from strcpy(), but this is better still. But...

>  Come to think of it, I suppose we could copy the whole struct and not
>  just the color array. Hmmm...

Yes, this seems even better. If our goal is just to start our new
grep_opt the same as grep_defaults, then a single-line struct copy
(whether through assignment or memcpy) is even clearer and more
maintainable.

-Peff

Re: [PATCH 3/4] grep: simplify color setup

[Martin Ågren]

On Sat, 21 Nov 2020 at 21:23, Jeff King <peff@peff.net> wrote:
>
> On Sat, Nov 21, 2020 at 07:31:09PM +0100, Martin Ågren wrote:
>> >  Cc-ing Peff, who initially introduced this helper. After having inlined
> >  the function into the for loop, it seemed better to just copy the whole
> >  array. Happy to hear arguments against.
>
> No, this is way better than the existing code. I introduced it to get
> away from strcpy(), but this is better still. But...
>
> >  Come to think of it, I suppose we could copy the whole struct and not
> >  just the color array. Hmmm...
>
> Yes, this seems even better. If our goal is just to start our new
> grep_opt the same as grep_defaults, then a single-line struct copy
> (whether through assignment or memcpy) is even clearer and more
> maintainable.

Ok, thanks for the encouraging words. I couldn't keep myself from
thinking that we're doing this for some weird ... performance reason?
Thanks for taking me out of that thought.

I'll hold off for a while in case there's more feedback, then look into
replacing this patch with a more aggressive copy of the whole struct.

Martin

Re: [PATCH 3/4] grep: simplify color setup

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Sat, Nov 21, 2020 at 07:31:09PM +0100, Martin Ågren wrote:
>
>> The previous commit left us with only one user of the one-line wrapper
>> `color_set()`. We could inline it, but note how we're `xsnprintf()`-ing
>> all the entries in one array into another array of the same type. We
>> might as well just `memcpy()` everything into place.
>> 
>> Signed-off-by: Martin Ågren <martin.agren@gmail.com>
>> ---
>>  Cc-ing Peff, who initially introduced this helper. After having inlined
>>  the function into the for loop, it seemed better to just copy the whole
>>  array. Happy to hear arguments against.
>
> No, this is way better than the existing code. I introduced it to get
> away from strcpy(), but this is better still. But...

Yes, the copy in this patch looks eminently sensible.

>>  Come to think of it, I suppose we could copy the whole struct and not
>>  just the color array. Hmmm...
>
> Yes, this seems even better. If our goal is just to start our new
> grep_opt the same as grep_defaults, then a single-line struct copy
> (whether through assignment or memcpy) is even clearer and more
> maintainable.

... until such a time when typeof(grep_defaults) gains a field with
a non-const pointer value that we'd rather not to share amongst
instances of the type, at which point it no longer is clear win from
maintainability's point of view.

Re: [PATCH 0/4] grep: retire `init_grep_defaults()`

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 974 bytes --]

Hi Martin,

On Sat, 21 Nov 2020, Martin Ågren wrote:

> Users of the grep machinery need to call `init_grep_defaults()` to
> populate some defaults. There's a comment: "We could let the compiler do
> this, but without C99 initializers the code gets unwieldy and
> unreadable, so...".
>
> We have such initializers now, so we can simplify accordingly.

When I read this at first, I feared that you would change a
`pthread_init_mutex()` to using `PTHREAD_MUTEX_INITIALIZER` instead. On
the face of it, that would look good on paper, but it unfortunately falls
flat for us because there is no Win32 equivalent for the static
initializer: we emulate mutexes via `CRITICAL_SECTION`s, and those need to
be initialized by running a function.

To my surprise and relief, your patches do not do that; The only mutex in
`grep.c` (`grep_attr_mutex`) is not touched.

I just wanted to mention this here, in case anybody else had the same
idea.

Thanks,
Dscho

Re: [PATCH 3/4] grep: simplify color setup

[Jeff King]

On Sat, Nov 21, 2020 at 02:46:14PM -0800, Junio C Hamano wrote:

> >>  Come to think of it, I suppose we could copy the whole struct and not
> >>  just the color array. Hmmm...
> >
> > Yes, this seems even better. If our goal is just to start our new
> > grep_opt the same as grep_defaults, then a single-line struct copy
> > (whether through assignment or memcpy) is even clearer and more
> > maintainable.
> 
> ... until such a time when typeof(grep_defaults) gains a field with
> a non-const pointer value that we'd rather not to share amongst
> instances of the type, at which point it no longer is clear win from
> maintainability's point of view.

I don't think we are any worse off. Either way we need to add a
special-case deep copy to the function (which should definitely happen
in only one place; we do not want bare struct copies sprinkled around
the code).

-Peff

[PATCH v2 0/4] grep: simplify "grep defaults" handling

[Martin Ågren]

This is v2 of my series [1] to simplify the setup and use of the `struct
grep_opt grep_defaults`. The only difference compared to v1 is in the
third patch which now drops more code in favor of copying the whole
struct in one go.

[1] https://lore.kernel.org/git/cover.1605972564.git.martin.agren@gmail.com/

Martin Ågren (4):
  grep: don't set up a "default" repo for grep
  grep: use designated initializers for `grep_defaults`
  grep: copy struct in one fell swoop
  MyFirstObjectWalk: drop `init_walken_defaults()`

 Documentation/MyFirstObjectWalk.txt | 34 +----------
 grep.h                              |  1 -
 builtin/grep.c                      |  1 -
 builtin/log.c                       |  1 -
 grep.c                              | 90 +++++++++--------------------
 revision.c                          |  1 -
 6 files changed, 28 insertions(+), 100 deletions(-)

-- 
2.29.2.454.gaff20da3a2

[PATCH v2 1/4] grep: don't set up a "default" repo for grep

[Martin Ågren]

`init_grep_defaults()` fills a `static struct grep_opt grep_defaults`.
This struct is then used by `grep_init()` as a blueprint for other such
structs. Notably, `grep_init()` takes a `struct repo *` and assigns it
into the target struct.

As a result, it is unnecessary for us to take a `struct repo *` in
`init_grep_defaults()` as well. We assign it into the default struct and
never look at it again. And in light of how we return early if we have
already set up the default struct, it's not just unnecessary, but is
also a bit confusing: If we are called twice and with different repos,
is it a bug or a feature that we ignore the second repo?

Drop the repo parameter for `init_grep_defaults()`.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 Documentation/MyFirstObjectWalk.txt | 2 +-
 grep.h                              | 2 +-
 builtin/grep.c                      | 2 +-
 builtin/log.c                       | 2 +-
 grep.c                              | 3 +--
 revision.c                          | 2 +-
 6 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/Documentation/MyFirstObjectWalk.txt b/Documentation/MyFirstObjectWalk.txt
index c3f2d1a831..85434d1938 100644
--- a/Documentation/MyFirstObjectWalk.txt
+++ b/Documentation/MyFirstObjectWalk.txt
@@ -394,7 +394,7 @@ First some setup. Add `init_grep_defaults()` to `init_walken_defaults()` and add
 ----
 static void init_walken_defaults(void)
 {
-	init_grep_defaults(the_repository);
+	init_grep_defaults();
 }
 
 ...
diff --git a/grep.h b/grep.h
index 9115db8515..1c5478f381 100644
--- a/grep.h
+++ b/grep.h
@@ -170,7 +170,7 @@ struct grep_opt {
 	void *output_priv;
 };
 
-void init_grep_defaults(struct repository *);
+void init_grep_defaults(void);
 int grep_config(const char *var, const char *value, void *);
 void grep_init(struct grep_opt *, struct repository *repo, const char *prefix);
 void grep_destroy(void);
diff --git a/builtin/grep.c b/builtin/grep.c
index e58e57504c..2b96efa8c2 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -950,7 +950,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 
-	init_grep_defaults(the_repository);
+	init_grep_defaults();
 	git_config(grep_cmd_config, NULL);
 	grep_init(&opt, the_repository, prefix);
 
diff --git a/builtin/log.c b/builtin/log.c
index 49eb8f6431..eee4beca4d 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -131,7 +131,7 @@ static int log_line_range_callback(const struct option *option, const char *arg,
 
 static void init_log_defaults(void)
 {
-	init_grep_defaults(the_repository);
+	init_grep_defaults();
 	init_diff_ui_defaults();
 
 	decoration_style = auto_decoration_style();
diff --git a/grep.c b/grep.c
index 54af9f813e..b351449f7f 100644
--- a/grep.c
+++ b/grep.c
@@ -57,7 +57,7 @@ static void color_set(char *dst, const char *color_bytes)
  * We could let the compiler do this, but without C99 initializers
  * the code gets unwieldy and unreadable, so...
  */
-void init_grep_defaults(struct repository *repo)
+void init_grep_defaults(void)
 {
 	struct grep_opt *opt = &grep_defaults;
 	static int run_once;
@@ -67,7 +67,6 @@ void init_grep_defaults(struct repository *repo)
 	run_once++;
 
 	memset(opt, 0, sizeof(*opt));
-	opt->repo = repo;
 	opt->relative = 1;
 	opt->pathname = 1;
 	opt->max_depth = -1;
diff --git a/revision.c b/revision.c
index aa62212040..f35ea1db11 100644
--- a/revision.c
+++ b/revision.c
@@ -1834,7 +1834,7 @@ void repo_init_revisions(struct repository *r,
 	revs->commit_format = CMIT_FMT_DEFAULT;
 	revs->expand_tabs_in_log_default = 8;
 
-	init_grep_defaults(revs->repo);
+	init_grep_defaults();
 	grep_init(&revs->grep_filter, revs->repo, prefix);
 	revs->grep_filter.status_only = 1;
 
-- 
2.29.2.454.gaff20da3a2

[PATCH v2 2/4] grep: use designated initializers for `grep_defaults`

[Martin Ågren]

In 15fabd1bbd ("builtin/grep.c: make configuration callback more
reusable", 2012-10-09), we learned to fill a `static struct grep_opt
grep_defaults` which we can use as a blueprint for other such structs.

At the time, we didn't consider designated initializers to be widely
useable, but these days, we do. (See, e.g., cbc0f81d96 ("strbuf: use
designated initializers in STRBUF_INIT", 2017-07-10).)

Use designated initializers to let the compiler set up the struct and so
that we don't need to remember to call `init_grep_defaults()`.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 Documentation/MyFirstObjectWalk.txt | 10 +----
 grep.h                              |  1 -
 builtin/grep.c                      |  1 -
 builtin/log.c                       |  1 -
 grep.c                              | 64 +++++++++++------------------
 revision.c                          |  1 -
 6 files changed, 26 insertions(+), 52 deletions(-)

diff --git a/Documentation/MyFirstObjectWalk.txt b/Documentation/MyFirstObjectWalk.txt
index 85434d1938..7f4bffc4dd 100644
--- a/Documentation/MyFirstObjectWalk.txt
+++ b/Documentation/MyFirstObjectWalk.txt
@@ -388,17 +388,9 @@ Next, let's try to filter the commits we see based on their author. This is
 equivalent to running `git log --author=<pattern>`. We can add a filter by
 modifying `rev_info.grep_filter`, which is a `struct grep_opt`.
 
-First some setup. Add `init_grep_defaults()` to `init_walken_defaults()` and add
-`grep_config()` to `git_walken_config()`:
+First some setup. Add `grep_config()` to `git_walken_config()`:
 
 ----
-static void init_walken_defaults(void)
-{
-	init_grep_defaults();
-}
-
-...
-
 static int git_walken_config(const char *var, const char *value, void *cb)
 {
 	grep_config(var, value, cb);
diff --git a/grep.h b/grep.h
index 1c5478f381..b5c4e223a8 100644
--- a/grep.h
+++ b/grep.h
@@ -170,7 +170,6 @@ struct grep_opt {
 	void *output_priv;
 };
 
-void init_grep_defaults(void);
 int grep_config(const char *var, const char *value, void *);
 void grep_init(struct grep_opt *, struct repository *repo, const char *prefix);
 void grep_destroy(void);
diff --git a/builtin/grep.c b/builtin/grep.c
index 2b96efa8c2..ca259af441 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -950,7 +950,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 
-	init_grep_defaults();
 	git_config(grep_cmd_config, NULL);
 	grep_init(&opt, the_repository, prefix);
 
diff --git a/builtin/log.c b/builtin/log.c
index eee4beca4d..cf41714fb0 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -131,7 +131,6 @@ static int log_line_range_callback(const struct option *option, const char *arg,
 
 static void init_log_defaults(void)
 {
-	init_grep_defaults();
 	init_diff_ui_defaults();
 
 	decoration_style = auto_decoration_style();
diff --git a/grep.c b/grep.c
index b351449f7f..8f2009ec9f 100644
--- a/grep.c
+++ b/grep.c
@@ -14,7 +14,31 @@ static int grep_source_load(struct grep_source *gs);
 static int grep_source_is_binary(struct grep_source *gs,
 				 struct index_state *istate);
 
-static struct grep_opt grep_defaults;
+static void std_output(struct grep_opt *opt, const void *buf, size_t size)
+{
+	fwrite(buf, size, 1, stdout);
+}
+
+static struct grep_opt grep_defaults = {
+	.relative = 1,
+	.pathname = 1,
+	.max_depth = -1,
+	.pattern_type_option = GREP_PATTERN_TYPE_UNSPECIFIED,
+	.colors = {
+		[GREP_COLOR_CONTEXT] = "",
+		[GREP_COLOR_FILENAME] = "",
+		[GREP_COLOR_FUNCTION] = "",
+		[GREP_COLOR_LINENO] = "",
+		[GREP_COLOR_COLUMNNO] = "",
+		[GREP_COLOR_MATCH_CONTEXT] = GIT_COLOR_BOLD_RED,
+		[GREP_COLOR_MATCH_SELECTED] = GIT_COLOR_BOLD_RED,
+		[GREP_COLOR_SELECTED] = "",
+		[GREP_COLOR_SEP] = GIT_COLOR_CYAN,
+	},
+	.only_matching = 0,
+	.color = -1,
+	.output = std_output,
+};
 
 #ifdef USE_LIBPCRE2
 static pcre2_general_context *pcre2_global_context;
@@ -42,49 +66,11 @@ static const char *color_grep_slots[] = {
 	[GREP_COLOR_SEP]	    = "separator",
 };
 
-static void std_output(struct grep_opt *opt, const void *buf, size_t size)
-{
-	fwrite(buf, size, 1, stdout);
-}
-
 static void color_set(char *dst, const char *color_bytes)
 {
 	xsnprintf(dst, COLOR_MAXLEN, "%s", color_bytes);
 }
 
-/*
- * Initialize the grep_defaults template with hardcoded defaults.
- * We could let the compiler do this, but without C99 initializers
- * the code gets unwieldy and unreadable, so...
- */
-void init_grep_defaults(void)
-{
-	struct grep_opt *opt = &grep_defaults;
-	static int run_once;
-
-	if (run_once)
-		return;
-	run_once++;
-
-	memset(opt, 0, sizeof(*opt));
-	opt->relative = 1;
-	opt->pathname = 1;
-	opt->max_depth = -1;
-	opt->pattern_type_option = GREP_PATTERN_TYPE_UNSPECIFIED;
-	color_set(opt->colors[GREP_COLOR_CONTEXT], "");
-	color_set(opt->colors[GREP_COLOR_FILENAME], "");
-	color_set(opt->colors[GREP_COLOR_FUNCTION], "");
-	color_set(opt->colors[GREP_COLOR_LINENO], "");
-	color_set(opt->colors[GREP_COLOR_COLUMNNO], "");
-	color_set(opt->colors[GREP_COLOR_MATCH_CONTEXT], GIT_COLOR_BOLD_RED);
-	color_set(opt->colors[GREP_COLOR_MATCH_SELECTED], GIT_COLOR_BOLD_RED);
-	color_set(opt->colors[GREP_COLOR_SELECTED], "");
-	color_set(opt->colors[GREP_COLOR_SEP], GIT_COLOR_CYAN);
-	opt->only_matching = 0;
-	opt->color = -1;
-	opt->output = std_output;
-}
-
 static int parse_pattern_type_arg(const char *opt, const char *arg)
 {
 	if (!strcmp(arg, "default"))
diff --git a/revision.c b/revision.c
index f35ea1db11..963868f699 100644
--- a/revision.c
+++ b/revision.c
@@ -1834,7 +1834,6 @@ void repo_init_revisions(struct repository *r,
 	revs->commit_format = CMIT_FMT_DEFAULT;
 	revs->expand_tabs_in_log_default = 8;
 
-	init_grep_defaults();
 	grep_init(&revs->grep_filter, revs->repo, prefix);
 	revs->grep_filter.status_only = 1;
 
-- 
2.29.2.454.gaff20da3a2

[PATCH v2 3/4] grep: copy struct in one fell swoop

[Martin Ågren]

We have a `struct grep_opt` with our defaults which we then copy into
the caller's struct. Rather than zeroing the target struct and copying
each element one by one, just copy everything at once. This leaves the
code simpler and more maintainable.

We don't have any ownership issues with what we're copying now and can
just greedily copy the whole thing. If and when we do need to handle
such elements (`char *`?), we must and can handle it appropriately. This
commit doesn't really change that.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 grep.c | 25 ++-----------------------
 1 file changed, 2 insertions(+), 23 deletions(-)

diff --git a/grep.c b/grep.c
index 8f2009ec9f..7d740452cd 100644
--- a/grep.c
+++ b/grep.c
@@ -66,11 +66,6 @@ static const char *color_grep_slots[] = {
 	[GREP_COLOR_SEP]	    = "separator",
 };
 
-static void color_set(char *dst, const char *color_bytes)
-{
-	xsnprintf(dst, COLOR_MAXLEN, "%s", color_bytes);
-}
-
 static int parse_pattern_type_arg(const char *opt, const char *arg)
 {
 	if (!strcmp(arg, "default"))
@@ -157,9 +152,6 @@ int grep_config(const char *var, const char *value, void *cb)
  */
 void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix)
 {
-	struct grep_opt *def = &grep_defaults;
-	int i;
-
 #if defined(USE_LIBPCRE2)
 	if (!pcre2_global_context)
 		pcre2_global_context = pcre2_general_context_create(
@@ -171,26 +163,13 @@ void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix
 	pcre_free = free;
 #endif
 
-	memset(opt, 0, sizeof(*opt));
+	*opt = grep_defaults;
+
 	opt->repo = repo;
 	opt->prefix = prefix;
 	opt->prefix_length = (prefix && *prefix) ? strlen(prefix) : 0;
 	opt->pattern_tail = &opt->pattern_list;
 	opt->header_tail = &opt->header_list;
-
-	opt->only_matching = def->only_matching;
-	opt->color = def->color;
-	opt->extended_regexp_option = def->extended_regexp_option;
-	opt->pattern_type_option = def->pattern_type_option;
-	opt->linenum = def->linenum;
-	opt->columnnum = def->columnnum;
-	opt->max_depth = def->max_depth;
-	opt->pathname = def->pathname;
-	opt->relative = def->relative;
-	opt->output = def->output;
-
-	for (i = 0; i < NR_GREP_COLORS; i++)
-		color_set(opt->colors[i], def->colors[i]);
 }
 
 void grep_destroy(void)
-- 
2.29.2.454.gaff20da3a2

[PATCH v2 4/4] MyFirstObjectWalk: drop `init_walken_defaults()`

[Martin Ågren]

In a recent commit, we stopped calling `init_grep_defaults()` from this
function. Thus, by the end of the tutorial, we still haven't added any
contents to this function. Let's remove it for simplicity.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 Documentation/MyFirstObjectWalk.txt | 24 ------------------------
 1 file changed, 24 deletions(-)

diff --git a/Documentation/MyFirstObjectWalk.txt b/Documentation/MyFirstObjectWalk.txt
index 7f4bffc4dd..2d10eea7a9 100644
--- a/Documentation/MyFirstObjectWalk.txt
+++ b/Documentation/MyFirstObjectWalk.txt
@@ -182,30 +182,6 @@ its `init_log_defaults()` sets its own state (`decoration_style`) and asks
 `grep` and `diff` to initialize themselves by calling each of their
 initialization functions.
 
-For our first example within `git walken`, we don't intend to use any other
-components within Git, and we don't have any configuration to do.  However, we
-may want to add some later, so for now, we can add an empty placeholder. Create
-a new function in `builtin/walken.c`:
-
-----
-static void init_walken_defaults(void)
-{
-	/*
-	 * We don't actually need the same components `git log` does; leave this
-	 * empty for now.
-	 */
-}
-----
-
-Make sure to add a line invoking it inside of `cmd_walken()`.
-
-----
-int cmd_walken(int argc, const char **argv, const char *prefix)
-{
-	init_walken_defaults();
-}
-----
-
 ==== Configuring From `.gitconfig`
 
 Next, we should have a look at any relevant configuration settings (i.e.,
-- 
2.29.2.454.gaff20da3a2

Re: [PATCH v2 3/4] grep: copy struct in one fell swoop

[Junio C Hamano]

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

> We have a `struct grep_opt` with our defaults which we then copy into
> the caller's struct. Rather than zeroing the target struct and copying
> each element one by one, just copy everything at once. This leaves the
> code simpler and more maintainable.
>
> We don't have any ownership issues with what we're copying now and can
> just greedily copy the whole thing. If and when we do need to handle
> such elements (`char *`?), we must and can handle it appropriately.

That is correct, but ...

> This
> commit doesn't really change that.

... I suspect this is not.

In the original code, those who are adding a new field would notice
that it is not copied over to the new instance (because they didn't
add anything to grep_init() to copy the field) and at that point
they must stop and think how the new field need to be copied.

The structure assignment of the outer shell done in this patch means
they are robbed of the opportunity to stop and think, because most
of the time it "works" out of the box.  I'd feel safer if we left a
clue to future developers if we were to do your clean-up, perhaps
like:

diff --git c/grep.h w/grep.h
index b5c4e223a8..388d226da3 100644
--- c/grep.h
+++ w/grep.h
@@ -115,6 +115,14 @@ struct grep_expr {
 	} u;
 };
 
+/*
+ * grep_config() initializes one "default" instance of this type, and
+ * it is copied by grep_init() to be used by each individual
+ * invocation.  When adding a new field to this structure that is
+ * populated from the configuration, be sure to think about ownership
+ * (i.e. a shallow copy may not be what you want for the type of your
+ * newly added field).
+ */
 struct grep_opt {
 	struct grep_pat *pattern_list;
 	struct grep_pat **pattern_tail;

Re: [PATCH v2 3/4] grep: copy struct in one fell swoop

[Martin Ågren]

On Tue, 24 Nov 2020 at 23:34, Junio C Hamano <gitster@pobox.com> wrote:
>
> Martin Ågren <martin.agren@gmail.com> writes:
>
> > We don't have any ownership issues with what we're copying now and can
> > just greedily copy the whole thing. If and when we do need to handle
> > such elements (`char *`?), we must and can handle it appropriately.
>
> That is correct, but ...
>
> > This
> > commit doesn't really change that.
>
> ... I suspect this is not.
>
> In the original code, those who are adding a new field would notice
> that it is not copied over to the new instance (because they didn't
> add anything to grep_init() to copy the field) and at that point
> they must stop and think how the new field need to be copied.
>
> The structure assignment of the outer shell done in this patch means
> they are robbed of the opportunity to stop and think, because most
> of the time it "works" out of the box.  I'd feel safer if we left a
> clue to future developers if we were to do your clean-up, perhaps
> like:
>
> diff --git c/grep.h w/grep.h
> index b5c4e223a8..388d226da3 100644
> --- c/grep.h
> +++ w/grep.h
> @@ -115,6 +115,14 @@ struct grep_expr {
>         } u;
>  };
>
> +/*
> + * grep_config() initializes one "default" instance of this type, and
> + * it is copied by grep_init() to be used by each individual
> + * invocation.  When adding a new field to this structure that is
> + * populated from the configuration, be sure to think about ownership
> + * (i.e. a shallow copy may not be what you want for the type of your
> + * newly added field).
> + */
>  struct grep_opt {
>         struct grep_pat *pattern_list;
>         struct grep_pat **pattern_tail;

Ok, that makes sense. Maybe put it in `grep_config()` though? We can add
anything we want to to this struct and initialize it from the command
line. It's when we start pre-filling it in `grep_config()` that we need
to think about this. What do you think? We could also do both of
course to really hedge our bets...

  /*
   * The instance of grep_opt that we set up here is copied by
   * grep_init() to be used by each individual invocation.
   * When populating a new field of this structure here,
   * be sure to think about ownership (i.e. a shallow copy in
   * grep_init() may not be what you want).
   */

Thanks
Martin

Re: [PATCH v2 3/4] grep: copy struct in one fell swoop

[Junio C Hamano]

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

> On Tue, 24 Nov 2020 at 23:34, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> +/*
>> + * grep_config() initializes one "default" instance of this type, and
>> + * it is copied by grep_init() to be used by each individual
>> + * invocation.  When adding a new field to this structure that is
>> + * populated from the configuration, be sure to think about ownership
>> + * (i.e. a shallow copy may not be what you want for the type of your
>> + * newly added field).
>> + */
>>  struct grep_opt {
>>         struct grep_pat *pattern_list;
>>         struct grep_pat **pattern_tail;
>
> Ok, that makes sense. Maybe put it in `grep_config()` though? We can add
> anything we want to to this struct and initialize it from the command
> line. It's when we start pre-filling it in `grep_config()` that we need
> to think about this. What do you think? We could also do both of
> course to really hedge our bets...

I agree with you that it would be the most helpful to have the
comment near grep_config(), as that function is what defines the
design of populating the singleton to be copied by the instance
used by individual invocation.

>   /*
>    * The instance of grep_opt that we set up here is copied by
>    * grep_init() to be used by each individual invocation.
>    * When populating a new field of this structure here,
>    * be sure to think about ownership (i.e. a shallow copy in
>    * grep_init() may not be what you want).
>    */

I find the text near the end of both my version and yours a bit
unsatisfying.  One thing I care about is not to mislead readers to
think that the way grep_init() copies the singleton template is
correct and sacred and they need to design their data structure to
be compatible with the shallow copying.  We'd want it to be clear
that it is expected that they will deep copy the field, and release
it once individual invocation is done, when they need a new field
that won't work well with shallow copying.  Perhaps "may not be wnat
you want" is explicit enough, but I dunno.

Thanks.

Re: [PATCH v2 0/4] grep: simplify "grep defaults" handling

[Ævar Arnfjörð Bjarmason]

On Tue, Nov 24 2020, Martin Ågren wrote:

> This is v2 of my series [1] to simplify the setup and use of the `struct
> grep_opt grep_defaults`. The only difference compared to v1 is in the
> third patch which now drops more code in favor of copying the whole
> struct in one go.

It's good to see this change, I remembered having to juggle these values
a while back in:

    git log -p --author=Ævar --grep=memset -- grep.c

Thanks.

Re: [PATCH v2 3/4] grep: copy struct in one fell swoop

[Martin Ågren]

On Wed, 25 Nov 2020 at 08:53, Junio C Hamano <gitster@pobox.com> wrote:
>
> Martin Ågren <martin.agren@gmail.com> writes:
>
> >   /*
> >    * The instance of grep_opt that we set up here is copied by
> >    * grep_init() to be used by each individual invocation.
> >    * When populating a new field of this structure here,
> >    * be sure to think about ownership (i.e. a shallow copy in
> >    * grep_init() may not be what you want).
> >    */
>
> I find the text near the end of both my version and yours a bit
> unsatisfying.  One thing I care about is not to mislead readers to
> think that the way grep_init() copies the singleton template is
> correct and sacred and they need to design their data structure to
> be compatible with the shallow copying.  We'd want it to be clear
> that it is expected that they will deep copy the field, and release
> it once individual invocation is done, when they need a new field
> that won't work well with shallow copying.  Perhaps "may not be wnat
> you want" is explicit enough, but I dunno.

I understand your concern. Here's what I'm considering using:

  /*
   * The instance of grep_opt that we set up here is copied by
   * grep_init() to be used by each individual invocation.
   * When populating a new field of this structure here, be
   * sure to think about ownership -- e.g., you might need to
   * override the shallow copy in grep_init() with a deep copy.
   */

I'm not going into the releasing of those resources, but hopefully
that part can be left to the reader.

Other suggestions welcome, of course. I hope to get around to rerolling
this on the weekend.

Martin

[PATCH v3 0/4] grep: simplify "grep defaults" handling

[Martin Ågren]

This is v3 of my series [1] [2] to simplify the setup and use of the
`struct grep_opt grep_defaults`. The only difference compared to v2 is
an added comment in the third patch.

[1] https://lore.kernel.org/git/cover.1605972564.git.martin.agren@gmail.com/
[2] https://lore.kernel.org/git/cover.1606251357.git.martin.agren@gmail.com/

Martin Ågren (4):
  grep: don't set up a "default" repo for grep
  grep: use designated initializers for `grep_defaults`
  grep: copy struct in one fell swoop
  MyFirstObjectWalk: drop `init_walken_defaults()`

 Documentation/MyFirstObjectWalk.txt | 34 +---------
 grep.h                              |  1 -
 builtin/grep.c                      |  1 -
 builtin/log.c                       |  1 -
 grep.c                              | 98 +++++++++++------------------
 revision.c                          |  1 -
 6 files changed, 36 insertions(+), 100 deletions(-)

-- 
2.29.2.454.gaff20da3a2

[PATCH v3 1/4] grep: don't set up a "default" repo for grep

[Martin Ågren]

`init_grep_defaults()` fills a `static struct grep_opt grep_defaults`.
This struct is then used by `grep_init()` as a blueprint for other such
structs. Notably, `grep_init()` takes a `struct repo *` and assigns it
into the target struct.

As a result, it is unnecessary for us to take a `struct repo *` in
`init_grep_defaults()` as well. We assign it into the default struct and
never look at it again. And in light of how we return early if we have
already set up the default struct, it's not just unnecessary, but is
also a bit confusing: If we are called twice and with different repos,
is it a bug or a feature that we ignore the second repo?

Drop the repo parameter for `init_grep_defaults()`.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/MyFirstObjectWalk.txt | 2 +-
 grep.h                              | 2 +-
 builtin/grep.c                      | 2 +-
 builtin/log.c                       | 2 +-
 grep.c                              | 3 +--
 revision.c                          | 2 +-
 6 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/Documentation/MyFirstObjectWalk.txt b/Documentation/MyFirstObjectWalk.txt
index c3f2d1a831..85434d1938 100644
--- a/Documentation/MyFirstObjectWalk.txt
+++ b/Documentation/MyFirstObjectWalk.txt
@@ -394,7 +394,7 @@ First some setup. Add `init_grep_defaults()` to `init_walken_defaults()` and add
 ----
 static void init_walken_defaults(void)
 {
-	init_grep_defaults(the_repository);
+	init_grep_defaults();
 }
 
 ...
diff --git a/grep.h b/grep.h
index 9115db8515..1c5478f381 100644
--- a/grep.h
+++ b/grep.h
@@ -170,7 +170,7 @@ struct grep_opt {
 	void *output_priv;
 };
 
-void init_grep_defaults(struct repository *);
+void init_grep_defaults(void);
 int grep_config(const char *var, const char *value, void *);
 void grep_init(struct grep_opt *, struct repository *repo, const char *prefix);
 void grep_destroy(void);
diff --git a/builtin/grep.c b/builtin/grep.c
index e58e57504c..2b96efa8c2 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -950,7 +950,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 
-	init_grep_defaults(the_repository);
+	init_grep_defaults();
 	git_config(grep_cmd_config, NULL);
 	grep_init(&opt, the_repository, prefix);
 
diff --git a/builtin/log.c b/builtin/log.c
index 49eb8f6431..eee4beca4d 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -131,7 +131,7 @@ static int log_line_range_callback(const struct option *option, const char *arg,
 
 static void init_log_defaults(void)
 {
-	init_grep_defaults(the_repository);
+	init_grep_defaults();
 	init_diff_ui_defaults();
 
 	decoration_style = auto_decoration_style();
diff --git a/grep.c b/grep.c
index 54af9f813e..b351449f7f 100644
--- a/grep.c
+++ b/grep.c
@@ -57,7 +57,7 @@ static void color_set(char *dst, const char *color_bytes)
  * We could let the compiler do this, but without C99 initializers
  * the code gets unwieldy and unreadable, so...
  */
-void init_grep_defaults(struct repository *repo)
+void init_grep_defaults(void)
 {
 	struct grep_opt *opt = &grep_defaults;
 	static int run_once;
@@ -67,7 +67,6 @@ void init_grep_defaults(struct repository *repo)
 	run_once++;
 
 	memset(opt, 0, sizeof(*opt));
-	opt->repo = repo;
 	opt->relative = 1;
 	opt->pathname = 1;
 	opt->max_depth = -1;
diff --git a/revision.c b/revision.c
index aa62212040..f35ea1db11 100644
--- a/revision.c
+++ b/revision.c
@@ -1834,7 +1834,7 @@ void repo_init_revisions(struct repository *r,
 	revs->commit_format = CMIT_FMT_DEFAULT;
 	revs->expand_tabs_in_log_default = 8;
 
-	init_grep_defaults(revs->repo);
+	init_grep_defaults();
 	grep_init(&revs->grep_filter, revs->repo, prefix);
 	revs->grep_filter.status_only = 1;
 
-- 
2.29.2.454.gaff20da3a2

[PATCH v3 2/4] grep: use designated initializers for `grep_defaults`

[Martin Ågren]

In 15fabd1bbd ("builtin/grep.c: make configuration callback more
reusable", 2012-10-09), we learned to fill a `static struct grep_opt
grep_defaults` which we can use as a blueprint for other such structs.

At the time, we didn't consider designated initializers to be widely
useable, but these days, we do. (See, e.g., cbc0f81d96 ("strbuf: use
designated initializers in STRBUF_INIT", 2017-07-10).)

Use designated initializers to let the compiler set up the struct and so
that we don't need to remember to call `init_grep_defaults()`.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/MyFirstObjectWalk.txt | 10 +----
 grep.h                              |  1 -
 builtin/grep.c                      |  1 -
 builtin/log.c                       |  1 -
 grep.c                              | 64 +++++++++++------------------
 revision.c                          |  1 -
 6 files changed, 26 insertions(+), 52 deletions(-)

diff --git a/Documentation/MyFirstObjectWalk.txt b/Documentation/MyFirstObjectWalk.txt
index 85434d1938..7f4bffc4dd 100644
--- a/Documentation/MyFirstObjectWalk.txt
+++ b/Documentation/MyFirstObjectWalk.txt
@@ -388,17 +388,9 @@ Next, let's try to filter the commits we see based on their author. This is
 equivalent to running `git log --author=<pattern>`. We can add a filter by
 modifying `rev_info.grep_filter`, which is a `struct grep_opt`.
 
-First some setup. Add `init_grep_defaults()` to `init_walken_defaults()` and add
-`grep_config()` to `git_walken_config()`:
+First some setup. Add `grep_config()` to `git_walken_config()`:
 
 ----
-static void init_walken_defaults(void)
-{
-	init_grep_defaults();
-}
-
-...
-
 static int git_walken_config(const char *var, const char *value, void *cb)
 {
 	grep_config(var, value, cb);
diff --git a/grep.h b/grep.h
index 1c5478f381..b5c4e223a8 100644
--- a/grep.h
+++ b/grep.h
@@ -170,7 +170,6 @@ struct grep_opt {
 	void *output_priv;
 };
 
-void init_grep_defaults(void);
 int grep_config(const char *var, const char *value, void *);
 void grep_init(struct grep_opt *, struct repository *repo, const char *prefix);
 void grep_destroy(void);
diff --git a/builtin/grep.c b/builtin/grep.c
index 2b96efa8c2..ca259af441 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -950,7 +950,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 
-	init_grep_defaults();
 	git_config(grep_cmd_config, NULL);
 	grep_init(&opt, the_repository, prefix);
 
diff --git a/builtin/log.c b/builtin/log.c
index eee4beca4d..cf41714fb0 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -131,7 +131,6 @@ static int log_line_range_callback(const struct option *option, const char *arg,
 
 static void init_log_defaults(void)
 {
-	init_grep_defaults();
 	init_diff_ui_defaults();
 
 	decoration_style = auto_decoration_style();
diff --git a/grep.c b/grep.c
index b351449f7f..8f2009ec9f 100644
--- a/grep.c
+++ b/grep.c
@@ -14,7 +14,31 @@ static int grep_source_load(struct grep_source *gs);
 static int grep_source_is_binary(struct grep_source *gs,
 				 struct index_state *istate);
 
-static struct grep_opt grep_defaults;
+static void std_output(struct grep_opt *opt, const void *buf, size_t size)
+{
+	fwrite(buf, size, 1, stdout);
+}
+
+static struct grep_opt grep_defaults = {
+	.relative = 1,
+	.pathname = 1,
+	.max_depth = -1,
+	.pattern_type_option = GREP_PATTERN_TYPE_UNSPECIFIED,
+	.colors = {
+		[GREP_COLOR_CONTEXT] = "",
+		[GREP_COLOR_FILENAME] = "",
+		[GREP_COLOR_FUNCTION] = "",
+		[GREP_COLOR_LINENO] = "",
+		[GREP_COLOR_COLUMNNO] = "",
+		[GREP_COLOR_MATCH_CONTEXT] = GIT_COLOR_BOLD_RED,
+		[GREP_COLOR_MATCH_SELECTED] = GIT_COLOR_BOLD_RED,
+		[GREP_COLOR_SELECTED] = "",
+		[GREP_COLOR_SEP] = GIT_COLOR_CYAN,
+	},
+	.only_matching = 0,
+	.color = -1,
+	.output = std_output,
+};
 
 #ifdef USE_LIBPCRE2
 static pcre2_general_context *pcre2_global_context;
@@ -42,49 +66,11 @@ static const char *color_grep_slots[] = {
 	[GREP_COLOR_SEP]	    = "separator",
 };
 
-static void std_output(struct grep_opt *opt, const void *buf, size_t size)
-{
-	fwrite(buf, size, 1, stdout);
-}
-
 static void color_set(char *dst, const char *color_bytes)
 {
 	xsnprintf(dst, COLOR_MAXLEN, "%s", color_bytes);
 }
 
-/*
- * Initialize the grep_defaults template with hardcoded defaults.
- * We could let the compiler do this, but without C99 initializers
- * the code gets unwieldy and unreadable, so...
- */
-void init_grep_defaults(void)
-{
-	struct grep_opt *opt = &grep_defaults;
-	static int run_once;
-
-	if (run_once)
-		return;
-	run_once++;
-
-	memset(opt, 0, sizeof(*opt));
-	opt->relative = 1;
-	opt->pathname = 1;
-	opt->max_depth = -1;
-	opt->pattern_type_option = GREP_PATTERN_TYPE_UNSPECIFIED;
-	color_set(opt->colors[GREP_COLOR_CONTEXT], "");
-	color_set(opt->colors[GREP_COLOR_FILENAME], "");
-	color_set(opt->colors[GREP_COLOR_FUNCTION], "");
-	color_set(opt->colors[GREP_COLOR_LINENO], "");
-	color_set(opt->colors[GREP_COLOR_COLUMNNO], "");
-	color_set(opt->colors[GREP_COLOR_MATCH_CONTEXT], GIT_COLOR_BOLD_RED);
-	color_set(opt->colors[GREP_COLOR_MATCH_SELECTED], GIT_COLOR_BOLD_RED);
-	color_set(opt->colors[GREP_COLOR_SELECTED], "");
-	color_set(opt->colors[GREP_COLOR_SEP], GIT_COLOR_CYAN);
-	opt->only_matching = 0;
-	opt->color = -1;
-	opt->output = std_output;
-}
-
 static int parse_pattern_type_arg(const char *opt, const char *arg)
 {
 	if (!strcmp(arg, "default"))
diff --git a/revision.c b/revision.c
index f35ea1db11..963868f699 100644
--- a/revision.c
+++ b/revision.c
@@ -1834,7 +1834,6 @@ void repo_init_revisions(struct repository *r,
 	revs->commit_format = CMIT_FMT_DEFAULT;
 	revs->expand_tabs_in_log_default = 8;
 
-	init_grep_defaults();
 	grep_init(&revs->grep_filter, revs->repo, prefix);
 	revs->grep_filter.status_only = 1;
 
-- 
2.29.2.454.gaff20da3a2

[PATCH v3 3/4] grep: copy struct in one fell swoop

[Martin Ågren]

We have a `struct grep_opt` with our defaults which we then copy into
the caller's struct. Rather than zeroing the target struct and copying
each element one by one, just copy everything at once. This leaves the
code simpler and more maintainable.

We don't have any ownership issues with what we're copying now and can
just greedily copy the whole thing. If and when we do need to handle
such elements (`char *`?), we must and can handle it appropriately. Make
sure to leave a comment to our future selves.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 grep.c | 33 ++++++++++-----------------------
 1 file changed, 10 insertions(+), 23 deletions(-)

diff --git a/grep.c b/grep.c
index 8f2009ec9f..efeb6dc58d 100644
--- a/grep.c
+++ b/grep.c
@@ -66,11 +66,6 @@ static const char *color_grep_slots[] = {
 	[GREP_COLOR_SEP]	    = "separator",
 };
 
-static void color_set(char *dst, const char *color_bytes)
-{
-	xsnprintf(dst, COLOR_MAXLEN, "%s", color_bytes);
-}
-
 static int parse_pattern_type_arg(const char *opt, const char *arg)
 {
 	if (!strcmp(arg, "default"))
@@ -100,6 +95,14 @@ int grep_config(const char *var, const char *value, void *cb)
 	if (userdiff_config(var, value) < 0)
 		return -1;
 
+	/*
+	 * The instance of grep_opt that we set up here is copied by
+	 * grep_init() to be used by each individual invocation.
+	 * When populating a new field of this structure here, be
+	 * sure to think about ownership -- e.g., you might need to
+	 * override the shallow copy in grep_init() with a deep copy.
+	 */
+
 	if (!strcmp(var, "grep.extendedregexp")) {
 		opt->extended_regexp_option = git_config_bool(var, value);
 		return 0;
@@ -157,9 +160,6 @@ int grep_config(const char *var, const char *value, void *cb)
  */
 void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix)
 {
-	struct grep_opt *def = &grep_defaults;
-	int i;
-
 #if defined(USE_LIBPCRE2)
 	if (!pcre2_global_context)
 		pcre2_global_context = pcre2_general_context_create(
@@ -171,26 +171,13 @@ void grep_init(struct grep_opt *opt, struct repository *repo, const char *prefix
 	pcre_free = free;
 #endif
 
-	memset(opt, 0, sizeof(*opt));
+	*opt = grep_defaults;
+
 	opt->repo = repo;
 	opt->prefix = prefix;
 	opt->prefix_length = (prefix && *prefix) ? strlen(prefix) : 0;
 	opt->pattern_tail = &opt->pattern_list;
 	opt->header_tail = &opt->header_list;
-
-	opt->only_matching = def->only_matching;
-	opt->color = def->color;
-	opt->extended_regexp_option = def->extended_regexp_option;
-	opt->pattern_type_option = def->pattern_type_option;
-	opt->linenum = def->linenum;
-	opt->columnnum = def->columnnum;
-	opt->max_depth = def->max_depth;
-	opt->pathname = def->pathname;
-	opt->relative = def->relative;
-	opt->output = def->output;
-
-	for (i = 0; i < NR_GREP_COLORS; i++)
-		color_set(opt->colors[i], def->colors[i]);
 }
 
 void grep_destroy(void)
-- 
2.29.2.454.gaff20da3a2

[PATCH v3 4/4] MyFirstObjectWalk: drop `init_walken_defaults()`

[Martin Ågren]

In a recent commit, we stopped calling `init_grep_defaults()` from this
function. Thus, by the end of the tutorial, we still haven't added any
contents to this function. Let's remove it for simplicity.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/MyFirstObjectWalk.txt | 24 ------------------------
 1 file changed, 24 deletions(-)

diff --git a/Documentation/MyFirstObjectWalk.txt b/Documentation/MyFirstObjectWalk.txt
index 7f4bffc4dd..2d10eea7a9 100644
--- a/Documentation/MyFirstObjectWalk.txt
+++ b/Documentation/MyFirstObjectWalk.txt
@@ -182,30 +182,6 @@ its `init_log_defaults()` sets its own state (`decoration_style`) and asks
 `grep` and `diff` to initialize themselves by calling each of their
 initialization functions.
 
-For our first example within `git walken`, we don't intend to use any other
-components within Git, and we don't have any configuration to do.  However, we
-may want to add some later, so for now, we can add an empty placeholder. Create
-a new function in `builtin/walken.c`:
-
-----
-static void init_walken_defaults(void)
-{
-	/*
-	 * We don't actually need the same components `git log` does; leave this
-	 * empty for now.
-	 */
-}
-----
-
-Make sure to add a line invoking it inside of `cmd_walken()`.
-
-----
-int cmd_walken(int argc, const char **argv, const char *prefix)
-{
-	init_walken_defaults();
-}
-----
-
 ==== Configuring From `.gitconfig`
 
 Next, we should have a look at any relevant configuration settings (i.e.,
-- 
2.29.2.454.gaff20da3a2

Re: [PATCH v3 0/4] grep: simplify "grep defaults" handling

[Junio C Hamano]

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

> This is v3 of my series [1] [2] to simplify the setup and use of the
> `struct grep_opt grep_defaults`. The only difference compared to v2 is
> an added comment in the third patch.

Thanks, will queue.