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

From: Elijah Newren <newren@gmail.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: Elijah Newren via GitGitGadget <gitgitgadget@gmail.com>,
	Git Mailing List <git@vger.kernel.org>,
	Fedor Biryukov <fedor.birjukov@gmail.com>,
	Philip Oakley <philipoakley@iee.email>,
	Phillip Wood <phillip.wood123@gmail.com>
Subject: Re: [PATCH v3 04/11] unpack-trees: introduce preserve_ignored to unpack_trees_options
Date: Mon, 4 Oct 2021 07:57:38 -0700	[thread overview]
Message-ID: <CABPp-BHgot=CPNyK_xNfog_SqsNPNoCGfiSb-gZoS2sn_741dQ@mail.gmail.com> (raw)
In-Reply-To: <8735pgop57.fsf@evledraar.gmail.com>

On Mon, Oct 4, 2021 at 7:12 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>
>
> 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;

Yeah, that doesn't help much at all, and I'd argue even makes things
worse, because you're just looking at a single member.  This subtly
implies that all the other private variables are public API.  The
dir.h portion of the patch should look more like this:

$ git diff -w dir.h

diff --git a/dir.h b/dir.h
index 83f46c0fb4..93a9f02688 100644
--- a/dir.h
+++ b/dir.h
@@ -214,14 +214,9 @@ struct dir_struct {
        /* The number of members in `entries[]` array. */
        int nr;

-       /* Internal use; keeps track of allocation of `entries[]` array.*/
-       int alloc;
-
        /* The number of members in `ignored[]` array. */
        int ignored_nr;

-       int ignored_alloc;
-
        /* bit-field of options */
        enum {

@@ -301,11 +296,19 @@ struct dir_struct {
         */
        const char *exclude_per_dir;

+       struct dir_struct_internal {
+               /* Keeps track of allocation of `entries[]` array.*/
+               int alloc;
+
+               /* Keeps track of allocation of `ignored[]` array. */
+               int ignored_alloc;
+
                /*
                 * We maintain three groups of exclude pattern lists:
                 *
                 * EXC_CMDL lists patterns explicitly given on the command line.
-        * EXC_DIRS lists patterns obtained from per-directory ignore files.
+                * EXC_DIRS lists patterns obtained from per-directory ignore
+                *          files.
                 * EXC_FILE lists patterns from fallback ignore files, e.g.
                 *   - .git/info/exclude
                 *   - core.excludesfile
@@ -340,6 +343,7 @@ struct dir_struct {
                /* Stats about the traversal */
                unsigned visited_paths;
                unsigned visited_directories;
+       } internal;
 };

 #define DIR_INIT { 0 }


The above change would make it clear that there are 12 variables meant
for use only within dir.c that external callers should not be
initializing or reading for output after the fact -- and only 6 that
are part of the public API that they need worry about.  It also makes
it easier for folks messing with dir.c to know which parts are just
internal state management, which I think would have made it easier to
understand the weird basebuf/exclude_stack stuff in prep_exclude()
that you nicely tracked down.  But overall, I'm really most happy
about the part of this patch that lets external callers realize they
only need to worry about 6 out of 18 fields and that they can ignore
the rest.

unpack_trees_options should have something similar done with it, and
maybe some others.
