* [PATCH 0/2] dir: update outdated fields and comments about oid_stat @ 2020-03-16 3:47 Matheus Tavares 2020-03-16 3:47 ` [PATCH 1/2] dir: fix outdated comment on add_patterns() Matheus Tavares ` (2 more replies) 0 siblings, 3 replies; 13+ messages in thread From: Matheus Tavares @ 2020-03-16 3:47 UTC (permalink / raw) To: git; +Cc: gitster 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/2] dir: fix outdated comment on add_patterns() 2020-03-16 3:47 [PATCH 0/2] dir: update outdated fields and comments about oid_stat Matheus Tavares @ 2020-03-16 3:47 ` Matheus Tavares 2020-03-16 3:47 ` [PATCH 2/2] dir: improve naming of oid_stat fields in two structs Matheus Tavares 2020-03-17 18:57 ` [PATCH v2 0/3] dir: update outdated field names and comments about oid_stat Matheus Tavares 2 siblings, 0 replies; 13+ messages in thread From: Matheus Tavares @ 2020-03-16 3:47 UTC (permalink / raw) To: git Cc: gitster, Brandon Williams, Nguyễn Thái Ngọc Duy, Patryk Obara, Derrick Stolee 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/2] dir: improve naming of oid_stat fields in two structs 2020-03-16 3:47 [PATCH 0/2] dir: update outdated fields and comments about oid_stat Matheus Tavares 2020-03-16 3:47 ` [PATCH 1/2] dir: fix outdated comment on add_patterns() Matheus Tavares @ 2020-03-16 3:47 ` Matheus Tavares 2020-03-16 6:17 ` Junio C Hamano 2020-03-17 18:57 ` [PATCH v2 0/3] dir: update outdated field names and comments about oid_stat Matheus Tavares 2 siblings, 1 reply; 13+ messages in thread From: Matheus Tavares @ 2020-03-16 3:47 UTC (permalink / raw) To: git; +Cc: gitster, René Scharfe, Jeff King 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] dir: improve naming of oid_stat fields in two structs 2020-03-16 3:47 ` [PATCH 2/2] dir: improve naming of oid_stat fields in two structs Matheus Tavares @ 2020-03-16 6:17 ` Junio C Hamano 2020-03-16 12:31 ` Patryk Obara 2020-03-16 17:22 ` Matheus Tavares Bernardino 0 siblings, 2 replies; 13+ messages in thread From: Junio C Hamano @ 2020-03-16 6:17 UTC (permalink / raw) To: Matheus Tavares; +Cc: git, René Scharfe, Jeff King 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] dir: improve naming of oid_stat fields in two structs 2020-03-16 6:17 ` Junio C Hamano @ 2020-03-16 12:31 ` Patryk Obara 2020-03-16 17:22 ` Matheus Tavares Bernardino 1 sibling, 0 replies; 13+ messages in thread From: Patryk Obara @ 2020-03-16 12:31 UTC (permalink / raw) To: Junio C Hamano, Matheus Tavares; +Cc: git, René Scharfe, Jeff King 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] dir: improve naming of oid_stat fields in two structs 2020-03-16 6:17 ` Junio C Hamano 2020-03-16 12:31 ` Patryk Obara @ 2020-03-16 17:22 ` Matheus Tavares Bernardino 2020-03-16 18:31 ` Junio C Hamano 1 sibling, 1 reply; 13+ messages in thread From: Matheus Tavares Bernardino @ 2020-03-16 17:22 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, René Scharfe, Jeff King 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? ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] dir: improve naming of oid_stat fields in two structs 2020-03-16 17:22 ` Matheus Tavares Bernardino @ 2020-03-16 18:31 ` Junio C Hamano 2020-03-16 18:35 ` Junio C Hamano 0 siblings, 1 reply; 13+ messages in thread From: Junio C Hamano @ 2020-03-16 18:31 UTC (permalink / raw) To: Matheus Tavares Bernardino; +Cc: git, René Scharfe, Jeff King 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] dir: improve naming of oid_stat fields in two structs 2020-03-16 18:31 ` Junio C Hamano @ 2020-03-16 18:35 ` Junio C Hamano 2020-03-16 19:20 ` Jeff King 0 siblings, 1 reply; 13+ messages in thread From: Junio C Hamano @ 2020-03-16 18:35 UTC (permalink / raw) To: Matheus Tavares Bernardino; +Cc: git, René Scharfe, Jeff King 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] dir: improve naming of oid_stat fields in two structs 2020-03-16 18:35 ` Junio C Hamano @ 2020-03-16 19:20 ` Jeff King 0 siblings, 0 replies; 13+ messages in thread From: Jeff King @ 2020-03-16 19:20 UTC (permalink / raw) To: Junio C Hamano; +Cc: Matheus Tavares Bernardino, git, René Scharfe 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 0/3] dir: update outdated field names and comments about oid_stat 2020-03-16 3:47 [PATCH 0/2] dir: update outdated fields and comments about oid_stat Matheus Tavares 2020-03-16 3:47 ` [PATCH 1/2] dir: fix outdated comment on add_patterns() Matheus Tavares 2020-03-16 3:47 ` [PATCH 2/2] dir: improve naming of oid_stat fields in two structs Matheus Tavares @ 2020-03-17 18:57 ` Matheus Tavares 2020-03-17 18:57 ` [PATCH v2 1/3] dir: fix outdated comment on add_patterns() Matheus Tavares ` (2 more replies) 2 siblings, 3 replies; 13+ messages in thread From: Matheus Tavares @ 2020-03-17 18:57 UTC (permalink / raw) To: git; +Cc: gitster, dreamer.tan, peff 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 1/3] dir: fix outdated comment on add_patterns() 2020-03-17 18:57 ` [PATCH v2 0/3] dir: update outdated field names and comments about oid_stat Matheus Tavares @ 2020-03-17 18:57 ` Matheus Tavares 2020-03-17 18:57 ` [PATCH v2 2/3] dir: improve naming of oid_stat fields in two structs Matheus Tavares 2020-03-17 18:57 ` [PATCH v2 3/3] dir: update outdated comments about untracked cache Matheus Tavares 2 siblings, 0 replies; 13+ messages in thread From: Matheus Tavares @ 2020-03-17 18:57 UTC (permalink / raw) To: git; +Cc: gitster, dreamer.tan, peff 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v2 2/3] dir: improve naming of oid_stat fields in two structs 2020-03-17 18:57 ` [PATCH v2 0/3] dir: update outdated field names and comments about oid_stat Matheus Tavares 2020-03-17 18:57 ` [PATCH v2 1/3] dir: fix outdated comment on add_patterns() Matheus Tavares @ 2020-03-17 18:57 ` Matheus Tavares 2020-03-17 18:57 ` [PATCH v2 3/3] dir: update outdated comments about untracked cache Matheus Tavares 2 siblings, 0 replies; 13+ messages in thread From: Matheus Tavares @ 2020-03-17 18:57 UTC (permalink / raw) To: git; +Cc: gitster, dreamer.tan, peff 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v2 3/3] dir: update outdated comments about untracked cache 2020-03-17 18:57 ` [PATCH v2 0/3] dir: update outdated field names and comments about oid_stat Matheus Tavares 2020-03-17 18:57 ` [PATCH v2 1/3] dir: fix outdated comment on add_patterns() Matheus Tavares 2020-03-17 18:57 ` [PATCH v2 2/3] dir: improve naming of oid_stat fields in two structs Matheus Tavares @ 2020-03-17 18:57 ` Matheus Tavares 2 siblings, 0 replies; 13+ messages in thread From: Matheus Tavares @ 2020-03-17 18:57 UTC (permalink / raw) To: git; +Cc: gitster, dreamer.tan, peff 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
end of thread, other threads:[~2020-03-17 18:58 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-03-16 3:47 [PATCH 0/2] dir: update outdated fields and comments about oid_stat Matheus Tavares 2020-03-16 3:47 ` [PATCH 1/2] dir: fix outdated comment on add_patterns() Matheus Tavares 2020-03-16 3:47 ` [PATCH 2/2] dir: improve naming of oid_stat fields in two structs Matheus Tavares 2020-03-16 6:17 ` Junio C Hamano 2020-03-16 12:31 ` Patryk Obara 2020-03-16 17:22 ` Matheus Tavares Bernardino 2020-03-16 18:31 ` Junio C Hamano 2020-03-16 18:35 ` Junio C Hamano 2020-03-16 19:20 ` Jeff King 2020-03-17 18:57 ` [PATCH v2 0/3] dir: update outdated field names and comments about oid_stat Matheus Tavares 2020-03-17 18:57 ` [PATCH v2 1/3] dir: fix outdated comment on add_patterns() Matheus Tavares 2020-03-17 18:57 ` [PATCH v2 2/3] dir: improve naming of oid_stat fields in two structs Matheus Tavares 2020-03-17 18:57 ` [PATCH v2 3/3] dir: update outdated comments about untracked cache Matheus Tavares
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).