| Message ID | 6fee28469e49d501e5184162bc820350f60cc3de.1584329834.git.matheus.bernardino@usp.br (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dir: update outdated fields and comments about oid_stat | expand |
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.
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 ;)
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?
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.
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.
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
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)
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(-)