{"thread":{"id":"54427","subject":"[PATCH] dir.c: fix comments to agree with argument name","startedAt":"2020-10-15T12:49:26Z","lastAt":"2020-10-16T12:40:09Z","messageCount":8,"participants":["Nipunn Koorapati via GitGitGadget","Jeff King","Junio C Hamano","Nipunn Koorapati"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"407598","messageId":"pull.757.git.1602766160815.gitgitgadget@gmail.com","threadId":"54427","inReplyTo":null,"subject":"[PATCH] dir.c: fix comments to agree with argument name","fromName":"Nipunn Koorapati via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-15T12:49:20Z","receivedAt":"2020-10-15T12:49:26Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"From: Alex Vandiver \n\nSigned-off-by: Alex Vandiver \nSigned-off-by: Nipunn Koorapati \n---\n dir.c: Fix comments to agree with argument name\n \n Comments are out of date with the variable names.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-757%2Fnipunn1313%2Fcomments-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-757/nipunn1313/comments-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/757\n\n dir.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex 78387110e6..4c79c4f0e1 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -1040,9 +1040,9 @@ 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 SHA-1 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 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@@ -1090,7 +1090,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\nbase-commit: d4a392452e292ff924e79ec8458611c0f679d6d4\n-- \ngitgitgadget\n"},{"id":"407615","messageId":"20201015160725.GA1104947@coredump.intra.peff.net","threadId":"54427","inReplyTo":"pull.757.git.1602766160815.gitgitgadget@gmail.com","subject":"Re: [PATCH] dir.c: fix comments to agree with argument name","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-15T16:07:25Z","receivedAt":"2020-10-15T16:07:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 15, 2020 at 12:49:20PM +0000, Nipunn Koorapati via GitGitGadget wrote:\n\n> diff --git a/dir.c b/dir.c\n> index 78387110e6..4c79c4f0e1 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -1040,9 +1040,9 @@ 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 SHA-1 of the exclude file and fill\n\nMakes sense. This changed as part of 4b33e60201 (dir: convert struct\nsha1_stat to use object_id, 2018-01-28). Perhaps it would likewise make\nsense to stop saying \"SHA-1\" here, and just say \"hash\" (or even \"object\nid\", though TBH I think the fact that the hash is the same as an\nobject-id is largely an implementation detail).\n\n-Peff\n"},{"id":"407624","messageId":"pull.757.v2.git.1602779316760.gitgitgadget@gmail.com","threadId":"54427","inReplyTo":"pull.757.git.1602766160815.gitgitgadget@gmail.com","subject":"[PATCH v2] dir.c: fix comments to agree with argument name","fromName":"Nipunn Koorapati via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-15T16:28:36Z","receivedAt":"2020-10-15T16:28:40Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"From: Alex Vandiver \n\nSigned-off-by: Alex Vandiver \nSigned-off-by: Nipunn Koorapati \n---\n dir.c: Fix comments to agree with argument name\n \n Comments are out of date with the variable names.\n \n Update since v1\n \n * Change \"SHA1\" to \"oid\" for consistency as suggested by Peff\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-757%2Fnipunn1313%2Fcomments-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-757/nipunn1313/comments-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/757\n\nRange-diff vs v1:\n\n 1: 08ad8fe7af ! 1: 571fa7dd3d dir.c: fix comments to agree with argument name\n @@ dir.c: static int add_patterns_from_buffer(char *buf, size_t size,\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 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 input.\n\n\n dir.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex 78387110e6..ebea5f1f91 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -1040,9 +1040,9 @@ 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 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@@ -1090,7 +1090,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\nbase-commit: d4a392452e292ff924e79ec8458611c0f679d6d4\n-- \ngitgitgadget\n"},{"id":"407653","messageId":"xmqqk0vrfi1r.fsf@gitster.c.googlers.com","threadId":"54427","inReplyTo":"20201015160725.GA1104947@coredump.intra.peff.net","subject":"Re: [PATCH] dir.c: fix comments to agree with argument name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-15T18:41:36Z","receivedAt":"2020-10-15T18:41:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> - * If \"ss\" is not NULL, compute SHA-1 of the exclude file and fill\n>> + * If \"oid_stat\" is not NULL, compute SHA-1 of the exclude file and fill\n>\n> Makes sense. This changed as part of 4b33e60201 (dir: convert struct\n> sha1_stat to use object_id, 2018-01-28). Perhaps it would likewise make\n> sense to stop saying \"SHA-1\" here, and just say \"hash\" (or even \"object\n> id\", though TBH I think the fact that the hash is the same as an\n> object-id is largely an implementation detail).\n\nI do not quite get your \"though TBH\", though. I do agree with you\nthat it is an implementation detail that an object is named after\nthe hash of its contents, so saying \"compute object name\" probably\nmakes sense in more context than \"compute hash\" outside the narrow\nparts of the code that actually implements how object names are\ncomputed. So I would have expected \"because TBH\", not \"though TBH\".\n\nAnyway. Nipunn, can you fix both of them in the same commit, as\nthey are addressing a problem from the same cause (i.e. we are no\nlonger SHA-1 centric).\n\nThanks.\n"},{"id":"407665","messageId":"CAN8Z4-XdDzJL6k4wZqKJt3=mXajycb0oj-f0WSjcLaFCFRmHKA@mail.gmail.com","threadId":"54427","inReplyTo":"xmqqk0vrfi1r.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] dir.c: fix comments to agree with argument name","fromName":"Nipunn Koorapati","fromEmail":"nipunn1313@gmail.com","sentAt":"2020-10-15T19:33:21Z","receivedAt":"2020-10-15T19:33:36Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"Happy to update it to use the object based terminology, though I'm not\nsure how the desired final result differs from above.\nI believe I said \"compute oid\" in the comment - and it is all in one commit.\n\ngitgitgadget appears to have shown a range-diff from the previous\niteration, but the latest iteration is still one commit.\n\n--Nipunn\n\nOn Thu, Oct 15, 2020 at 7:41 PM Junio C Hamano wrote:\n>\n> Jeff King writes:\n>\n> >> - * If \"ss\" is not NULL, compute SHA-1 of the exclude file and fill\n> >> + * If \"oid_stat\" is not NULL, compute SHA-1 of the exclude file and fill\n> >\n> > Makes sense. This changed as part of 4b33e60201 (dir: convert struct\n> > sha1_stat to use object_id, 2018-01-28). Perhaps it would likewise make\n> > sense to stop saying \"SHA-1\" here, and just say \"hash\" (or even \"object\n> > id\", though TBH I think the fact that the hash is the same as an\n> > object-id is largely an implementation detail).\n>\n> I do not quite get your \"though TBH\", though. I do agree with you\n> that it is an implementation detail that an object is named after\n> the hash of its contents, so saying \"compute object name\" probably\n> makes sense in more context than \"compute hash\" outside the narrow\n> parts of the code that actually implements how object names are\n> computed. So I would have expected \"because TBH\", not \"though TBH\".\n>\n> Anyway. Nipunn, can you fix both of them in the same commit, as\n> they are addressing a problem from the same cause (i.e. we are no\n> longer SHA-1 centric).\n>\n> Thanks.\n"},{"id":"407669","messageId":"20201015194158.GA1490964@coredump.intra.peff.net","threadId":"54427","inReplyTo":"xmqqk0vrfi1r.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] dir.c: fix comments to agree with argument name","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-15T19:41:58Z","receivedAt":"2020-10-15T19:42:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 15, 2020 at 11:41:36AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> >> - * If \"ss\" is not NULL, compute SHA-1 of the exclude file and fill\n> >> + * If \"oid_stat\" is not NULL, compute SHA-1 of the exclude file and fill\n> >\n> > Makes sense. This changed as part of 4b33e60201 (dir: convert struct\n> > sha1_stat to use object_id, 2018-01-28). Perhaps it would likewise make\n> > sense to stop saying \"SHA-1\" here, and just say \"hash\" (or even \"object\n> > id\", though TBH I think the fact that the hash is the same as an\n> > object-id is largely an implementation detail).\n> \n> I do not quite get your \"though TBH\", though. I do agree with you\n> that it is an implementation detail that an object is named after\n> the hash of its contents, so saying \"compute object name\" probably\n> makes sense in more context than \"compute hash\" outside the narrow\n> parts of the code that actually implements how object names are\n> computed. So I would have expected \"because TBH\", not \"though TBH\".\n\nSorry, I was just confused.\n\nThe implementation detail I meant is that we are using a \"struct\nobject_id\" in the oid_stat type (and also that \"oid_stat\" is likewise\nexposing too much). I thought this was another version of our\nstat_validity, where we are checking quickly to see if a random file\nthat is not necessarily part of the working tree has been updated.\n\nAnd indeed, we do use it that way for files like .git/info/exclude,\nwhere having an \"object_id\" is really irrelevant. But we also use it for\nchecking the validity of tracked files, where we populate it from an\nactual blob (see dir.c:do_read_blob).\n\nSo it is actually reasonable to expose that in the name, and to think\nabout it as an object_id.\n\n> Anyway. Nipunn, can you fix both of them in the same commit, as\n> they are addressing a problem from the same cause (i.e. we are no\n> longer SHA-1 centric).\n\nThe v2 that Nipunn sent with \"oid\" in the comment looks good to me.\n\n-Peff\n"},{"id":"407675","messageId":"xmqqsgafdyri.fsf@gitster.c.googlers.com","threadId":"54427","inReplyTo":"20201015194158.GA1490964@coredump.intra.peff.net","subject":"Re: [PATCH] dir.c: fix comments to agree with argument name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-15T20:23:29Z","receivedAt":"2020-10-15T20:23:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> Anyway. Nipunn, can you fix both of them in the same commit, as\n>> they are addressing a problem from the same cause (i.e. we are no\n>> longer SHA-1 centric).\n>\n> The v2 that Nipunn sent with \"oid\" in the comment looks good to me.\n\nOK, then all is good. Thanks.\n"},{"id":"407713","messageId":"CAN8Z4-Wiun-e1s5v2G_2qucy_BmWm=fomd=t3=ZU=9KBu_cVCA@mail.gmail.com","threadId":"54427","inReplyTo":"xmqqsgafdyri.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] dir.c: fix comments to agree with argument name","fromName":"Nipunn Koorapati","fromEmail":"nipunn1313@gmail.com","sentAt":"2020-10-16T12:39:55Z","receivedAt":"2020-10-16T12:40:09Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"Sounds good to me. Please let me know if there's anything further I need to do.\nI'm relatively new to git contributions - and my understanding is that\nat this point a maintainer would merge the commit at their leisure.\nHappy to do any further cleanup required to make that possible\n\nThanks\n--Nipunn\n\nOn Thu, Oct 15, 2020 at 9:23 PM Junio C Hamano wrote:\n>\n> Jeff King writes:\n>\n> >> Anyway. Nipunn, can you fix both of them in the same commit, as\n> >> they are addressing a problem from the same cause (i.e. we are no\n> >> longer SHA-1 centric).\n> >\n> > The v2 that Nipunn sent with \"oid\" in the comment looks good to me.\n>\n> OK, then all is good. Thanks.\n"}]}