Re: [PATCH v3 04/11] unpack-trees: introduce preserve_ignored to unpack_trees_options
flat view
From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2021-10-04 14:13:04
Subsystem:
the rest · Maintainer:
Linus Torvalds
On Mon, Oct 04 2021, Elijah Newren wrote:
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;