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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 25, 2020, 07:53 UTC
Message-ID: <xmqqlfepg9dq.fsf@gitster.c.googlers.com>
In-Reply-To: <CAN0heSoM+qQe8BdKHVpqhA0RAqzyyL3Qr98G=O8kD504diruCg@mail.gmail.com>

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.