Re: [PATCH v3 04/11] unpack-trees: introduce preserve_ignored to unpack_trees_options

[Elijah Newren <newren@gmail.com>]

On Wed, Sep 29, 2021 at 11:32 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
> On Wed, Sep 29 2021, Elijah Newren wrote:
>
> > On Wed, Sep 29, 2021 at 2:27 AM Ævar Arnfjörð Bjarmason
> > <avarab@gmail.com> wrote:
> >>
...
> >>
> >> I think getting rid of the boilerplate makes sense, but it doesn't sound
> >> from the commit message like you've considered just making that "struct
> >> dir*" member a "struct dir" instead.
> >>
> >> That simplifies things a lot, i.e. we can just DIR_INIT it, and don't
> >> need every caller to malloc/free it.
> >
> > See the next patch in the series.  :-)
>
> Ah!
>
> >> Sometimes a pointer makes sense, but in this case the "struct
> >> unpack_trees_options" can just own it.
> >
> > I did make it internal to unpack_trees_options in the next patch, but
> > kept it as a pointer just because that let me know whether it was used
> > or not.  I guess I could have added a boolean as well.  But I don't
> > actually allocate anything, because it's either a NULL pointer, or a
> > pointer to something on the stack.  So, I do get to just use DIR_INIT.
>
> I think I'm probably missing something. I just made it allocated on the
> stack by the caller using "struct unpack_trees_options", but then you
> end up having a dir* in the struct, but that's only filled in as a
> pointer to the stack variable? Maybe there's some subtlety I'm missing
> here...

As per the next patch:

int unpack_trees(..., struct unpack_trees_options *o)
{
    struct dir_struct dir = DIR_INIT;
    ...
    if (!o->preserve_ignored) {
        /* Setup 'dir', make o->dir point to it */
        ....
        o->dir = &dir;
    }
    ...
    if (o->dir)
        /* cleanup */
    ....
}

The caller doesn't touch o->dir (other than initializing it to zeros);
unpack_trees() is wholly responsible for it.  I'd kind of like to
entirely remove dir from unpack_trees_options(), but I need a way of
passing it down through all the other functions in unpack-trees.c, and
leaving it in unpack_trees_options seems the easiest way to do so.  So
I just marked it as "for internal use only".