threads / patch / 54687

patch, 4 partsgrep: retire `init_grep_defaults()`

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

## tl;dr

26 messages between Nov 21, 2020 and Dec 1, 2020. Diffs are folded; open one to read it.

replies: 25people: 5as markdown or json

Martin Ågren· Nov 21, 2020, 18:31 UTC · lore

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
Martin Ågren· Nov 21, 2020, 18:31 UTC · re: Martin Ågren · lore

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

`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(-)
Show changes to 6 files +6 −7

Documentation/MyFirstObjectWalk.txt, grep.h, builtin/grep.c, builtin/log.c, grep.c, revision.c

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
Martin Ågren· Nov 21, 2020, 18:31 UTC · re: Martin Ågren · lore

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

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(-)
Show changes to 6 files +26 −52

Documentation/MyFirstObjectWalk.txt, grep.h, builtin/grep.c, builtin/log.c, grep.c, revision.c

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
Martin Ågren· Nov 21, 2020, 18:31 UTC · re: Martin Ågren · lore

[PATCH 3/4] grep: simplify color setup

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(-)
Show changes to grep.c +1 −8
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
Jeff King· Nov 21, 2020, 20:23 UTC · re: Martin Ågren · lore

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

On Sat, Nov 21, 2020 at 07:31:09PM +0100, Martin Ågren wrote:
Show 10 quoted lines
> 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
Martin Ågren· Nov 21, 2020, 20:52 UTC · re: Jeff King · lore

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

On Sat, 21 Nov 2020 at 21:23, Jeff King <peff@peff.net> wrote:
Show 16 quoted lines
>
> 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
Junio C Hamano· Nov 21, 2020, 22:46 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 15 quoted lines
> 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.
Show 7 quoted lines
>>  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.

Jeff King· Nov 24, 2020, 06:54 UTC · re: Junio C Hamano · lore

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

On Sat, Nov 21, 2020 at 02:46:14PM -0800, Junio C Hamano wrote:
Show 12 quoted lines
> >>  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
Martin Ågren· Nov 21, 2020, 18:31 UTC · re: Martin Ågren · lore

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

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(-)
Show changes to Documentation/MyFirstObjectWalk.txt +0 −20
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
Johannes Schindelin· Nov 23, 2020, 11:03 UTC · re: Martin Ågren · lore

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

Hi Martin,
On Sat, 21 Nov 2020, Martin Ågren wrote:
Show 6 quoted lines
> 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

Martin Ågren· Nov 24, 2020, 21:04 UTC · re: Martin Ågren · lore

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

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
Martin Ågren· Nov 24, 2020, 21:04 UTC · re: Martin Ågren · lore

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

`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(-)
Show changes to 6 files +6 −7

Documentation/MyFirstObjectWalk.txt, grep.h, builtin/grep.c, builtin/log.c, grep.c, revision.c

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
Martin Ågren· Nov 24, 2020, 21:04 UTC · re: Martin Ågren · lore

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

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(-)
Show changes to 6 files +26 −52

Documentation/MyFirstObjectWalk.txt, grep.h, builtin/grep.c, builtin/log.c, grep.c, revision.c

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
Martin Ågren· Nov 24, 2020, 21:04 UTC · re: Martin Ågren · lore

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

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(-)
Show changes to grep.c +2 −23
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
Junio C Hamano· Nov 24, 2020, 22:34 UTC · re: Martin Ågren · lore

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

Martin Ågren <martin.agren@gmail.com> writes:
Show 8 quoted lines
> 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:

Show changes to diff +8 −0
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;
Martin Ågren· Nov 25, 2020, 06:25 UTC · re: Junio C Hamano · lore

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

On Tue, 24 Nov 2020 at 23:34, Junio C Hamano <gitster@pobox.com> wrote:
Show 44 quoted lines
>
> 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

Junio C Hamano· Nov 25, 2020, 07:53 UTC · re: Martin Ågren · lore

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

Martin Ågren <martin.agren@gmail.com> writes:
Show 19 quoted lines
> 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.

Show 7 quoted lines
>   /*
>    * 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.
Martin Ågren· Nov 26, 2020, 20:25 UTC · re: Junio C Hamano · lore

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

On Wed, 25 Nov 2020 at 08:53, Junio C Hamano <gitster@pobox.com> wrote:
Show 20 quoted lines
>
> 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
Martin Ågren· Nov 24, 2020, 21:04 UTC · re: Martin Ågren · lore

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

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(-)
Show changes to Documentation/MyFirstObjectWalk.txt +0 −20
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
Ævar Arnfjörð Bjarmason· Nov 25, 2020, 09:27 UTC · re: Martin Ågren · lore

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

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.
Martin Ågren· Nov 29, 2020, 19:52 UTC · re: Martin Ågren · lore

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

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
Martin Ågren· Nov 29, 2020, 19:52 UTC · re: Martin Ågren · lore

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

`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(-)
Show changes to 6 files +6 −7

Documentation/MyFirstObjectWalk.txt, grep.h, builtin/grep.c, builtin/log.c, grep.c, revision.c

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
Martin Ågren· Nov 29, 2020, 19:52 UTC · re: Martin Ågren · lore

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

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(-)
Show changes to 6 files +26 −52

Documentation/MyFirstObjectWalk.txt, grep.h, builtin/grep.c, builtin/log.c, grep.c, revision.c

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
Martin Ågren· Nov 29, 2020, 19:52 UTC · re: Martin Ågren · lore

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

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(-)
Show changes to grep.c +10 −23
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
Martin Ågren· Nov 29, 2020, 19:52 UTC · re: Martin Ågren · lore

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

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(-)
Show changes to Documentation/MyFirstObjectWalk.txt +0 −20
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
Junio C Hamano· Dec 1, 2020, 04:46 UTC · re: Martin Ågren · lore

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

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.

← back to recent threads