Re: [PATCH 6/8] grep API: call grep_config() after grep_init()

On Mon, Nov 08 2021, Taylor Blau wrote:

On Sat, Nov 06, 2021 at 10:10:52PM +0100, Ævar Arnfjörð Bjarmason wrote:

quoted

The grep_init() function used the odd pattern of initializing the
passed-in "struct grep_opt" with a statically defined "grep_defaults"
struct, which would be modified in-place when we invoked
grep_config().

So we effectively (b) initialized config, (a) then defaults, (c)
followed by user options. Usually those are ordered as "a", "b" and
"c" instead.

Do we risk changing any user-visible behavior here? Based on my reading
of grep.c before and after this patch, I think the answer is "no", but I
wasn't sure if you had done a similar analysis.

In any case, I think the "bring your own structure" instead of getting
one copied around is much easier to reason about. Even if we weren't
accidentally stomping on ownership of the struct before, not having to
reason about it is a nice benefit.

I don't think we're changing any behavior except the one noted in this
series.

We only set a few config variables, so I thought that was fairly easy to
trace...

[...]

quoted

diff --git a/builtin/grep.c b/builtin/grep.c
index 960c7aac123..7f95f44e948 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c

@@ -288,7 +288,7 @@ static int wait_all(void)
 static int grep_cmd_config(const char *var, const char *value, void *cb)
 {
 	int st = grep_config(var, value, cb);
-	if (git_color_default_config(var, value, cb) < 0)
+	if (git_color_default_config(var, value, NULL) < 0)

This doesn't appear strictly related to the rest of your changes, but
only serves to prevent the caller-provided data from being sent down to
git_color_default_config().

It didn't matter before because (a) the caller doesn't specify any data
to begin with, and git_color_default_config() (or the functions that it
calls) don't do anything with the extra pointer. Now cmd_grep() is going
to start passing around a pointer to a struct grep_opt.

But git_color_default_config() still doesn't do anything with the
pointer it receives, and passing that pointer around is standard
practice among config.c code. So I don't think that this hunk is
strictly necessary, and it's somewhat different than the pattern
established within config.c.

I wouldn't be sad to see this hunk dropped (and in fact have a slight
preference leaning this way), but I don't mind keeping it around,
either.

Will either split it up or drop it.

[...]

quoted

-/*
- * Initialize one instance of grep_opt and copy the
- * default values from the template we read the configuration
- * information in an earlier call to git_config(grep_config).
- */
 void grep_init(struct grep_opt *opt, struct repository *repo)
 {
-	*opt = grep_defaults;
+	struct grep_opt blank = GREP_OPT_INIT;
+	memcpy(opt, &blank, sizeof(*opt));

I'm nit-picking, but creating a throwaway struct for the convenience of
using designated initialization (at the cost of having to memcpy an
entire struct around) seems like overkill.

Especially since we're just going to write into the other fields of the
the target struct anyway, I'd probably rather have seen everything
written out explicitly without the throwaway or memcpy.

It's a widely used pattern in the codebase at this point, see
5726a6b4012 (*.c *_init(): define in terms of corresponding *_INIT
macro, 2021-07-01) (mine, but I stole it from Jeff King).

As his linked-to compiler test shows the memcpy() is optimized away, so
modern compilers will treat these idioms the same way.

There was a suggestions somewhere that we should prorably move to that
"*<x> = <y>" or whatever it was briefer C99 (I think) syntax across the
board, it would be less verbose. But I haven't tested if it's as widely
supported, so I've just been sticking with that blank/memcpy() pattern
for "do init in terms of macro".