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

From: Martin Ågren <martin.agren@gmail.com>
Date: Nov 26, 2020, 20:25 UTC
Message-ID: <CAN0heSp2L4Q0pcn8V5hAKzXF6szZgpGqncLNYZOq5tmM7EucOQ@mail.gmail.com>
In-Reply-To: <xmqqlfepg9dq.fsf@gitster.c.googlers.com>

On Wed, 25 Nov 2020 at 08:53, Junio C Hamano <gitster@pobox.com> wrote:

Show 20 quoted lines

>
> Martin Ågren <martin.agren@gmail.com> writes:
>
> >   /*
> >    * 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.

I understand your concern. Here's what I'm considering using:

  /*
   * 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 -- e.g., you might need to
   * override the shallow copy in grep_init() with a deep copy.
   */

I'm not going into the releasing of those resources, but hopefully that part can be left to the reader.

Other suggestions welcome, of course. I hope to get around to rerolling this on the weekend.

Martin