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

>
>
> On Mon, Oct 04 2021, Elijah Newren wrote:
>
> > On Sat, Oct 2, 2021 at 2:07 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> >>
> >> On Fri, Oct 01 2021, Elijah Newren wrote:
> >>
> > ...
> >> > So maybe I'll submit some patches on top that rip these direct members
> >> > out of of unpack_trees_options and push them inside some opaque
> >> > struct.
> >>
> >> Sure, that sounds good. I only had a mild objection to doing it in a way
> >> where you'll need that sort of code I removed in the linked commit in
> >> prep_exclude() because you were trying not to expose that at any cost,
> >> including via some *_INIT macro. I.e. if it's private we can just name
> >> it "priv_*" or have a :
> >>
> >>     struct dont_touch_this {
> >>         struct dir_struct dir;
> >>     };
> >>
> >> Which are both ways of /messaging/ that it's private, and since the
> >> target audience is just the rest of the git.git codebase I think that
> >> ultimately something that 1) sends the right message 2) makes accidents
> >> pretty much impossible suffices. I.e. you don't accidentally introduce a
> >> new API user accessing a field called "->priv_*" or
> >> "->private_*". Someone will review those patches...
> >
> > An internal struct with all the members meant to be internal-only
> > provides nearly all the advantages that I was going for with the
> > opaque struct, while also being a smaller change than what I was
> > thinking of doing.  I like that idea; thanks for the suggestion.
>
> Yeah, just to provide an explicit example something like the below. It
> compiles to the same assembly (at least under -O3, didn't exhaustively
> try other optimization levels).
>
> I'm rather "meh" on it v.s. just prefixing the relevant member names
> with "priv_" or "private_", but it results in the same semantics &
> machine code, so it's effectively just a way of doing the labeling for
> human consumption.
>
> diff --git a/dir.c b/dir.c
> index 39fce3bcba7..a714640e782 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -1533,12 +1533,12 @@ static void prep_exclude(struct dir_struct *dir,
>          * which originate from directories not in the prefix of the
>          * path being checked.
>          */
> -       while ((stk = dir->exclude_stack) != NULL) {
> +       while ((stk = dir->private.exclude_stack) != NULL) {
>                 if (stk->baselen <= baselen &&
>                     !strncmp(dir->basebuf.buf, base, stk->baselen))
>                         break;
> -               pl = &group->pl[dir->exclude_stack->exclude_ix];
> -               dir->exclude_stack = stk->prev;
> +               pl = &group->pl[dir->private.exclude_stack->exclude_ix];
> +               dir->private.exclude_stack = stk->prev;
>                 dir->pattern = NULL;
>                 free((char *)pl->src); /* see strbuf_detach() below */
>                 clear_pattern_list(pl);
> @@ -1584,7 +1584,7 @@ static void prep_exclude(struct dir_struct *dir,
>                                                  base + current,
>                                                  cp - base - current);
>                 }
> -               stk->prev = dir->exclude_stack;
> +               stk->prev = dir->private.exclude_stack;
>                 stk->baselen = cp - base;
>                 stk->exclude_ix = group->nr;
>                 stk->ucd = untracked;
> @@ -1605,7 +1605,7 @@ static void prep_exclude(struct dir_struct *dir,
>                             dir->pattern->flags & PATTERN_FLAG_NEGATIVE)
>                                 dir->pattern = NULL;
>                         if (dir->pattern) {
> -                               dir->exclude_stack = stk;
> +                               dir->private.exclude_stack = stk;
>                                 return;
>                         }
>                 }
> @@ -1662,7 +1662,7 @@ static void prep_exclude(struct dir_struct *dir,
>                         invalidate_gitignore(dir->untracked, untracked);
>                         oidcpy(&untracked->exclude_oid, &oid_stat.oid);
>                 }
> -               dir->exclude_stack = stk;
> +               dir->private.exclude_stack = stk;
>                 current = stk->baselen;
>         }
>         strbuf_setlen(&dir->basebuf, baselen);
> @@ -3302,7 +3302,7 @@ void dir_clear(struct dir_struct *dir)
>         free(dir->ignored);
>         free(dir->entries);
>
> -       stk = dir->exclude_stack;
> +       stk = dir->private.exclude_stack;
>         while (stk) {
>                 struct exclude_stack *prev = stk->prev;
>                 free(stk);
> diff --git a/dir.h b/dir.h
> index 83f46c0fb4c..d30d294308d 100644
> --- a/dir.h
> +++ b/dir.h
> @@ -209,6 +209,11 @@ struct untracked_cache {
>   * record the paths discovered. A single `struct dir_struct` is used regardless
>   * of whether or not the traversal recursively descends into subdirectories.
>   */
> +
> +struct dir_struct_private {
> +       struct exclude_stack *exclude_stack;
> +};
> +
>  struct dir_struct {
>
>         /* The number of members in `entries[]` array. */
> @@ -327,7 +332,7 @@ struct dir_struct {
>          * (sub)directory in the traversal. Exclude points to the
>          * matching exclude struct if the directory is excluded.
>          */
> -       struct exclude_stack *exclude_stack;
> +       struct dir_struct_private private;
>         struct path_pattern *pattern;
>         struct strbuf basebuf;