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

From: Martin Ågren <martin.agren@gmail.com>
Date: Nov 25, 2020, 06:25 UTC
Message-ID: <CAN0heSoM+qQe8BdKHVpqhA0RAqzyyL3Qr98G=O8kD504diruCg@mail.gmail.com>
In-Reply-To: <xmqqsg8ygza7.fsf@gitster.c.googlers.com>

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