{"thread":{"id":"51190","subject":"[PATCH 0/1] sha1-file: split OBJECT_INFO_FOR_PREFETCH","startedAt":"2019-05-28T15:19:10Z","lastAt":"2019-05-29T00:29:41Z","messageCount":5,"participants":["Derrick Stolee via GitGitGadget","Junio C Hamano","Jeff King","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"376309","messageId":"pull.228.git.gitgitgadget@gmail.com","threadId":"51190","inReplyTo":null,"subject":"[PATCH 0/1] sha1-file: split OBJECT_INFO_FOR_PREFETCH","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-05-28T15:19:06Z","receivedAt":"2019-05-28T15:19:10Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"I found this interaction while testing the VFS for Git fork rebasing onto\nv2.22.0-rc1 [1]. It seems this new flag meant for partial clone prefetches\ninteracts poorly with the read-object hook we use in our fork.\n\nThe issue is that OBJECT_INFO_FOR_PREFETCH has multiple bits on, so testing\n\"flag & OBJECT_INFO_FOR_PREFETCH\" can be true even if not all bits are on.\nMy fix simply splits the new bit into a special flag while keeping\nOBJECT_INFO_FOR_PREFETCH as a union of flags.\n\nJury is out if this fixes our problem, but it definitely seems like a bug\nwaiting to happen in Git, too.\n\nThanks, -Stolee\n\n[1] https://github.com/microsoft/git/pull/140\n\nDerrick Stolee (1):\n sha1-file: split OBJECT_INFO_FOR_PREFETCH\n\n object-store.h | 10 +++++++---\n sha1-file.c | 2 +-\n 2 files changed, 8 insertions(+), 4 deletions(-)\n\n\nbase-commit: 0f4a4fb1c4239a2aa46343add84ad6f99f6f3aae\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-228%2Fderrickstolee%2Fobject-info-prefetch-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-228/derrickstolee/object-info-prefetch-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/228\n-- \ngitgitgadget\n"},{"id":"376310","messageId":"2737e62966cae2f00d75a93446bad76e5816d07e.1559056745.git.gitgitgadget@gmail.com","threadId":"51190","inReplyTo":"pull.228.git.gitgitgadget@gmail.com","subject":"[PATCH 1/1] sha1-file: split OBJECT_INFO_FOR_PREFETCH","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-05-28T15:19:07Z","receivedAt":"2019-05-28T15:19:11Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe OBJECT_INFO_FOR_PREFETCH bitflag was added to sha1-file.c in 0f4a4fb1\n(sha1-file: support OBJECT_INFO_FOR_PREFETCH, 2019-03-29) and is used to\nprevent the fetch_objects() method when enabled.\n\nHowever, there is a problem with the current use. The definition of\nOBJECT_INFO_FOR_PREFETCH is given by adding 32 to OBJECT_INFO_QUICK. This is\nclearly stated above the definition (in a comment) that this is so\nOBJECT_INFO_FOR_PREFETCH implies OBJECT_INFO_QUICK. The problem is that using\n\"flag & OBJECT_INFO_FOR_PREFETCH\" means that OBJECT_INFO_QUICK also implies\nOBJECT_INFO_FOR_PREFETCH.\n\nSplit out the single bit from OBJECT_INFO_FOR_PREFETCH into a new\nOBJECT_INFO_SKIP_FETCH_OBJECT as the single bit and keep\nOBJECT_INFO_FOR_PREFETCH as the union of two flags. This allows a clearer use\nof flag checking while also keeping the implication of OBJECT_INFO_QUICK.\n\nSigned-off-by: Derrick Stolee \n---\n object-store.h | 10 +++++++---\n sha1-file.c | 2 +-\n 2 files changed, 8 insertions(+), 4 deletions(-)\n\ndiff --git a/object-store.h b/object-store.h\nindex dd3f9b75f0..c90628d839 100644\n--- a/object-store.h\n+++ b/object-store.h\n@@ -282,10 +282,14 @@ struct object_info {\n #define OBJECT_INFO_IGNORE_LOOSE 16\n /*\n * Do not attempt to fetch the object if missing (even if fetch_is_missing is\n- * nonzero). This is meant for bulk prefetching of missing blobs in a partial\n- * clone. Implies OBJECT_INFO_QUICK.\n+ * nonzero).\n */\n-#define OBJECT_INFO_FOR_PREFETCH (32 + OBJECT_INFO_QUICK)\n+#define OBJECT_INFO_SKIP_FETCH_OBJECT 32\n+/*\n+ * This is meant for bulk prefetching of missing blobs in a partial\n+ * clone. Implies OBJECT_INFO_SKIP_FETCH_OBJECT and OBJECT_INFO_QUICK\n+ */\n+#define OBJECT_INFO_FOR_PREFETCH (OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK)\n \n int oid_object_info_extended(struct repository *r,\n \t\t\t const struct object_id *,\ndiff --git a/sha1-file.c b/sha1-file.c\nindex ad02649124..0299fdd516 100644\n--- a/sha1-file.c\n+++ b/sha1-file.c\n@@ -1371,7 +1371,7 @@ int oid_object_info_extended(struct repository *r, const struct object_id *oid,\n \t\t/* Check if it is a missing object */\n \t\tif (fetch_if_missing && repository_format_partial_clone &&\n \t\t !already_retried && r == the_repository &&\n-\t\t !(flags & OBJECT_INFO_FOR_PREFETCH)) {\n+\t\t !(flags & OBJECT_INFO_SKIP_FETCH_OBJECT)) {\n \t\t\t/*\n \t\t\t * TODO Investigate having fetch_object() return\n \t\t\t * TODO error/success and stopping the music here.\n-- \ngitgitgadget\n"},{"id":"376348","messageId":"xmqqblzm1fuc.fsf@gitster-ct.c.googlers.com","threadId":"51190","inReplyTo":"2737e62966cae2f00d75a93446bad76e5816d07e.1559056745.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/1] sha1-file: split OBJECT_INFO_FOR_PREFETCH","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-05-28T20:31:07Z","receivedAt":"2019-05-28T20:31:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> From: Derrick Stolee \n>\n> The OBJECT_INFO_FOR_PREFETCH bitflag was added to sha1-file.c in 0f4a4fb1\n> (sha1-file: support OBJECT_INFO_FOR_PREFETCH, 2019-03-29) and is used to\n> prevent the fetch_objects() method when enabled.\n>\n> However, there is a problem with the current use. The definition of\n> OBJECT_INFO_FOR_PREFETCH is given by adding 32 to OBJECT_INFO_QUICK. This is\n> clearly stated above the definition (in a comment) that this is so\n> OBJECT_INFO_FOR_PREFETCH implies OBJECT_INFO_QUICK. The problem is that using\n> \"flag & OBJECT_INFO_FOR_PREFETCH\" means that OBJECT_INFO_QUICK also implies\n> OBJECT_INFO_FOR_PREFETCH.\n\nSo the right test to see prefetch is in effect is not\n\n\tif (!!(flag & TWO_BITS))\n\nbut to use\n\n\tif ((flag & TWO_BITS) == TWO_BITS)\n\ninstead?\n\n> Split out the single bit from OBJECT_INFO_FOR_PREFETCH into a new\n> OBJECT_INFO_SKIP_FETCH_OBJECT as the single bit and keep\n> OBJECT_INFO_FOR_PREFETCH as the union of two flags. This allows a clearer use\n> of flag checking while also keeping the implication of OBJECT_INFO_QUICK.\n\nOK, I guess that would work and with far less damage to the existing\ncode.\n\n>\n> Signed-off-by: Derrick Stolee \n> ---\n> object-store.h | 10 +++++++---\n> sha1-file.c | 2 +-\n> 2 files changed, 8 insertions(+), 4 deletions(-)\n>\n> diff --git a/object-store.h b/object-store.h\n> index dd3f9b75f0..c90628d839 100644\n> --- a/object-store.h\n> +++ b/object-store.h\n> @@ -282,10 +282,14 @@ struct object_info {\n> #define OBJECT_INFO_IGNORE_LOOSE 16\n> /*\n> * Do not attempt to fetch the object if missing (even if fetch_is_missing is\n> - * nonzero). This is meant for bulk prefetching of missing blobs in a partial\n> - * clone. Implies OBJECT_INFO_QUICK.\n> + * nonzero).\n> */\n> -#define OBJECT_INFO_FOR_PREFETCH (32 + OBJECT_INFO_QUICK)\n> +#define OBJECT_INFO_SKIP_FETCH_OBJECT 32\n> +/*\n> + * This is meant for bulk prefetching of missing blobs in a partial\n> + * clone. Implies OBJECT_INFO_SKIP_FETCH_OBJECT and OBJECT_INFO_QUICK\n> + */\n> +#define OBJECT_INFO_FOR_PREFETCH (OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK)\n> \n> int oid_object_info_extended(struct repository *r,\n> \t\t\t const struct object_id *,\n> diff --git a/sha1-file.c b/sha1-file.c\n> index ad02649124..0299fdd516 100644\n> --- a/sha1-file.c\n> +++ b/sha1-file.c\n> @@ -1371,7 +1371,7 @@ int oid_object_info_extended(struct repository *r, const struct object_id *oid,\n> \t\t/* Check if it is a missing object */\n> \t\tif (fetch_if_missing && repository_format_partial_clone &&\n> \t\t !already_retried && r == the_repository &&\n> -\t\t !(flags & OBJECT_INFO_FOR_PREFETCH)) {\n> +\t\t !(flags & OBJECT_INFO_SKIP_FETCH_OBJECT)) {\n> \t\t\t/*\n> \t\t\t * TODO Investigate having fetch_object() return\n> \t\t\t * TODO error/success and stopping the music here.\n"},{"id":"376354","messageId":"20190528205441.GB24650@sigill.intra.peff.net","threadId":"51190","inReplyTo":"2737e62966cae2f00d75a93446bad76e5816d07e.1559056745.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/1] sha1-file: split OBJECT_INFO_FOR_PREFETCH","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-28T20:54:41Z","receivedAt":"2019-05-28T20:54:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 28, 2019 at 08:19:07AM -0700, Derrick Stolee via GitGitGadget wrote:\n\n> From: Derrick Stolee \n> \n> The OBJECT_INFO_FOR_PREFETCH bitflag was added to sha1-file.c in 0f4a4fb1\n> (sha1-file: support OBJECT_INFO_FOR_PREFETCH, 2019-03-29) and is used to\n> prevent the fetch_objects() method when enabled.\n> \n> However, there is a problem with the current use. The definition of\n> OBJECT_INFO_FOR_PREFETCH is given by adding 32 to OBJECT_INFO_QUICK. This is\n> clearly stated above the definition (in a comment) that this is so\n> OBJECT_INFO_FOR_PREFETCH implies OBJECT_INFO_QUICK. The problem is that using\n> \"flag & OBJECT_INFO_FOR_PREFETCH\" means that OBJECT_INFO_QUICK also implies\n> OBJECT_INFO_FOR_PREFETCH.\n> \n> Split out the single bit from OBJECT_INFO_FOR_PREFETCH into a new\n> OBJECT_INFO_SKIP_FETCH_OBJECT as the single bit and keep\n> OBJECT_INFO_FOR_PREFETCH as the union of two flags. This allows a clearer use\n> of flag checking while also keeping the implication of OBJECT_INFO_QUICK.\n\nOof. I actually suggested splitting these up for review, but thought it\nwas only a clarity/flexibility issue, and completely missed the\ncorrectness aspect of checking when the bit is set.\n\nI agree with Junio's other response that using \"==\" would be the right\nway for a multi-bit check, in general. But I like the split here,\nbecause I think the result is more clear to read and harder to get\nwrong for future checks.\n\nI'd even go so far as to say...\n\n> + * This is meant for bulk prefetching of missing blobs in a partial\n> + * clone. Implies OBJECT_INFO_SKIP_FETCH_OBJECT and OBJECT_INFO_QUICK\n> + */\n> +#define OBJECT_INFO_FOR_PREFETCH (OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK)\n\nwe could dump this, and callers should just say what they mean (i.e.,\nspecify both flags).\n\nThere are only two of them, and I think both would be more readable with\na helper more like:\n\n int should_prefetch_object(struct repository *r,\n const struct object_id *oid) {\n\treturn !oid_object_info_extended(r, oid, NULL,\n\t OBJECT_INFO_SKIP_FETCH_OBJECT |\n\t\t\t\t\t OBJECT_INFO_QUICK);\n }\n\nbut unless everybody is immediately on-board with \"yes, that is much\nnicer\", I don't want bikeshedding to hold up your important and\nobviously-correct fix.\n\n-Peff\n"},{"id":"376376","messageId":"f30f82f9-b07e-d6c3-5ccb-9b08b8424f7c@gmail.com","threadId":"51190","inReplyTo":"20190528205441.GB24650@sigill.intra.peff.net","subject":"Re: [PATCH 1/1] sha1-file: split OBJECT_INFO_FOR_PREFETCH","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-05-29T00:29:36Z","receivedAt":"2019-05-29T00:29:41Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/28/2019 4:54 PM, Jeff King wrote:\n> On Tue, May 28, 2019 at 08:19:07AM -0700, Derrick Stolee via GitGitGadget wrote:\n> \n>> From: Derrick Stolee \n>>\n>> The OBJECT_INFO_FOR_PREFETCH bitflag was added to sha1-file.c in 0f4a4fb1\n>> (sha1-file: support OBJECT_INFO_FOR_PREFETCH, 2019-03-29) and is used to\n>> prevent the fetch_objects() method when enabled.\n>>\n>> However, there is a problem with the current use. The definition of\n>> OBJECT_INFO_FOR_PREFETCH is given by adding 32 to OBJECT_INFO_QUICK. This is\n>> clearly stated above the definition (in a comment) that this is so\n>> OBJECT_INFO_FOR_PREFETCH implies OBJECT_INFO_QUICK. The problem is that using\n>> \"flag & OBJECT_INFO_FOR_PREFETCH\" means that OBJECT_INFO_QUICK also implies\n>> OBJECT_INFO_FOR_PREFETCH.\n>>\n>> Split out the single bit from OBJECT_INFO_FOR_PREFETCH into a new\n>> OBJECT_INFO_SKIP_FETCH_OBJECT as the single bit and keep\n>> OBJECT_INFO_FOR_PREFETCH as the union of two flags. This allows a clearer use\n>> of flag checking while also keeping the implication of OBJECT_INFO_QUICK.\n> \n> Oof. I actually suggested splitting these up for review, but thought it\n> was only a clarity/flexibility issue, and completely missed the\n> correctness aspect of checking when the bit is set.\n> \n> I agree with Junio's other response that using \"==\" would be the right\n> way for a multi-bit check, in general. But I like the split here,\n> because I think the result is more clear to read and harder to get\n> wrong for future checks.\n\nThanks, for the feedback, both of you.\n\n> I'd even go so far as to say...\n> \n>> + * This is meant for bulk prefetching of missing blobs in a partial\n>> + * clone. Implies OBJECT_INFO_SKIP_FETCH_OBJECT and OBJECT_INFO_QUICK\n>> + */\n>> +#define OBJECT_INFO_FOR_PREFETCH (OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK)\n> \n> we could dump this, and callers should just say what they mean (i.e.,\n> specify both flags).\n\nDropping the _PREFETCH flag also makes oid_object_info_extended() slightly\nless \"coupled\" to the prefetch feature, and instead describes more explicitly\nthe way the flag is changing the behavior of the method.\n \n> There are only two of them, and I think both would be more readable with\n> a helper more like:\n> \n> int should_prefetch_object(struct repository *r,\n> const struct object_id *oid) {\n> \treturn !oid_object_info_extended(r, oid, NULL,\n> \t OBJECT_INFO_SKIP_FETCH_OBJECT |\n> \t\t\t\t\t OBJECT_INFO_QUICK);\n> }\n> \n> but unless everybody is immediately on-board with \"yes, that is much\n> nicer\", I don't want bikeshedding to hold up your important and\n> obviously-correct fix.\n\nI'll come back with another series to drop the _PREFETCH flag after the\nrelease calms down. It can give more time for others to chime in here.\n\nThanks, Junio for the quick turnaround in taking the patch.\n\nThanks,\n-Stolee\n"}]}