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

On Mon, Oct 04 2021, Elijah Newren wrote:

quoted

On Sat, Oct 2, 2021 at 2:07 AM Ævar Arnfjörð Bjarmason [off-list ref] wrote:

quoted

On Fri, Oct 01 2021, Elijah Newren wrote:

...

quoted

quoted

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;