{"thread":{"id":"53001","subject":"[PATCH 0/2] dir: update outdated fields and comments about oid_stat","startedAt":"2020-03-16T03:47:42Z","lastAt":"2020-03-17T18:58:53Z","messageCount":13,"participants":["Matheus Tavares","Junio C Hamano","Patryk Obara","Matheus Tavares Bernardino","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"393293","messageId":"cover.1584329834.git.matheus.bernardino@usp.br","threadId":"53001","inReplyTo":null,"subject":"[PATCH 0/2] dir: update outdated fields and comments about oid_stat","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-03-16T03:47:25Z","receivedAt":"2020-03-16T03:47:42Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"In 4b33e60 (\"dir: convert struct sha1_stat to use object_id\",\n2018-01-28), we renamed \"struct sha1_stat\" to \"struct oid_stat\".\nHowever, there are still some comments and struct field names which\nrefer to the old sha1_stat. The two patches in this series update those\nplaces to avoid confusion.\n\nMatheus Tavares (2):\n dir: fix outdated comment on add_patterns()\n dir: improve naming of oid_stat fields in two structs\n\n dir.c | 35 ++++++++++++++--------------\n dir.h | 8 +++----\n t/helper/test-dump-untracked-cache.c | 4 ++--\n 3 files changed, 24 insertions(+), 23 deletions(-)\n\n-- \n2.25.0\n\n"},{"id":"393294","messageId":"9de4c8cdee441be7d9bbef2a961a8ed7d423f448.1584329834.git.matheus.bernardino@usp.br","threadId":"53001","inReplyTo":"cover.1584329834.git.matheus.bernardino@usp.br","subject":"[PATCH 1/2] dir: fix outdated comment on add_patterns()","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-03-16T03:47:26Z","receivedAt":"2020-03-16T03:48:00Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"In 4b33e60 (\"dir: convert struct sha1_stat to use object_id\",\n2018-01-28), the struct sha1_stat was converted to oid_stat. In this\nprocess, add_patterns() also learned to use the new struct definition.\nHowever, the comments in this function still refer to \"ss\" (i.e. the old\nsha1_stat). Update that.\n\nSigned-off-by: Matheus Tavares \n---\n dir.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex 0ffb1b3302..ab2e5b8717 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -1015,9 +1015,10 @@ static int add_patterns_from_buffer(char *buf, size_t size,\n * an index if 'istate' is non-null), parse it and store the\n * exclude rules in \"pl\".\n *\n- * If \"ss\" is not NULL, compute SHA-1 of the exclude file and fill\n+ * If oid_stat is not NULL, compute OID of the exclude file and fill\n * stat data from disk (only valid if add_patterns returns zero). If\n- * ss_valid is non-zero, \"ss\" must contain good value as input.\n+ * oid_stat->valid is non-zero, oid_stat must contain good value as\n+ * input.\n */\n static int add_patterns(const char *fname, const char *base, int baselen,\n \t\t\tstruct pattern_list *pl, struct index_state *istate,\n@@ -1065,7 +1066,7 @@ static int add_patterns(const char *fname, const char *base, int baselen,\n \t\t\tint pos;\n \t\t\tif (oid_stat->valid &&\n \t\t\t !match_stat_data_racy(istate, &oid_stat->stat, &st))\n-\t\t\t\t; /* no content change, ss->sha1 still good */\n+\t\t\t\t; /* no content change, oid_stat->oid still good */\n \t\t\telse if (istate &&\n \t\t\t\t (pos = index_name_pos(istate, fname, strlen(fname))) >= 0 &&\n \t\t\t\t !ce_stage(istate->cache[pos]) &&\n-- \n2.25.0\n\n"},{"id":"393295","messageId":"6fee28469e49d501e5184162bc820350f60cc3de.1584329834.git.matheus.bernardino@usp.br","threadId":"53001","inReplyTo":"cover.1584329834.git.matheus.bernardino@usp.br","subject":"[PATCH 2/2] dir: improve naming of oid_stat fields in two structs","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-03-16T03:47:27Z","receivedAt":"2020-03-16T03:48:06Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Both \"struct untracked_cache\" and \"struct dir_struct\" contain fields of\nthe type \"struct oid_stat\". The latter used to be called \"sha1_stat\"\nbefore 4b33e60 (\"dir: convert struct sha1_stat to use object_id\",\n2018-01-28). Although this struct was renamed, the previously\nmentioned fields are still named in the format \"ss_*\", which refers to\nthe old \"sha1_stat\". Since this might be confusing, rename those fields\nas \"st_*\", referring to \"stat\", instead.\n\nSigned-off-by: Matheus Tavares \n---\n\nNote: I choosed to use \"st_*\", as naming, for simplicity, and to keep\nthe code lines small. But should we use a more verbose \"oidst_*\" format,\ninstead, to avoid confusions with \"struct stat\"?\n\n dir.c | 28 ++++++++++++++--------------\n dir.h | 8 ++++----\n t/helper/test-dump-untracked-cache.c | 4 ++--\n 3 files changed, 20 insertions(+), 20 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex ab2e5b8717..e678c8184c 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -2613,15 +2613,15 @@ static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *d\n \n \t/* Validate $GIT_DIR/info/exclude and core.excludesfile */\n \troot = dir->untracked->root;\n-\tif (!oideq(&dir->ss_info_exclude.oid,\n-\t\t &dir->untracked->ss_info_exclude.oid)) {\n+\tif (!oideq(&dir->st_info_exclude.oid,\n+\t\t &dir->untracked->st_info_exclude.oid)) {\n \t\tinvalidate_gitignore(dir->untracked, root);\n-\t\tdir->untracked->ss_info_exclude = dir->ss_info_exclude;\n+\t\tdir->untracked->st_info_exclude = dir->st_info_exclude;\n \t}\n-\tif (!oideq(&dir->ss_excludes_file.oid,\n-\t\t &dir->untracked->ss_excludes_file.oid)) {\n+\tif (!oideq(&dir->st_excludes_file.oid,\n+\t\t &dir->untracked->st_excludes_file.oid)) {\n \t\tinvalidate_gitignore(dir->untracked, root);\n-\t\tdir->untracked->ss_excludes_file = dir->ss_excludes_file;\n+\t\tdir->untracked->st_excludes_file = dir->st_excludes_file;\n \t}\n \n \t/* Make sure this directory is not dropped out at saving phase */\n@@ -2884,14 +2884,14 @@ void setup_standard_excludes(struct dir_struct *dir)\n \t\texcludes_file = xdg_config_home(\"ignore\");\n \tif (excludes_file && !access_or_warn(excludes_file, R_OK, 0))\n \t\tadd_patterns_from_file_1(dir, excludes_file,\n-\t\t\t\t\t dir->untracked ? &dir->ss_excludes_file : NULL);\n+\t\t\t\t\t dir->untracked ? &dir->st_excludes_file : NULL);\n \n \t/* per repository user preference */\n \tif (startup_info->have_repository) {\n \t\tconst char *path = git_path_info_exclude();\n \t\tif (!access_or_warn(path, R_OK, 0))\n \t\t\tadd_patterns_from_file_1(dir, path,\n-\t\t\t\t\t\t dir->untracked ? &dir->ss_info_exclude : NULL);\n+\t\t\t\t\t\t dir->untracked ? &dir->st_info_exclude : NULL);\n \t}\n }\n \n@@ -3037,8 +3037,8 @@ void write_untracked_extension(struct strbuf *out, struct untracked_cache *untra\n \tconst unsigned hashsz = the_hash_algo->rawsz;\n \n \touc = xcalloc(1, sizeof(*ouc));\n-\tstat_data_to_disk(&ouc->info_exclude_stat, &untracked->ss_info_exclude.stat);\n-\tstat_data_to_disk(&ouc->excludes_file_stat, &untracked->ss_excludes_file.stat);\n+\tstat_data_to_disk(&ouc->info_exclude_stat, &untracked->st_info_exclude.stat);\n+\tstat_data_to_disk(&ouc->excludes_file_stat, &untracked->st_excludes_file.stat);\n \touc->dir_flags = htonl(untracked->dir_flags);\n \n \tvarint_len = encode_varint(untracked->ident.len, varbuf);\n@@ -3046,8 +3046,8 @@ void write_untracked_extension(struct strbuf *out, struct untracked_cache *untra\n \tstrbuf_addbuf(out, &untracked->ident);\n \n \tstrbuf_add(out, ouc, sizeof(*ouc));\n-\tstrbuf_add(out, untracked->ss_info_exclude.oid.hash, hashsz);\n-\tstrbuf_add(out, untracked->ss_excludes_file.oid.hash, hashsz);\n+\tstrbuf_add(out, untracked->st_info_exclude.oid.hash, hashsz);\n+\tstrbuf_add(out, untracked->st_excludes_file.oid.hash, hashsz);\n \tstrbuf_add(out, untracked->exclude_per_dir, strlen(untracked->exclude_per_dir) + 1);\n \tFREE_AND_NULL(ouc);\n \n@@ -3250,10 +3250,10 @@ struct untracked_cache *read_untracked_extension(const void *data, unsigned long\n \tuc = xcalloc(1, sizeof(*uc));\n \tstrbuf_init(&uc->ident, ident_len);\n \tstrbuf_add(&uc->ident, ident, ident_len);\n-\tload_oid_stat(&uc->ss_info_exclude,\n+\tload_oid_stat(&uc->st_info_exclude,\n \t\t next + ouc_offset(info_exclude_stat),\n \t\t next + offset);\n-\tload_oid_stat(&uc->ss_excludes_file,\n+\tload_oid_stat(&uc->st_excludes_file,\n \t\t next + ouc_offset(excludes_file_stat),\n \t\t next + offset + hashsz);\n \tuc->dir_flags = get_be32(next + ouc_offset(dir_flags));\ndiff --git a/dir.h b/dir.h\nindex 5855c065a6..4d30816271 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -186,8 +186,8 @@ struct untracked_cache_dir {\n };\n \n struct untracked_cache {\n-\tstruct oid_stat ss_info_exclude;\n-\tstruct oid_stat ss_excludes_file;\n+\tstruct oid_stat st_info_exclude;\n+\tstruct oid_stat st_excludes_file;\n \tconst char *exclude_per_dir;\n \tstruct strbuf ident;\n \t/*\n@@ -334,8 +334,8 @@ struct dir_struct {\n \n \t/* Enable untracked file cache if set */\n \tstruct untracked_cache *untracked;\n-\tstruct oid_stat ss_info_exclude;\n-\tstruct oid_stat ss_excludes_file;\n+\tstruct oid_stat st_info_exclude;\n+\tstruct oid_stat st_excludes_file;\n \tunsigned unmanaged_exclude_files;\n };\n \ndiff --git a/t/helper/test-dump-untracked-cache.c b/t/helper/test-dump-untracked-cache.c\nindex cf0f2c7228..71a47e974b 100644\n--- a/t/helper/test-dump-untracked-cache.c\n+++ b/t/helper/test-dump-untracked-cache.c\n@@ -56,8 +56,8 @@ int cmd__dump_untracked_cache(int ac, const char **av)\n \t\tprintf(\"no untracked cache\\n\");\n \t\treturn 0;\n \t}\n-\tprintf(\"info/exclude %s\\n\", oid_to_hex(&uc->ss_info_exclude.oid));\n-\tprintf(\"core.excludesfile %s\\n\", oid_to_hex(&uc->ss_excludes_file.oid));\n+\tprintf(\"info/exclude %s\\n\", oid_to_hex(&uc->st_info_exclude.oid));\n+\tprintf(\"core.excludesfile %s\\n\", oid_to_hex(&uc->st_excludes_file.oid));\n \tprintf(\"exclude_per_dir %s\\n\", uc->exclude_per_dir);\n \tprintf(\"flags %08x\\n\", uc->dir_flags);\n \tif (uc->root)\n-- \n2.25.0\n\n"},{"id":"393296","messageId":"xmqq1rpsako1.fsf@gitster.c.googlers.com","threadId":"53001","inReplyTo":"6fee28469e49d501e5184162bc820350f60cc3de.1584329834.git.matheus.bernardino@usp.br","subject":"Re: [PATCH 2/2] dir: improve naming of oid_stat fields in two structs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-16T06:17:02Z","receivedAt":"2020-03-16T06:17:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matheus Tavares writes:\n\n> Note: I choosed to use \"st_*\", as naming, for simplicity, and to keep\n> the code lines small. But should we use a more verbose \"oidst_*\" format,\n> instead, to avoid confusions with \"struct stat\"?\n> ...\n> @@ -334,8 +334,8 @@ struct dir_struct {\n> \n> \t/* Enable untracked file cache if set */\n> \tstruct untracked_cache *untracked;\n> -\tstruct oid_stat ss_info_exclude;\n> -\tstruct oid_stat ss_excludes_file;\n> +\tstruct oid_stat st_info_exclude;\n> +\tstruct oid_stat st_excludes_file;\n> \tunsigned unmanaged_exclude_files;\n> };\n\nI tend to agree with you that using prefix \"st_\" for anything other\nthan \"struct stat\" proper would be confusing. If \"ss\" used to stand\nfor \"sha1 stat\", I would expect \"oid stat\" to abbreviate to \"os\", at\nleast.\n\nI also am wondering if we can do without any prefix, i.e. just call\nthem \"info_exclude\" and \"excludes_file\", because the field names are\nprivate to each struct and there is no strong reason to have a\ncommon prefix among fields in a single struct. Rather, it is more\nimportant for the fields of the same type in a single struct to have\ndistinct names so that the reader can easily tell them apart and the\nreason for their use is straight-forward to understand, and the two\nnames without any prefix convey their distinction pretty well, I\nwould think.\n\nIt is not like we adopt a convention to name our variables with\nabbreviated typenames embedded in the variable names. We shouldn't\nbe doing that for field names, either, but it smells that the \"give\nthem prefix ss_ because they are of type sha1_stat\" pretty much has\norigin in such school.\n\nThanks.\n"},{"id":"393299","messageId":"938146aa-e095-66fb-da0b-10e8796eaa12@gmail.com","threadId":"53001","inReplyTo":"xmqq1rpsako1.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 2/2] dir: improve naming of oid_stat fields in two structs","fromName":"Patryk Obara","fromEmail":"dreamer.tan@gmail.com","sentAt":"2020-03-16T12:31:20Z","receivedAt":"2020-03-16T12:31:29Z","isPatch":true,"sender":{"key":"dreamer.tan@gmail.com","avatar":"https://gravatar.com/avatar/02a74855b64d0dcc954a6d40779626ceccb682459daccef9f688d1f67b230ea4?d=mp&s=160"},"body":"On 16/03/2020 07:17, Junio C Hamano wrote:\n> Matheus Tavares writes:\n> \n>> Note: I choosed to use \"st_*\", as naming, for simplicity, and to keep\n>> the code lines small. But should we use a more verbose \"oidst_*\" format,\n>> instead, to avoid confusions with \"struct stat\"?\n>> ...\n>> @@ -334,8 +334,8 @@ struct dir_struct {\n>> \n>> \t/* Enable untracked file cache if set */\n>> \tstruct untracked_cache *untracked;\n>> -\tstruct oid_stat ss_info_exclude;\n>> -\tstruct oid_stat ss_excludes_file;\n>> +\tstruct oid_stat st_info_exclude;\n>> +\tstruct oid_stat st_excludes_file;\n>> \tunsigned unmanaged_exclude_files;\n>> };\n> \n> I tend to agree with you that using prefix \"st_\" for anything other\n> than \"struct stat\" proper would be confusing. If \"ss\" used to stand\n> for \"sha1 stat\", I would expect \"oid stat\" to abbreviate to \"os\", at\n> least.\n\nI think I stopped myself from more renames in that patch series, because\nthe number of touched lines across various functions started to grow too\nfast and I was already stepping on brian's toes when touching oid\nconversions.\n\n> I also am wondering if we can do without any prefix, i.e. just call\n> them \"info_exclude\" and \"excludes_file\", because the field names are\n> private to each struct and there is no strong reason to have a\n> common prefix among fields in a single struct. Rather, it is more\n> important for the fields of the same type in a single struct to have\n> distinct names so that the reader can easily tell them apart and the\n> reason for their use is straight-forward to understand, and the two\n> names without any prefix convey their distinction pretty well, I\n> would think.\n\nThe evolution of this name seems to be an artefact of refactorization\nprocess, and not really a design decision:\n\ninfo_exclude_sha1 (unsigned char[20]) changed to:\nss_info_exclude (struct sha1_stat) changed to:\nss_info_exclude (struct oid_stat)\n\nThere are some comments in dir.h still referring to info_exclude_sha1.\n\nTherefore removing the prefix altogether (and fixing outdated comment!)\nwould be very welcome.\n\nOn the other hand, naming them sss_info_exclude or sos_info_exclude\nwould be quite funny, although I don't find hilarity to be a desirable\nquality for a naming convention ;)\n\n-- \n| ← Ceci n'est pas une pipe\nPatryk Obara\n"},{"id":"393309","messageId":"CAHd-oW4OpoW0Qbp6_HfFiXRXasLXVnw3hDkzDG3S4WR3yH9Wuw@mail.gmail.com","threadId":"53001","inReplyTo":"xmqq1rpsako1.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 2/2] dir: improve naming of oid_stat fields in two structs","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-03-16T17:22:53Z","receivedAt":"2020-03-16T17:23:12Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Mon, Mar 16, 2020 at 3:17 AM Junio C Hamano wrote:\n>\n> Matheus Tavares writes:\n>\n> > Note: I choosed to use \"st_*\", as naming, for simplicity, and to keep\n> > the code lines small. But should we use a more verbose \"oidst_*\" format,\n> > instead, to avoid confusions with \"struct stat\"?\n> > ...\n> > @@ -334,8 +334,8 @@ struct dir_struct {\n> >\n> > /* Enable untracked file cache if set */\n> > struct untracked_cache *untracked;\n> > - struct oid_stat ss_info_exclude;\n> > - struct oid_stat ss_excludes_file;\n> > + struct oid_stat st_info_exclude;\n> > + struct oid_stat st_excludes_file;\n> > unsigned unmanaged_exclude_files;\n> > };\n>\n> I tend to agree with you that using prefix \"st_\" for anything other\n> than \"struct stat\" proper would be confusing. If \"ss\" used to stand\n> for \"sha1 stat\", I would expect \"oid stat\" to abbreviate to \"os\", at\n> least.\n\nRight. I also thought about changing the names to \"os_*\". But since OS\nis so commonly used for \"Operating System\", I wasn't sure whether that\ncould also be somehow confusing.\n\n> I also am wondering if we can do without any prefix, i.e. just call\n> them \"info_exclude\" and \"excludes_file\", because the field names are\n> private to each struct and there is no strong reason to have a\n> common prefix among fields in a single struct. Rather, it is more\n> important for the fields of the same type in a single struct to have\n> distinct names so that the reader can easily tell them apart and the\n> reason for their use is straight-forward to understand, and the two\n> names without any prefix convey their distinction pretty well, I\n> would think.\n\nYes, I guess removing the prefix wouldn't make the names less\ndescriptive. However, especially for \"ss_excludes_file\", I think using\njust \"excludes_file\" might induce the reader to think that the field\nrefers to a \"char *\" holding a path. (We also have a \"excludes_file\"\nglobal variable in environment.c which is used like that). What if we\nrenamed them to \"oidst_info_exclude\" and \"oidst_excludes_file\", would\nthat be too verbose?\n"},{"id":"393320","messageId":"xmqqsgi8882t.fsf@gitster.c.googlers.com","threadId":"53001","inReplyTo":"CAHd-oW4OpoW0Qbp6_HfFiXRXasLXVnw3hDkzDG3S4WR3yH9Wuw@mail.gmail.com","subject":"Re: [PATCH 2/2] dir: improve naming of oid_stat fields in two structs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-16T18:31:54Z","receivedAt":"2020-03-16T18:32:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matheus Tavares Bernardino writes:\n\n>> I also am wondering if we can do without any prefix, i.e. just call\n>> them \"info_exclude\" and \"excludes_file\", because the field names are\n>> private to each struct and there is no strong reason to have a\n>> common prefix among fields in a single struct. Rather, it is more\n>> important for the fields of the same type in a single struct to have\n>> distinct names so that the reader can easily tell them apart and the\n>> reason for their use is straight-forward to understand, and the two\n>> names without any prefix convey their distinction pretty well, I\n>> would think.\n>\n> Yes, I guess removing the prefix wouldn't make the names less\n> descriptive. However, especially for \"ss_excludes_file\", I think using\n> just \"excludes_file\" might induce the reader to think that the field\n> refers to a \"char *\" holding a path. (We also have a \"excludes_file\"\n> global variable in environment.c which is used like that). What if we\n> renamed them to \"oidst_info_exclude\" and \"oidst_excludes_file\", would\n> that be too verbose?\n\nThe potential for confusion with \"path to these files\" is real, I\nwould think, so they may benefit from some prefix.\n\nBut instead of basing the prefix on their type, can we name it after\nwhat this struct holds about the excludes file, and what the data\nthe struct holds is used for? Is \"oidst\" something that conveys it\nwell to the readers of the code?\n\nI guess the definition of \"struct oid_stat\" in dir.h should say what\nit is for and why it is called oid_stat, or even better yet, rename\nit to what is better than \"this is a random bag to hold the file\nstat data and an object id for an unspecified purpose\". IOW, it\nwould be better for the name of a structure to say what's in it, but\nwhat the data it holds are for.\n\nIn a sense, this struct is a pared down version of cache_entry that\nkeeps the filesystem stat data to allow us quickly find if the path\nwas modified, and also lets us know if two contents are the same\nwithout comparing bytes. It is a mechanism for us to tell validity\nof our cached data. \"struct path_validity\" perhaps? I dunno.\n"},{"id":"393322","messageId":"xmqqo8sw87xj.fsf@gitster.c.googlers.com","threadId":"53001","inReplyTo":"xmqqsgi8882t.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 2/2] dir: improve naming of oid_stat fields in two structs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-16T18:35:04Z","receivedAt":"2020-03-16T18:35:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> The potential for confusion with \"path to these files\" is real, I\n> would think, so they may benefit from some prefix.\n>\n> But instead of basing the prefix on their type, can we name it after\n> what this struct holds about the excludes file, and what the data\n> the struct holds is used for? Is \"oidst\" something that conveys it\n> well to the readers of the code?\n> ...\n> In a sense, this struct is a pared down version of cache_entry that\n> keeps the filesystem stat data to allow us quickly find if the path\n> was modified, and also lets us know if two contents are the same\n> without comparing bytes. It is a mechanism for us to tell validity\n> of our cached data. \"struct path_validity\" perhaps? I dunno.\n\nI think \"path_validity\", while it probably is much better than\n\"oid_stat\", is a horrible name for the struct, so I'd welcome\nsuggestions from third-party ;-)\n\nBut I think renaming \"ss_info_exclude\" to \"info_exclude_validity\"\n(or any name that talks about \"info/exclude\" and \"validity\") would\nbe a vast improvement, regardless of what the struct is called.\n"},{"id":"393324","messageId":"20200316192000.GB1073710@coredump.intra.peff.net","threadId":"53001","inReplyTo":"xmqqo8sw87xj.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 2/2] dir: improve naming of oid_stat fields in two structs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-03-16T19:20:00Z","receivedAt":"2020-03-16T19:20:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 16, 2020 at 11:35:04AM -0700, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n> \n> > The potential for confusion with \"path to these files\" is real, I\n> > would think, so they may benefit from some prefix.\n> >\n> > But instead of basing the prefix on their type, can we name it after\n> > what this struct holds about the excludes file, and what the data\n> > the struct holds is used for? Is \"oidst\" something that conveys it\n> > well to the readers of the code?\n> > ...\n> > In a sense, this struct is a pared down version of cache_entry that\n> > keeps the filesystem stat data to allow us quickly find if the path\n> > was modified, and also lets us know if two contents are the same\n> > without comparing bytes. It is a mechanism for us to tell validity\n> > of our cached data. \"struct path_validity\" perhaps? I dunno.\n> \n> I think \"path_validity\", while it probably is much better than\n> \"oid_stat\", is a horrible name for the struct, so I'd welcome\n> suggestions from third-party ;-)\n\nWe also have \"struct stat_validity\" already, which is an even more\npared-down version of the same concept. :)\n\n> But I think renaming \"ss_info_exclude\" to \"info_exclude_validity\"\n> (or any name that talks about \"info/exclude\" and \"validity\") would\n> be a vast improvement, regardless of what the struct is called.\n\nYeah. I think it is good to get rid of the \"ss_\", but it's probably not\nworth spending too many more brain cycles coming up with a perfect name.\nIMHO \"info_exclude_validity\" and \"excludes_file_validity\" seem quite\ndescriptive.\n\n-Peff\n"},{"id":"393368","messageId":"cover.1584471137.git.matheus.bernardino@usp.br","threadId":"53001","inReplyTo":"cover.1584329834.git.matheus.bernardino@usp.br","subject":"[PATCH v2 0/3] dir: update outdated field names and comments about oid_stat","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-03-17T18:57:42Z","receivedAt":"2020-03-17T18:58:30Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"In 4b33e60 (\"dir: convert struct sha1_stat to use object_id\",\n2018-01-28), we renamed \"struct sha1_stat\" to \"struct oid_stat\".\nHowever, there are still some comments and struct field names which\nrefer to the old sha1_stat. The patches in this series update those\nplaces.\n\nChanges since v1:\n- Renamed \"ss_info_exclude\" and \"ss_excludes_file\" to\n \"info_exclude_validity\" and \"excludes_file_validity\", as suggested\n by Junio.\n- Added patch 3, addressing outdated comments' updates on dir.h, as\n suggested by Patryk.\n\nMatheus Tavares (3):\n dir: fix outdated comment on add_patterns()\n dir: improve naming of oid_stat fields in two structs\n dir: update outdated comments about untracked cache\n\n dir.c | 44 ++++++++++++++++------------\n dir.h | 15 +++++-----\n t/helper/test-dump-untracked-cache.c | 5 ++--\n 3 files changed, 37 insertions(+), 27 deletions(-)\n\n-- \n2.25.1\n\n"},{"id":"393369","messageId":"9de4c8cdee441be7d9bbef2a961a8ed7d423f448.1584471137.git.matheus.bernardino@usp.br","threadId":"53001","inReplyTo":"cover.1584471137.git.matheus.bernardino@usp.br","subject":"[PATCH v2 1/3] dir: fix outdated comment on add_patterns()","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-03-17T18:57:43Z","receivedAt":"2020-03-17T18:58:32Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"In 4b33e60 (\"dir: convert struct sha1_stat to use object_id\",\n2018-01-28), the struct sha1_stat was converted to oid_stat. In this\nprocess, add_patterns() also learned to use the new struct definition.\nHowever, the comments in this function still refers to \"ss\" (i.e. the\nold sha1_stat). Update that.\n\nSigned-off-by: Matheus Tavares \n---\n dir.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex 0ffb1b3302..ab2e5b8717 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -1015,9 +1015,10 @@ static int add_patterns_from_buffer(char *buf, size_t size,\n * an index if 'istate' is non-null), parse it and store the\n * exclude rules in \"pl\".\n *\n- * If \"ss\" is not NULL, compute SHA-1 of the exclude file and fill\n+ * If oid_stat is not NULL, compute OID of the exclude file and fill\n * stat data from disk (only valid if add_patterns returns zero). If\n- * ss_valid is non-zero, \"ss\" must contain good value as input.\n+ * oid_stat->valid is non-zero, oid_stat must contain good value as\n+ * input.\n */\n static int add_patterns(const char *fname, const char *base, int baselen,\n \t\t\tstruct pattern_list *pl, struct index_state *istate,\n@@ -1065,7 +1066,7 @@ static int add_patterns(const char *fname, const char *base, int baselen,\n \t\t\tint pos;\n \t\t\tif (oid_stat->valid &&\n \t\t\t !match_stat_data_racy(istate, &oid_stat->stat, &st))\n-\t\t\t\t; /* no content change, ss->sha1 still good */\n+\t\t\t\t; /* no content change, oid_stat->oid still good */\n \t\t\telse if (istate &&\n \t\t\t\t (pos = index_name_pos(istate, fname, strlen(fname))) >= 0 &&\n \t\t\t\t !ce_stage(istate->cache[pos]) &&\n-- \n2.25.1\n\n"},{"id":"393370","messageId":"776db73a8c2bd95f787d6398c41bc69c063be858.1584471137.git.matheus.bernardino@usp.br","threadId":"53001","inReplyTo":"cover.1584471137.git.matheus.bernardino@usp.br","subject":"[PATCH v2 2/3] dir: improve naming of oid_stat fields in two structs","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-03-17T18:57:44Z","receivedAt":"2020-03-17T18:58:50Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Both \"struct untracked_cache\" and \"struct dir_struct\" contain fields of\nthe type \"struct oid_stat\". The latter used to be called \"sha1_stat\"\nbefore 4b33e60 (\"dir: convert struct sha1_stat to use object_id\",\n2018-01-28). Although this struct was renamed, the previously mentioned\nfields are still named with an \"ss_\" prefix (\"ss_info_exclude\" and\n\"ss_excludes_file\"), which refers to the old \"sha1_stat\". As this can be\nconfusing, let's rename these fields. Since \"struct oid_stat\" is used to\nquery the validity of cached data, a better naming alternative would be\n\"info_exclude_validity\" and \"excludes_file_validity\".\n\nHelped-by: Junio C Hamano \nSigned-off-by: Matheus Tavares \n---\n dir.c | 34 ++++++++++++++++------------\n dir.h | 8 +++----\n t/helper/test-dump-untracked-cache.c | 5 ++--\n 3 files changed, 27 insertions(+), 20 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex ab2e5b8717..92226b1af2 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -2613,15 +2613,17 @@ static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *d\n \n \t/* Validate $GIT_DIR/info/exclude and core.excludesfile */\n \troot = dir->untracked->root;\n-\tif (!oideq(&dir->ss_info_exclude.oid,\n-\t\t &dir->untracked->ss_info_exclude.oid)) {\n+\tif (!oideq(&dir->info_exclude_validity.oid,\n+\t\t &dir->untracked->info_exclude_validity.oid)) {\n \t\tinvalidate_gitignore(dir->untracked, root);\n-\t\tdir->untracked->ss_info_exclude = dir->ss_info_exclude;\n+\t\tdir->untracked->info_exclude_validity =\n+\t\t\tdir->info_exclude_validity;\n \t}\n-\tif (!oideq(&dir->ss_excludes_file.oid,\n-\t\t &dir->untracked->ss_excludes_file.oid)) {\n+\tif (!oideq(&dir->excludes_file_validity.oid,\n+\t\t &dir->untracked->excludes_file_validity.oid)) {\n \t\tinvalidate_gitignore(dir->untracked, root);\n-\t\tdir->untracked->ss_excludes_file = dir->ss_excludes_file;\n+\t\tdir->untracked->excludes_file_validity =\n+\t\t\tdir->excludes_file_validity;\n \t}\n \n \t/* Make sure this directory is not dropped out at saving phase */\n@@ -2884,14 +2886,16 @@ void setup_standard_excludes(struct dir_struct *dir)\n \t\texcludes_file = xdg_config_home(\"ignore\");\n \tif (excludes_file && !access_or_warn(excludes_file, R_OK, 0))\n \t\tadd_patterns_from_file_1(dir, excludes_file,\n-\t\t\t\t\t dir->untracked ? &dir->ss_excludes_file : NULL);\n+\t\t\t\t\t dir->untracked ?\n+\t\t\t\t\t &dir->excludes_file_validity : NULL);\n \n \t/* per repository user preference */\n \tif (startup_info->have_repository) {\n \t\tconst char *path = git_path_info_exclude();\n \t\tif (!access_or_warn(path, R_OK, 0))\n \t\t\tadd_patterns_from_file_1(dir, path,\n-\t\t\t\t\t\t dir->untracked ? &dir->ss_info_exclude : NULL);\n+\t\t\t\t\t\t dir->untracked ?\n+\t\t\t\t\t\t &dir->info_exclude_validity : NULL);\n \t}\n }\n \n@@ -3037,8 +3041,10 @@ void write_untracked_extension(struct strbuf *out, struct untracked_cache *untra\n \tconst unsigned hashsz = the_hash_algo->rawsz;\n \n \touc = xcalloc(1, sizeof(*ouc));\n-\tstat_data_to_disk(&ouc->info_exclude_stat, &untracked->ss_info_exclude.stat);\n-\tstat_data_to_disk(&ouc->excludes_file_stat, &untracked->ss_excludes_file.stat);\n+\tstat_data_to_disk(&ouc->info_exclude_stat,\n+\t\t\t &untracked->info_exclude_validity.stat);\n+\tstat_data_to_disk(&ouc->excludes_file_stat,\n+\t\t\t &untracked->excludes_file_validity.stat);\n \touc->dir_flags = htonl(untracked->dir_flags);\n \n \tvarint_len = encode_varint(untracked->ident.len, varbuf);\n@@ -3046,8 +3052,8 @@ void write_untracked_extension(struct strbuf *out, struct untracked_cache *untra\n \tstrbuf_addbuf(out, &untracked->ident);\n \n \tstrbuf_add(out, ouc, sizeof(*ouc));\n-\tstrbuf_add(out, untracked->ss_info_exclude.oid.hash, hashsz);\n-\tstrbuf_add(out, untracked->ss_excludes_file.oid.hash, hashsz);\n+\tstrbuf_add(out, untracked->info_exclude_validity.oid.hash, hashsz);\n+\tstrbuf_add(out, untracked->excludes_file_validity.oid.hash, hashsz);\n \tstrbuf_add(out, untracked->exclude_per_dir, strlen(untracked->exclude_per_dir) + 1);\n \tFREE_AND_NULL(ouc);\n \n@@ -3250,10 +3256,10 @@ struct untracked_cache *read_untracked_extension(const void *data, unsigned long\n \tuc = xcalloc(1, sizeof(*uc));\n \tstrbuf_init(&uc->ident, ident_len);\n \tstrbuf_add(&uc->ident, ident, ident_len);\n-\tload_oid_stat(&uc->ss_info_exclude,\n+\tload_oid_stat(&uc->info_exclude_validity,\n \t\t next + ouc_offset(info_exclude_stat),\n \t\t next + offset);\n-\tload_oid_stat(&uc->ss_excludes_file,\n+\tload_oid_stat(&uc->excludes_file_validity,\n \t\t next + ouc_offset(excludes_file_stat),\n \t\t next + offset + hashsz);\n \tuc->dir_flags = get_be32(next + ouc_offset(dir_flags));\ndiff --git a/dir.h b/dir.h\nindex 5855c065a6..6c3aaeba71 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -186,8 +186,8 @@ struct untracked_cache_dir {\n };\n \n struct untracked_cache {\n-\tstruct oid_stat ss_info_exclude;\n-\tstruct oid_stat ss_excludes_file;\n+\tstruct oid_stat info_exclude_validity;\n+\tstruct oid_stat excludes_file_validity;\n \tconst char *exclude_per_dir;\n \tstruct strbuf ident;\n \t/*\n@@ -334,8 +334,8 @@ struct dir_struct {\n \n \t/* Enable untracked file cache if set */\n \tstruct untracked_cache *untracked;\n-\tstruct oid_stat ss_info_exclude;\n-\tstruct oid_stat ss_excludes_file;\n+\tstruct oid_stat info_exclude_validity;\n+\tstruct oid_stat excludes_file_validity;\n \tunsigned unmanaged_exclude_files;\n };\n \ndiff --git a/t/helper/test-dump-untracked-cache.c b/t/helper/test-dump-untracked-cache.c\nindex cf0f2c7228..568a4588bb 100644\n--- a/t/helper/test-dump-untracked-cache.c\n+++ b/t/helper/test-dump-untracked-cache.c\n@@ -56,8 +56,9 @@ int cmd__dump_untracked_cache(int ac, const char **av)\n \t\tprintf(\"no untracked cache\\n\");\n \t\treturn 0;\n \t}\n-\tprintf(\"info/exclude %s\\n\", oid_to_hex(&uc->ss_info_exclude.oid));\n-\tprintf(\"core.excludesfile %s\\n\", oid_to_hex(&uc->ss_excludes_file.oid));\n+\tprintf(\"info/exclude %s\\n\", oid_to_hex(&uc->info_exclude_validity.oid));\n+\tprintf(\"core.excludesfile %s\\n\",\n+\t oid_to_hex(&uc->excludes_file_validity.oid));\n \tprintf(\"exclude_per_dir %s\\n\", uc->exclude_per_dir);\n \tprintf(\"flags %08x\\n\", uc->dir_flags);\n \tif (uc->root)\n-- \n2.25.1\n\n"},{"id":"393371","messageId":"544d625c471fd5d2d0e139dd0d48cf85d87f0cf8.1584471137.git.matheus.bernardino@usp.br","threadId":"53001","inReplyTo":"cover.1584471137.git.matheus.bernardino@usp.br","subject":"[PATCH v2 3/3] dir: update outdated comments about untracked cache","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2020-03-17T18:57:45Z","receivedAt":"2020-03-17T18:58:53Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"In dir.h we have a comment mentioning \"exclude_sha1[],\ninfo_exclude_sha1[] and excludes_file_sha1[]\", but none of those names\nappears in code anymore. The first appearance of \"info_exclude_sha1\" and\n\"excludes_file_sha1\" in code happened in 83c094a (\"untracked cache: save\nto an index extension\", 2015-03-08). In this patch, the two names were\nadded as fields of \"struct ondisk_untracked_cache\". Both fields were\nlater removed, in 3899b88 (\"dir: make untracked cache extension hash\nsize independent\", 2019-02-19). However, before their removal, we can\nsee in dir.c:read_untracked_extension() that their data would be copied\nto the \"struct untracked_cache\". More specifically, to the now named\n\"info_exclude_validity\" and \"excludes_file_validity\" fields of the said\nstruct. So it should be safe to say that what the comment referred to as\n\"info_exclude_sha1[] and excludes_file_sha1[]\", back then, could be\nupdated to \"info_exclude_validity.oid and excludes_file_validity.oid\"\nnowadays.\n\nAs for \"exclude_sha1[]\", its update is easier: it used to be a field of\n\"struct untracked_cache_dir\" that was renamed in 70c369c (\"dir: convert\nstruct untracked_cache_dir to object_id\", 2018-05-02). This outdated\nfield name is also mentioned in one more place: a dir.c comment about\n\"struct write_data\". So let's also update it there.\n\nSuggested-by: Patryk Obara \nSigned-off-by: Matheus Tavares \n---\n dir.c | 3 ++-\n dir.h | 7 ++++---\n 2 files changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex 92226b1af2..3ad2d61278 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -2961,7 +2961,8 @@ struct write_data {\n \tint index;\t /* number of written untracked_cache_dir */\n \tstruct ewah_bitmap *check_only; /* from untracked_cache_dir */\n \tstruct ewah_bitmap *valid;\t/* from untracked_cache_dir */\n-\tstruct ewah_bitmap *sha1_valid; /* set if exclude_sha1 is not null */\n+\t/* set if untracked_cache_dir.exclude_oid is not null */\n+\tstruct ewah_bitmap *sha1_valid;\n \tstruct strbuf out;\n \tstruct strbuf sb_stat;\n \tstruct strbuf sb_sha1;\ndiff --git a/dir.h b/dir.h\nindex 6c3aaeba71..549e04b068 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -166,9 +166,10 @@ struct oid_stat {\n * Whenever a file (or a submodule) is added or removed from a\n * directory, we invalidate that directory.\n *\n- * The remaining inputs are easy, their SHA-1 could be used to verify\n- * their contents (exclude_sha1[], info_exclude_sha1[] and\n- * excludes_file_sha1[])\n+ * The remaining inputs are easy, their OID could be used to verify\n+ * their contents (untracked_cache_dir.exclude_oid,\n+ * untracked_cache.info_exclude_validity.oid and\n+ * untracked_cache.excludes_file_validity.oid)\n */\n struct untracked_cache_dir {\n \tstruct untracked_cache_dir **dirs;\n-- \n2.25.1\n\n"}]}