[PATCH 0/2] dir: update outdated fields and comments about oid_stat

[PATCH 0/2] dir: update outdated fields and comments about oid_stat

[Matheus Tavares]

In 4b33e60 ("dir: convert struct sha1_stat to use object_id",
2018-01-28), we renamed "struct sha1_stat" to "struct oid_stat".
However, there are still some comments and struct field names which
refer to the old sha1_stat. The two patches in this series update those
places to avoid confusion.

Matheus Tavares (2):
  dir: fix outdated comment on add_patterns()
  dir: improve naming of oid_stat fields in two structs

 dir.c                                | 35 ++++++++++++++--------------
 dir.h                                |  8 +++----
 t/helper/test-dump-untracked-cache.c |  4 ++--
 3 files changed, 24 insertions(+), 23 deletions(-)

-- 
2.25.0

[PATCH 1/2] dir: fix outdated comment on add_patterns()

[Matheus Tavares]

In 4b33e60 ("dir: convert struct sha1_stat to use object_id",
2018-01-28), the struct sha1_stat was converted to oid_stat. In this
process, add_patterns() also learned to use the new struct definition.
However, the comments in this function still refer to "ss" (i.e. the old
sha1_stat). Update that.

Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
---
 dir.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/dir.c b/dir.c
index 0ffb1b3302..ab2e5b8717 100644
--- a/dir.c
+++ b/dir.c
@@ -1015,9 +1015,10 @@ static int add_patterns_from_buffer(char *buf, size_t size,
  * an index if 'istate' is non-null), parse it and store the
  * exclude rules in "pl".
  *
- * If "ss" is not NULL, compute SHA-1 of the exclude file and fill
+ * If oid_stat is not NULL, compute OID of the exclude file and fill
  * stat data from disk (only valid if add_patterns returns zero). If
- * ss_valid is non-zero, "ss" must contain good value as input.
+ * oid_stat->valid is non-zero, oid_stat must contain good value as
+ * input.
  */
 static int add_patterns(const char *fname, const char *base, int baselen,
 			struct pattern_list *pl, struct index_state *istate,
@@ -1065,7 +1066,7 @@ static int add_patterns(const char *fname, const char *base, int baselen,
 			int pos;
 			if (oid_stat->valid &&
 			    !match_stat_data_racy(istate, &oid_stat->stat, &st))
-				; /* no content change, ss->sha1 still good */
+				; /* no content change, oid_stat->oid still good */
 			else if (istate &&
 				 (pos = index_name_pos(istate, fname, strlen(fname))) >= 0 &&
 				 !ce_stage(istate->cache[pos]) &&
-- 
2.25.0

[PATCH 2/2] dir: improve naming of oid_stat fields in two structs

[Matheus Tavares]

Both "struct untracked_cache" and "struct dir_struct" contain fields of
the type "struct oid_stat". The latter used to be called "sha1_stat"
before 4b33e60 ("dir: convert struct sha1_stat to use object_id",
2018-01-28). Although this struct was renamed, the previously
mentioned fields are still named in the format "ss_*", which refers to
the old "sha1_stat". Since this might be confusing, rename those fields
as "st_*", referring to "stat", instead.

Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
---

Note: I choosed to use "st_*", as naming, for simplicity, and to keep
the code lines small. But should we use a more verbose "oidst_*" format,
instead, to avoid confusions with "struct stat"?

 dir.c                                | 28 ++++++++++++++--------------
 dir.h                                |  8 ++++----
 t/helper/test-dump-untracked-cache.c |  4 ++--
 3 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/dir.c b/dir.c
index ab2e5b8717..e678c8184c 100644
--- a/dir.c
+++ b/dir.c
@@ -2613,15 +2613,15 @@ static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *d
 
 	/* Validate $GIT_DIR/info/exclude and core.excludesfile */
 	root = dir->untracked->root;
-	if (!oideq(&dir->ss_info_exclude.oid,
-		   &dir->untracked->ss_info_exclude.oid)) {
+	if (!oideq(&dir->st_info_exclude.oid,
+		   &dir->untracked->st_info_exclude.oid)) {
 		invalidate_gitignore(dir->untracked, root);
-		dir->untracked->ss_info_exclude = dir->ss_info_exclude;
+		dir->untracked->st_info_exclude = dir->st_info_exclude;
 	}
-	if (!oideq(&dir->ss_excludes_file.oid,
-		   &dir->untracked->ss_excludes_file.oid)) {
+	if (!oideq(&dir->st_excludes_file.oid,
+		   &dir->untracked->st_excludes_file.oid)) {
 		invalidate_gitignore(dir->untracked, root);
-		dir->untracked->ss_excludes_file = dir->ss_excludes_file;
+		dir->untracked->st_excludes_file = dir->st_excludes_file;
 	}
 
 	/* Make sure this directory is not dropped out at saving phase */
@@ -2884,14 +2884,14 @@ void setup_standard_excludes(struct dir_struct *dir)
 		excludes_file = xdg_config_home("ignore");
 	if (excludes_file && !access_or_warn(excludes_file, R_OK, 0))
 		add_patterns_from_file_1(dir, excludes_file,
-					 dir->untracked ? &dir->ss_excludes_file : NULL);
+					 dir->untracked ? &dir->st_excludes_file : NULL);
 
 	/* per repository user preference */
 	if (startup_info->have_repository) {
 		const char *path = git_path_info_exclude();
 		if (!access_or_warn(path, R_OK, 0))
 			add_patterns_from_file_1(dir, path,
-						 dir->untracked ? &dir->ss_info_exclude : NULL);
+						 dir->untracked ? &dir->st_info_exclude : NULL);
 	}
 }
 
@@ -3037,8 +3037,8 @@ void write_untracked_extension(struct strbuf *out, struct untracked_cache *untra
 	const unsigned hashsz = the_hash_algo->rawsz;
 
 	ouc = xcalloc(1, sizeof(*ouc));
-	stat_data_to_disk(&ouc->info_exclude_stat, &untracked->ss_info_exclude.stat);
-	stat_data_to_disk(&ouc->excludes_file_stat, &untracked->ss_excludes_file.stat);
+	stat_data_to_disk(&ouc->info_exclude_stat, &untracked->st_info_exclude.stat);
+	stat_data_to_disk(&ouc->excludes_file_stat, &untracked->st_excludes_file.stat);
 	ouc->dir_flags = htonl(untracked->dir_flags);
 
 	varint_len = encode_varint(untracked->ident.len, varbuf);
@@ -3046,8 +3046,8 @@ void write_untracked_extension(struct strbuf *out, struct untracked_cache *untra
 	strbuf_addbuf(out, &untracked->ident);
 
 	strbuf_add(out, ouc, sizeof(*ouc));
-	strbuf_add(out, untracked->ss_info_exclude.oid.hash, hashsz);
-	strbuf_add(out, untracked->ss_excludes_file.oid.hash, hashsz);
+	strbuf_add(out, untracked->st_info_exclude.oid.hash, hashsz);
+	strbuf_add(out, untracked->st_excludes_file.oid.hash, hashsz);
 	strbuf_add(out, untracked->exclude_per_dir, strlen(untracked->exclude_per_dir) + 1);
 	FREE_AND_NULL(ouc);
 
@@ -3250,10 +3250,10 @@ struct untracked_cache *read_untracked_extension(const void *data, unsigned long
 	uc = xcalloc(1, sizeof(*uc));
 	strbuf_init(&uc->ident, ident_len);
 	strbuf_add(&uc->ident, ident, ident_len);
-	load_oid_stat(&uc->ss_info_exclude,
+	load_oid_stat(&uc->st_info_exclude,
 		      next + ouc_offset(info_exclude_stat),
 		      next + offset);
-	load_oid_stat(&uc->ss_excludes_file,
+	load_oid_stat(&uc->st_excludes_file,
 		      next + ouc_offset(excludes_file_stat),
 		      next + offset + hashsz);
 	uc->dir_flags = get_be32(next + ouc_offset(dir_flags));
diff --git a/dir.h b/dir.h
index 5855c065a6..4d30816271 100644
--- a/dir.h
+++ b/dir.h
@@ -186,8 +186,8 @@ struct untracked_cache_dir {
 };
 
 struct untracked_cache {
-	struct oid_stat ss_info_exclude;
-	struct oid_stat ss_excludes_file;
+	struct oid_stat st_info_exclude;
+	struct oid_stat st_excludes_file;
 	const char *exclude_per_dir;
 	struct strbuf ident;
 	/*
@@ -334,8 +334,8 @@ struct dir_struct {
 
 	/* Enable untracked file cache if set */
 	struct untracked_cache *untracked;
-	struct oid_stat ss_info_exclude;
-	struct oid_stat ss_excludes_file;
+	struct oid_stat st_info_exclude;
+	struct oid_stat st_excludes_file;
 	unsigned unmanaged_exclude_files;
 };
 
diff --git a/t/helper/test-dump-untracked-cache.c b/t/helper/test-dump-untracked-cache.c
index cf0f2c7228..71a47e974b 100644
--- a/t/helper/test-dump-untracked-cache.c
+++ b/t/helper/test-dump-untracked-cache.c
@@ -56,8 +56,8 @@ int cmd__dump_untracked_cache(int ac, const char **av)
 		printf("no untracked cache\n");
 		return 0;
 	}
-	printf("info/exclude %s\n", oid_to_hex(&uc->ss_info_exclude.oid));
-	printf("core.excludesfile %s\n", oid_to_hex(&uc->ss_excludes_file.oid));
+	printf("info/exclude %s\n", oid_to_hex(&uc->st_info_exclude.oid));
+	printf("core.excludesfile %s\n", oid_to_hex(&uc->st_excludes_file.oid));
 	printf("exclude_per_dir %s\n", uc->exclude_per_dir);
 	printf("flags %08x\n", uc->dir_flags);
 	if (uc->root)
-- 
2.25.0

Re: [PATCH 2/2] dir: improve naming of oid_stat fields in two structs

[Junio C Hamano]

Matheus Tavares <matheus.bernardino@usp.br> writes:

> Note: I choosed to use "st_*", as naming, for simplicity, and to keep
> the code lines small. But should we use a more verbose "oidst_*" format,
> instead, to avoid confusions with "struct stat"?
> ...
> @@ -334,8 +334,8 @@ struct dir_struct {
>  
>  	/* Enable untracked file cache if set */
>  	struct untracked_cache *untracked;
> -	struct oid_stat ss_info_exclude;
> -	struct oid_stat ss_excludes_file;
> +	struct oid_stat st_info_exclude;
> +	struct oid_stat st_excludes_file;
>  	unsigned unmanaged_exclude_files;
>  };

I tend to agree with you that using prefix "st_" for anything other
than "struct stat" proper would be confusing.  If "ss" used to stand
for "sha1 stat", I would expect "oid stat" to abbreviate to "os", at
least.

I also am wondering if we can do without any prefix, i.e. just call
them "info_exclude" and "excludes_file", because the field names are
private to each struct and there is no strong reason to have a
common prefix among fields in a single struct.  Rather, it is more
important for the fields of the same type in a single struct to have
distinct names so that the reader can easily tell them apart and the
reason for their use is straight-forward to understand, and the two
names without any prefix convey their distinction pretty well, I
would think.

It is not like we adopt a convention to name our variables with
abbreviated typenames embedded in the variable names.  We shouldn't
be doing that for field names, either, but it smells that the "give
them prefix ss_ because they are of type sha1_stat" pretty much has
origin in such school.

Thanks.

Re: [PATCH 2/2] dir: improve naming of oid_stat fields in two structs

[Patryk Obara]

On 16/03/2020 07:17, Junio C Hamano wrote:
> Matheus Tavares <matheus.bernardino@usp.br> writes:
> 
>> Note: I choosed to use "st_*", as naming, for simplicity, and to keep
>> the code lines small. But should we use a more verbose "oidst_*" format,
>> instead, to avoid confusions with "struct stat"?
>> ...
>> @@ -334,8 +334,8 @@ struct dir_struct {
>>   
>>   	/* Enable untracked file cache if set */
>>   	struct untracked_cache *untracked;
>> -	struct oid_stat ss_info_exclude;
>> -	struct oid_stat ss_excludes_file;
>> +	struct oid_stat st_info_exclude;
>> +	struct oid_stat st_excludes_file;
>>   	unsigned unmanaged_exclude_files;
>>   };
> 
> I tend to agree with you that using prefix "st_" for anything other
> than "struct stat" proper would be confusing.  If "ss" used to stand
> for "sha1 stat", I would expect "oid stat" to abbreviate to "os", at
> least.

I think I stopped myself from more renames in that patch series, because
the number of touched lines across various functions started to grow too
fast and I was already stepping on brian's toes when touching oid
conversions.

> I also am wondering if we can do without any prefix, i.e. just call
> them "info_exclude" and "excludes_file", because the field names are
> private to each struct and there is no strong reason to have a
> common prefix among fields in a single struct.  Rather, it is more
> important for the fields of the same type in a single struct to have
> distinct names so that the reader can easily tell them apart and the
> reason for their use is straight-forward to understand, and the two
> names without any prefix convey their distinction pretty well, I
> would think.

The evolution of this name seems to be an artefact of refactorization
process, and not really a design decision:

info_exclude_sha1 (unsigned char[20]) changed to:
ss_info_exclude (struct sha1_stat) changed to:
ss_info_exclude (struct oid_stat)

There are some comments in dir.h still referring to info_exclude_sha1.

Therefore removing the prefix altogether (and fixing outdated comment!)
would be very welcome.

On the other hand, naming them sss_info_exclude or sos_info_exclude
would be quite funny, although I don't find hilarity to be a desirable
quality for a naming convention ;)

-- 
| ← Ceci n'est pas une pipe
Patryk Obara

Re: [PATCH 2/2] dir: improve naming of oid_stat fields in two structs

[Matheus Tavares Bernardino]

On Mon, Mar 16, 2020 at 3:17 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Matheus Tavares <matheus.bernardino@usp.br> writes:
>
> > Note: I choosed to use "st_*", as naming, for simplicity, and to keep
> > the code lines small. But should we use a more verbose "oidst_*" format,
> > instead, to avoid confusions with "struct stat"?
> > ...
> > @@ -334,8 +334,8 @@ struct dir_struct {
> >
> >       /* Enable untracked file cache if set */
> >       struct untracked_cache *untracked;
> > -     struct oid_stat ss_info_exclude;
> > -     struct oid_stat ss_excludes_file;
> > +     struct oid_stat st_info_exclude;
> > +     struct oid_stat st_excludes_file;
> >       unsigned unmanaged_exclude_files;
> >  };
>
> I tend to agree with you that using prefix "st_" for anything other
> than "struct stat" proper would be confusing.  If "ss" used to stand
> for "sha1 stat", I would expect "oid stat" to abbreviate to "os", at
> least.

Right. I also thought about changing the names to "os_*". But since OS
is so commonly used for "Operating System", I wasn't sure whether that
could also be somehow confusing.

> I also am wondering if we can do without any prefix, i.e. just call
> them "info_exclude" and "excludes_file", because the field names are
> private to each struct and there is no strong reason to have a
> common prefix among fields in a single struct.  Rather, it is more
> important for the fields of the same type in a single struct to have
> distinct names so that the reader can easily tell them apart and the
> reason for their use is straight-forward to understand, and the two
> names without any prefix convey their distinction pretty well, I
> would think.

Yes, I guess removing the prefix wouldn't make the names less
descriptive. However, especially for "ss_excludes_file", I think using
just "excludes_file" might induce the reader to think that the field
refers to a "char *" holding a path. (We also have a "excludes_file"
global variable in environment.c which is used like that). What if we
renamed them to "oidst_info_exclude" and "oidst_excludes_file", would
that be too verbose?

Re: [PATCH 2/2] dir: improve naming of oid_stat fields in two structs

[Junio C Hamano]

Matheus Tavares Bernardino <matheus.bernardino@usp.br> writes:

>> I also am wondering if we can do without any prefix, i.e. just call
>> them "info_exclude" and "excludes_file", because the field names are
>> private to each struct and there is no strong reason to have a
>> common prefix among fields in a single struct.  Rather, it is more
>> important for the fields of the same type in a single struct to have
>> distinct names so that the reader can easily tell them apart and the
>> reason for their use is straight-forward to understand, and the two
>> names without any prefix convey their distinction pretty well, I
>> would think.
>
> Yes, I guess removing the prefix wouldn't make the names less
> descriptive. However, especially for "ss_excludes_file", I think using
> just "excludes_file" might induce the reader to think that the field
> refers to a "char *" holding a path. (We also have a "excludes_file"
> global variable in environment.c which is used like that). What if we
> renamed them to "oidst_info_exclude" and "oidst_excludes_file", would
> that be too verbose?

The potential for confusion with "path to these files" is real, I
would think, so they may benefit from some prefix.

But instead of basing the prefix on their type, can we name it after
what this struct holds about the excludes file, and what the data
the struct holds is used for?  Is "oidst" something that conveys it
well to the readers of the code?

I guess the definition of "struct oid_stat" in dir.h should say what
it is for and why it is called oid_stat, or even better yet, rename
it to what is better than "this is a random bag to hold the file
stat data and an object id for an unspecified purpose".  IOW, it
would be better for the name of a structure to say what's in it, but
what the data it holds are for.

In a sense, this struct is a pared down version of cache_entry that
keeps the filesystem stat data to allow us quickly find if the path
was modified, and also lets us know if two contents are the same
without comparing bytes.  It is a mechanism for us to tell validity
of our cached data.  "struct path_validity" perhaps?  I dunno.

Re: [PATCH 2/2] dir: improve naming of oid_stat fields in two structs

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> The potential for confusion with "path to these files" is real, I
> would think, so they may benefit from some prefix.
>
> But instead of basing the prefix on their type, can we name it after
> what this struct holds about the excludes file, and what the data
> the struct holds is used for?  Is "oidst" something that conveys it
> well to the readers of the code?
> ...
> In a sense, this struct is a pared down version of cache_entry that
> keeps the filesystem stat data to allow us quickly find if the path
> was modified, and also lets us know if two contents are the same
> without comparing bytes.  It is a mechanism for us to tell validity
> of our cached data.  "struct path_validity" perhaps?  I dunno.

I think "path_validity", while it probably is much better than
"oid_stat", is a horrible name for the struct, so I'd welcome
suggestions from third-party ;-)

But I think renaming "ss_info_exclude" to "info_exclude_validity"
(or any name that talks about "info/exclude" and "validity") would
be a vast improvement, regardless of what the struct is called.

Re: [PATCH 2/2] dir: improve naming of oid_stat fields in two structs

[Jeff King]

On Mon, Mar 16, 2020 at 11:35:04AM -0700, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > The potential for confusion with "path to these files" is real, I
> > would think, so they may benefit from some prefix.
> >
> > But instead of basing the prefix on their type, can we name it after
> > what this struct holds about the excludes file, and what the data
> > the struct holds is used for?  Is "oidst" something that conveys it
> > well to the readers of the code?
> > ...
> > In a sense, this struct is a pared down version of cache_entry that
> > keeps the filesystem stat data to allow us quickly find if the path
> > was modified, and also lets us know if two contents are the same
> > without comparing bytes.  It is a mechanism for us to tell validity
> > of our cached data.  "struct path_validity" perhaps?  I dunno.
> 
> I think "path_validity", while it probably is much better than
> "oid_stat", is a horrible name for the struct, so I'd welcome
> suggestions from third-party ;-)

We also have "struct stat_validity" already, which is an even more
pared-down version of the same concept. :)

> But I think renaming "ss_info_exclude" to "info_exclude_validity"
> (or any name that talks about "info/exclude" and "validity") would
> be a vast improvement, regardless of what the struct is called.

Yeah. I think it is good to get rid of the "ss_", but it's probably not
worth spending too many more brain cycles coming up with a perfect name.
IMHO "info_exclude_validity" and "excludes_file_validity" seem quite
descriptive.

-Peff

[PATCH v2 0/3] dir: update outdated field names and comments about oid_stat

[Matheus Tavares]

In 4b33e60 ("dir: convert struct sha1_stat to use object_id",
2018-01-28), we renamed "struct sha1_stat" to "struct oid_stat".
However, there are still some comments and struct field names which
refer to the old sha1_stat. The patches in this series update those
places.

Changes since v1:
- Renamed "ss_info_exclude" and "ss_excludes_file" to
  "info_exclude_validity" and "excludes_file_validity", as suggested
  by Junio.
- Added patch 3, addressing outdated comments' updates on dir.h, as
  suggested by Patryk.

Matheus Tavares (3):
  dir: fix outdated comment on add_patterns()
  dir: improve naming of oid_stat fields in two structs
  dir: update outdated comments about untracked cache

 dir.c                                | 44 ++++++++++++++++------------
 dir.h                                | 15 +++++-----
 t/helper/test-dump-untracked-cache.c |  5 ++--
 3 files changed, 37 insertions(+), 27 deletions(-)

-- 
2.25.1

[PATCH v2 1/3] dir: fix outdated comment on add_patterns()

[Matheus Tavares]

In 4b33e60 ("dir: convert struct sha1_stat to use object_id",
2018-01-28), the struct sha1_stat was converted to oid_stat. In this
process, add_patterns() also learned to use the new struct definition.
However, the comments in this function still refers to "ss" (i.e. the
old sha1_stat). Update that.

Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
---
 dir.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/dir.c b/dir.c
index 0ffb1b3302..ab2e5b8717 100644
--- a/dir.c
+++ b/dir.c
@@ -1015,9 +1015,10 @@ static int add_patterns_from_buffer(char *buf, size_t size,
  * an index if 'istate' is non-null), parse it and store the
  * exclude rules in "pl".
  *
- * If "ss" is not NULL, compute SHA-1 of the exclude file and fill
+ * If oid_stat is not NULL, compute OID of the exclude file and fill
  * stat data from disk (only valid if add_patterns returns zero). If
- * ss_valid is non-zero, "ss" must contain good value as input.
+ * oid_stat->valid is non-zero, oid_stat must contain good value as
+ * input.
  */
 static int add_patterns(const char *fname, const char *base, int baselen,
 			struct pattern_list *pl, struct index_state *istate,
@@ -1065,7 +1066,7 @@ static int add_patterns(const char *fname, const char *base, int baselen,
 			int pos;
 			if (oid_stat->valid &&
 			    !match_stat_data_racy(istate, &oid_stat->stat, &st))
-				; /* no content change, ss->sha1 still good */
+				; /* no content change, oid_stat->oid still good */
 			else if (istate &&
 				 (pos = index_name_pos(istate, fname, strlen(fname))) >= 0 &&
 				 !ce_stage(istate->cache[pos]) &&
-- 
2.25.1

[PATCH v2 2/3] dir: improve naming of oid_stat fields in two structs

[Matheus Tavares]

Both "struct untracked_cache" and "struct dir_struct" contain fields of
the type "struct oid_stat". The latter used to be called "sha1_stat"
before 4b33e60 ("dir: convert struct sha1_stat to use object_id",
2018-01-28). Although this struct was renamed, the previously mentioned
fields are still named with an "ss_" prefix ("ss_info_exclude" and
"ss_excludes_file"), which refers to the old "sha1_stat". As this can be
confusing, let's rename these fields. Since "struct oid_stat" is used to
query the validity of cached data, a better naming alternative would be
"info_exclude_validity" and "excludes_file_validity".

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
---
 dir.c                                | 34 ++++++++++++++++------------
 dir.h                                |  8 +++----
 t/helper/test-dump-untracked-cache.c |  5 ++--
 3 files changed, 27 insertions(+), 20 deletions(-)

diff --git a/dir.c b/dir.c
index ab2e5b8717..92226b1af2 100644
--- a/dir.c
+++ b/dir.c
@@ -2613,15 +2613,17 @@ static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *d
 
 	/* Validate $GIT_DIR/info/exclude and core.excludesfile */
 	root = dir->untracked->root;
-	if (!oideq(&dir->ss_info_exclude.oid,
-		   &dir->untracked->ss_info_exclude.oid)) {
+	if (!oideq(&dir->info_exclude_validity.oid,
+		   &dir->untracked->info_exclude_validity.oid)) {
 		invalidate_gitignore(dir->untracked, root);
-		dir->untracked->ss_info_exclude = dir->ss_info_exclude;
+		dir->untracked->info_exclude_validity =
+			dir->info_exclude_validity;
 	}
-	if (!oideq(&dir->ss_excludes_file.oid,
-		   &dir->untracked->ss_excludes_file.oid)) {
+	if (!oideq(&dir->excludes_file_validity.oid,
+		   &dir->untracked->excludes_file_validity.oid)) {
 		invalidate_gitignore(dir->untracked, root);
-		dir->untracked->ss_excludes_file = dir->ss_excludes_file;
+		dir->untracked->excludes_file_validity =
+			dir->excludes_file_validity;
 	}
 
 	/* Make sure this directory is not dropped out at saving phase */
@@ -2884,14 +2886,16 @@ void setup_standard_excludes(struct dir_struct *dir)
 		excludes_file = xdg_config_home("ignore");
 	if (excludes_file && !access_or_warn(excludes_file, R_OK, 0))
 		add_patterns_from_file_1(dir, excludes_file,
-					 dir->untracked ? &dir->ss_excludes_file : NULL);
+					 dir->untracked ?
+					 &dir->excludes_file_validity : NULL);
 
 	/* per repository user preference */
 	if (startup_info->have_repository) {
 		const char *path = git_path_info_exclude();
 		if (!access_or_warn(path, R_OK, 0))
 			add_patterns_from_file_1(dir, path,
-						 dir->untracked ? &dir->ss_info_exclude : NULL);
+						 dir->untracked ?
+						 &dir->info_exclude_validity : NULL);
 	}
 }
 
@@ -3037,8 +3041,10 @@ void write_untracked_extension(struct strbuf *out, struct untracked_cache *untra
 	const unsigned hashsz = the_hash_algo->rawsz;
 
 	ouc = xcalloc(1, sizeof(*ouc));
-	stat_data_to_disk(&ouc->info_exclude_stat, &untracked->ss_info_exclude.stat);
-	stat_data_to_disk(&ouc->excludes_file_stat, &untracked->ss_excludes_file.stat);
+	stat_data_to_disk(&ouc->info_exclude_stat,
+			  &untracked->info_exclude_validity.stat);
+	stat_data_to_disk(&ouc->excludes_file_stat,
+			  &untracked->excludes_file_validity.stat);
 	ouc->dir_flags = htonl(untracked->dir_flags);
 
 	varint_len = encode_varint(untracked->ident.len, varbuf);
@@ -3046,8 +3052,8 @@ void write_untracked_extension(struct strbuf *out, struct untracked_cache *untra
 	strbuf_addbuf(out, &untracked->ident);
 
 	strbuf_add(out, ouc, sizeof(*ouc));
-	strbuf_add(out, untracked->ss_info_exclude.oid.hash, hashsz);
-	strbuf_add(out, untracked->ss_excludes_file.oid.hash, hashsz);
+	strbuf_add(out, untracked->info_exclude_validity.oid.hash, hashsz);
+	strbuf_add(out, untracked->excludes_file_validity.oid.hash, hashsz);
 	strbuf_add(out, untracked->exclude_per_dir, strlen(untracked->exclude_per_dir) + 1);
 	FREE_AND_NULL(ouc);
 
@@ -3250,10 +3256,10 @@ struct untracked_cache *read_untracked_extension(const void *data, unsigned long
 	uc = xcalloc(1, sizeof(*uc));
 	strbuf_init(&uc->ident, ident_len);
 	strbuf_add(&uc->ident, ident, ident_len);
-	load_oid_stat(&uc->ss_info_exclude,
+	load_oid_stat(&uc->info_exclude_validity,
 		      next + ouc_offset(info_exclude_stat),
 		      next + offset);
-	load_oid_stat(&uc->ss_excludes_file,
+	load_oid_stat(&uc->excludes_file_validity,
 		      next + ouc_offset(excludes_file_stat),
 		      next + offset + hashsz);
 	uc->dir_flags = get_be32(next + ouc_offset(dir_flags));
diff --git a/dir.h b/dir.h
index 5855c065a6..6c3aaeba71 100644
--- a/dir.h
+++ b/dir.h
@@ -186,8 +186,8 @@ struct untracked_cache_dir {
 };
 
 struct untracked_cache {
-	struct oid_stat ss_info_exclude;
-	struct oid_stat ss_excludes_file;
+	struct oid_stat info_exclude_validity;
+	struct oid_stat excludes_file_validity;
 	const char *exclude_per_dir;
 	struct strbuf ident;
 	/*
@@ -334,8 +334,8 @@ struct dir_struct {
 
 	/* Enable untracked file cache if set */
 	struct untracked_cache *untracked;
-	struct oid_stat ss_info_exclude;
-	struct oid_stat ss_excludes_file;
+	struct oid_stat info_exclude_validity;
+	struct oid_stat excludes_file_validity;
 	unsigned unmanaged_exclude_files;
 };
 
diff --git a/t/helper/test-dump-untracked-cache.c b/t/helper/test-dump-untracked-cache.c
index cf0f2c7228..568a4588bb 100644
--- a/t/helper/test-dump-untracked-cache.c
+++ b/t/helper/test-dump-untracked-cache.c
@@ -56,8 +56,9 @@ int cmd__dump_untracked_cache(int ac, const char **av)
 		printf("no untracked cache\n");
 		return 0;
 	}
-	printf("info/exclude %s\n", oid_to_hex(&uc->ss_info_exclude.oid));
-	printf("core.excludesfile %s\n", oid_to_hex(&uc->ss_excludes_file.oid));
+	printf("info/exclude %s\n", oid_to_hex(&uc->info_exclude_validity.oid));
+	printf("core.excludesfile %s\n",
+	       oid_to_hex(&uc->excludes_file_validity.oid));
 	printf("exclude_per_dir %s\n", uc->exclude_per_dir);
 	printf("flags %08x\n", uc->dir_flags);
 	if (uc->root)
-- 
2.25.1

[PATCH v2 3/3] dir: update outdated comments about untracked cache

[Matheus Tavares]

In dir.h we have a comment mentioning "exclude_sha1[],
info_exclude_sha1[] and excludes_file_sha1[]", but none of those names
appears in code anymore. The first appearance of "info_exclude_sha1" and
"excludes_file_sha1" in code happened in 83c094a ("untracked cache: save
to an index extension", 2015-03-08). In this patch, the two names were
added as fields of "struct ondisk_untracked_cache". Both fields were
later removed, in 3899b88 ("dir: make untracked cache extension hash
size independent", 2019-02-19). However, before their removal, we can
see in dir.c:read_untracked_extension() that their data would be copied
to the "struct untracked_cache". More specifically, to the now named
"info_exclude_validity" and "excludes_file_validity" fields of the said
struct. So it should be safe to say that what the comment referred to as
"info_exclude_sha1[] and excludes_file_sha1[]", back then, could be
updated to "info_exclude_validity.oid and excludes_file_validity.oid"
nowadays.

As for "exclude_sha1[]", its update is easier: it used to be a field of
"struct untracked_cache_dir" that was renamed in 70c369c ("dir: convert
struct untracked_cache_dir to object_id", 2018-05-02). This outdated
field name is also mentioned in one more place: a dir.c comment about
"struct write_data". So let's also update it there.

Suggested-by: Patryk Obara <dreamer.tan@gmail.com>
Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
---
 dir.c | 3 ++-
 dir.h | 7 ++++---
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/dir.c b/dir.c
index 92226b1af2..3ad2d61278 100644
--- a/dir.c
+++ b/dir.c
@@ -2961,7 +2961,8 @@ struct write_data {
 	int index;	   /* number of written untracked_cache_dir */
 	struct ewah_bitmap *check_only; /* from untracked_cache_dir */
 	struct ewah_bitmap *valid;	/* from untracked_cache_dir */
-	struct ewah_bitmap *sha1_valid; /* set if exclude_sha1 is not null */
+	/* set if untracked_cache_dir.exclude_oid is not null */
+	struct ewah_bitmap *sha1_valid;
 	struct strbuf out;
 	struct strbuf sb_stat;
 	struct strbuf sb_sha1;
diff --git a/dir.h b/dir.h
index 6c3aaeba71..549e04b068 100644
--- a/dir.h
+++ b/dir.h
@@ -166,9 +166,10 @@ struct oid_stat {
  *  Whenever a file (or a submodule) is added or removed from a
  *  directory, we invalidate that directory.
  *
- *  The remaining inputs are easy, their SHA-1 could be used to verify
- *  their contents (exclude_sha1[], info_exclude_sha1[] and
- *  excludes_file_sha1[])
+ *  The remaining inputs are easy, their OID could be used to verify
+ *  their contents (untracked_cache_dir.exclude_oid,
+ *  untracked_cache.info_exclude_validity.oid and
+ *  untracked_cache.excludes_file_validity.oid)
  */
 struct untracked_cache_dir {
 	struct untracked_cache_dir **dirs;
-- 
2.25.1