On Mon, Nov 08 2021, Taylor Blau wrote:
> On Sat, Nov 06, 2021 at 10:10:52PM +0100, Ævar Arnfjörð Bjarmason wrote:
>> 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...
> [...]
>> 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.
> [...]
>> -/*
>> - * 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".
