On Tue, 24 Nov 2020 at 23:34, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> Martin Ågren <martin.agren@xxxxxxxxx> 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
