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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Oct 4, 2021, 14:07 UTC
Message-ID: <8735pgop57.fsf@evledraar.gmail.com>
In-Reply-To: <CABPp-BFE_5zPCZY8adJSQchdQzxq3uH-oma-S=_Sw0OUXx03OQ@mail.gmail.com>

On Mon, Oct 04 2021, Elijah Newren wrote:

Show 30 quoted lines

> 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;