{"thread":{"id":"53932","subject":"[PATCH] apply: do not fetch when checking object existence","startedAt":"2020-07-28T01:04:11Z","lastAt":"2020-08-06T20:00:50Z","messageCount":9,"participants":["Jonathan Tan","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"402177","messageId":"20200728010403.95142-1-jonathantanmy@google.com","threadId":"53932","inReplyTo":null,"subject":"[PATCH] apply: do not fetch when checking object existence","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-07-28T01:04:03Z","receivedAt":"2020-07-28T01:04:11Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"There have been a few bugs wherein Git fetches missing objects whenever\nthe existence of an object is checked, even though it does not need to\nperform such a fetch. To resolve these bugs, we could look at all the\nplaces that has_object_file() (or a similar function) is used. As a\nfirst step, introduce a new function has_object() that checks for the\nexistence of an object, with a default behavior of not fetching if the\nobject is missing and the repository is a partial clone. As we verify\neach has_object_file() (or similar) usage, we can replace it with\nhas_object(), and we will know that we are done when we can delete\nhas_object_file() (and the other similar functions).\n\nAlso, the new function has_object() has more appropriate defaults:\nbesides not fetching, it also does not recheck packed storage.\n\nThen, use this new function to fix a bug in apply.c.\n\nSigned-off-by: Jonathan Tan \n---\nI mentioned the idea for this change here:\nhttps://lore.kernel.org/git/20200721225020.1352772-1-jonathantanmy@google.com/\n\nI included the same have_repository check introduced in 3e8b7d3c77\n(\"has_sha1_file: don't bother if we are not in a repository\",\n2017-04-13), but I couldn't verify if it is still needed. In particular,\neven at that commit, all the tests pass (after I make a small change\nto an irrelevant test about the order of entries in a cookie file).\n---\n apply.c | 2 +-\n object-store.h | 25 +++++++++++++++++++++++--\n sha1-file.c | 12 ++++++++++++\n t/t4150-am.sh | 16 ++++++++++++++++\n 4 files changed, 52 insertions(+), 3 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex 8bff604dbe..402d80602a 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -3178,7 +3178,7 @@ static int apply_binary(struct apply_state *state,\n \t\treturn 0; /* deletion patch */\n \t}\n \n-\tif (has_object_file(&oid)) {\n+\tif (has_object(the_repository, &oid, 0)) {\n \t\t/* We already have the postimage */\n \t\tenum object_type type;\n \t\tunsigned long size;\ndiff --git a/object-store.h b/object-store.h\nindex f439d47af8..c4fc9dd74e 100644\n--- a/object-store.h\n+++ b/object-store.h\n@@ -239,12 +239,33 @@ int read_loose_object(const char *path,\n \t\t unsigned long *size,\n \t\t void **contents);\n \n+/* Retry packed storage after checking packed and loose storage */\n+#define HAS_OBJECT_RECHECK_PACKED 1\n+\n+/*\n+ * Returns 1 if the object exists. This function will not lazily fetch objects\n+ * in a partial clone.\n+ */\n+int has_object(struct repository *r, const struct object_id *oid,\n+\t unsigned flags);\n+\n+/*\n+ * These macros and functions are deprecated. If checking existence for an\n+ * object that is likely to be missing and/or whose absence is relatively\n+ * inconsequential (or is consequential but the caller is prepared to handle\n+ * it), use has_object(), which has better defaults (no lazy fetch in a partial\n+ * clone and no rechecking of packed storage). In the unlikely event that a\n+ * caller needs to assert existence of an object that it fully expects to\n+ * exist, and wants to trigger a lazy fetch in a partial clone, use\n+ * oid_object_info_extended() with a NULL struct object_info.\n+ *\n+ * These functions can be removed once all callers have migrated to\n+ * has_object() and/or oid_object_info_extended().\n+ */\n #ifndef NO_THE_REPOSITORY_COMPATIBILITY_MACROS\n #define has_sha1_file_with_flags(sha1, flags) repo_has_sha1_file_with_flags(the_repository, sha1, flags)\n #define has_sha1_file(sha1) repo_has_sha1_file(the_repository, sha1)\n #endif\n-\n-/* Same as the above, except for struct object_id. */\n int repo_has_object_file(struct repository *r, const struct object_id *oid);\n int repo_has_object_file_with_flags(struct repository *r,\n \t\t\t\t const struct object_id *oid, int flags);\ndiff --git a/sha1-file.c b/sha1-file.c\nindex ccd34dd9e8..ff444d7abb 100644\n--- a/sha1-file.c\n+++ b/sha1-file.c\n@@ -1988,6 +1988,18 @@ int force_object_loose(const struct object_id *oid, time_t mtime)\n \treturn ret;\n }\n \n+int has_object(struct repository *r, const struct object_id *oid,\n+\t unsigned flags)\n+{\n+\tint quick = !(flags & HAS_OBJECT_RECHECK_PACKED);\n+\tunsigned object_info_flags = OBJECT_INFO_SKIP_FETCH_OBJECT |\n+\t\t(quick ? OBJECT_INFO_QUICK : 0);\n+\n+\tif (!startup_info->have_repository)\n+\t\treturn 0;\n+\treturn oid_object_info_extended(r, oid, NULL, object_info_flags) >= 0;\n+}\n+\n int repo_has_object_file_with_flags(struct repository *r,\n \t\t\t\t const struct object_id *oid, int flags)\n {\ndiff --git a/t/t4150-am.sh b/t/t4150-am.sh\nindex bda4586a79..94a2c76522 100755\n--- a/t/t4150-am.sh\n+++ b/t/t4150-am.sh\n@@ -1133,4 +1133,20 @@ test_expect_success 'am and .gitattibutes' '\n \t)\n '\n \n+test_expect_success 'apply binary blob in partial clone' '\n+\tprintf \"\\\\000\" >binary &&\n+\tgit add binary &&\n+\tgit commit -m \"binary blob\" &&\n+\tgit format-patch --stdout -m HEAD^ >patch &&\n+\n+\ttest_create_repo server &&\n+\ttest_config -C server uploadpack.allowfilter 1 &&\n+\ttest_config -C server uploadpack.allowanysha1inwant 1 &&\n+\tgit clone --filter=blob:none \"file://$(pwd)/server\" client &&\n+\ttest_when_finished \"rm -rf client\" &&\n+\n+\t# Exercise to make sure that it works\n+\tgit -C client am ../patch\n+'\n+\n test_done\n-- \n2.28.0.rc0.142.g3c755180ce-goog\n\n"},{"id":"402178","messageId":"xmqqwo2oe8r9.fsf@gitster.c.googlers.com","threadId":"53932","inReplyTo":"20200728010403.95142-1-jonathantanmy@google.com","subject":"Re: [PATCH] apply: do not fetch when checking object existence","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-28T01:19:38Z","receivedAt":"2020-07-28T01:19:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> There have been a few bugs wherein Git fetches missing objects whenever\n> the existence of an object is checked, even though it does not need to\n> perform such a fetch. To resolve these bugs, we could look at all the\n> places that has_object_file() (or a similar function) is used. As a\n> first step, introduce a new function has_object() that checks for the\n> existence of an object, with a default behavior of not fetching if the\n> object is missing and the repository is a partial clone. As we verify\n> each has_object_file() (or similar) usage, we can replace it with\n> has_object(), and we will know that we are done when we can delete\n> has_object_file() (and the other similar functions).\n\nI wonder if we want to name the two (i.e. one variant that refuses\nto go to network because it is trying to see if a lazy fetch is\nneeded, and the other that goes to network behind caller's back for\nease of use in a lazy clone) a bit more distinctly so that which one\ncould potentially go outside.\n\nDepending on one's view which one is _normal_ access pattern, giving\nan explicit adverb to one variant while leaving the other one bland\nmight be sufficient. For example, I _think_ most of the places do\nnot want to handle the details of lazily fetching themselves, and I\nsuspect that the traditional has_object_file() semantics without \"do\nnot trigger lazy fetch\" option would be the normal access pattern.\n\nIn which case, renaming your new \"has_object\" to something like\n\"has_object_locally()\" would be a good name for a special case\ncodepath that wants to care---if the object does not exist locally\nand needs to be obtained lazily from elsewhere, the function would\nsay \"no\".\n\nAnd all the other names like has_object_file() that by default gives\ncallers a transparent access to lazily fetched objects can stay the\nsame.\n\n> I mentioned the idea for this change here:\n> https://lore.kernel.org/git/20200721225020.1352772-1-jonathantanmy@google.com/\n\nYup, I think that is going in a good direction. I suspect that\napply will not be the only remaining case we need to \"fix\", and\nusing the new helper function, codepaths that have already been\n\"fixed\" by passing \"do not lazily fetch\" option to the traditional\nAPI functions would become easier to read. And if that is the case,\nlet's have the introduction of the helper function as a separate\npatch, with each of [PATCH 2-N/N] be a fix for separate codepaths.\n\nThanks.\n"},{"id":"402226","messageId":"20200728182341.342162-1-jonathantanmy@google.com","threadId":"53932","inReplyTo":"xmqqwo2oe8r9.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] apply: do not fetch when checking object existence","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-07-28T18:23:40Z","receivedAt":"2020-07-28T18:23:48Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Jonathan Tan writes:\n> \n> > There have been a few bugs wherein Git fetches missing objects whenever\n> > the existence of an object is checked, even though it does not need to\n> > perform such a fetch. To resolve these bugs, we could look at all the\n> > places that has_object_file() (or a similar function) is used. As a\n> > first step, introduce a new function has_object() that checks for the\n> > existence of an object, with a default behavior of not fetching if the\n> > object is missing and the repository is a partial clone. As we verify\n> > each has_object_file() (or similar) usage, we can replace it with\n> > has_object(), and we will know that we are done when we can delete\n> > has_object_file() (and the other similar functions).\n> \n> I wonder if we want to name the two (i.e. one variant that refuses\n> to go to network because it is trying to see if a lazy fetch is\n> needed, and the other that goes to network behind caller's back for\n> ease of use in a lazy clone) a bit more distinctly so that which one\n> could potentially go outside.\n> \n> Depending on one's view which one is _normal_ access pattern, giving\n> an explicit adverb to one variant while leaving the other one bland\n> might be sufficient. For example, I _think_ most of the places do\n> not want to handle the details of lazily fetching themselves, and I\n> suspect that the traditional has_object_file() semantics without \"do\n> not trigger lazy fetch\" option would be the normal access pattern.\n\nRight now, I think that most (if not all) places would not want to fetch\nat all - so *with* \"do not trigger lazy fetch\" would be the normal\naccess pattern. This is because (in my opinion) if a caller checks the\nexistence of an object, it most likely can tolerate the object's\nabsence; if the caller couldn't tolerate it, it would just directly\nquery for its type or contents or something like that.\n\nI tried to communicate this in my documentation of the deprecated\nfunctions/macros, but perhaps it could be written better.\n\n(One other option to consider is to just change has_object_file() to\nnever fetch, although I think this is more risky.)\n\n> In which case, renaming your new \"has_object\" to something like\n> \"has_object_locally()\" would be a good name for a special case\n> codepath that wants to care---if the object does not exist locally\n> and needs to be obtained lazily from elsewhere, the function would\n> say \"no\".\n> \n> And all the other names like has_object_file() that by default gives\n> callers a transparent access to lazily fetched objects can stay the\n> same.\n\nIf my analysis above is wrong, then yes I agree that we should do this.\nBut we might need to find another way to indicate which has_object_file()\nhas been checked and which hasn't - changing away from has_object_file()\ncompletely gives us a way to indicate this, but if we're sticking with\nhas_object_file(), we have to find another way of indicating that we've\nlooked at this call and it is OK.\n\n> > I mentioned the idea for this change here:\n> > https://lore.kernel.org/git/20200721225020.1352772-1-jonathantanmy@google.com/\n> \n> Yup, I think that is going in a good direction. I suspect that\n> apply will not be the only remaining case we need to \"fix\", and\n> using the new helper function, codepaths that have already been\n> \"fixed\" by passing \"do not lazily fetch\" option to the traditional\n> API functions would become easier to read. And if that is the case,\n> let's have the introduction of the helper function as a separate\n> patch, with each of [PATCH 2-N/N] be a fix for separate codepaths.\n> \n> Thanks.\n\nOK - I'll separate out the helper function into its own patch in version\n2.\n"},{"id":"403002","messageId":"cover.1596668156.git.jonathantanmy@google.com","threadId":"53932","inReplyTo":"20200728010403.95142-1-jonathantanmy@google.com","subject":"[PATCH v2 0/4] No-lazy-fetch has_object() and some fixes","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-08-05T23:06:48Z","receivedAt":"2020-08-05T23:07:02Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Since v1, I've took a look at 3 other cases that use has_object_file():\n2 don't need lazy-fetching (patches 3 and 4 in this set) and 1 does -\n\"cat-file -e\" - although I think this is a special case. So I still\nthink that not lazy-fetching when checking object existence is more\nlikely, and should be privileged with the shorter function name\n(has_object() instead of has_object_locally()).\n\nChanges from v1:\n - Patch split into 2 (patch 1 and patch 2)\n - 2 additional patches that fix bugs by making use of the new function\n\nJonathan Tan (4):\n sha1-file: introduce no-lazy-fetch has_object()\n apply: do not lazy fetch when applying binary\n pack-objects: no fetch when allow-{any,promisor}\n fsck: do not lazy fetch known non-promisor object\n\n Documentation/git-pack-objects.txt | 11 +++++++----\n apply.c | 2 +-\n builtin/fsck.c | 2 +-\n builtin/pack-objects.c | 4 ++--\n object-store.h | 25 +++++++++++++++++++++++--\n sha1-file.c | 12 ++++++++++++\n t/t4150-am.sh | 16 ++++++++++++++++\n 7 files changed, 62 insertions(+), 10 deletions(-)\n\n-- \n2.28.0.236.gb10cc79966-goog\n\n"},{"id":"403003","messageId":"f413b34574a330030e08a6eb93ad812975959d7d.1596668156.git.jonathantanmy@google.com","threadId":"53932","inReplyTo":"cover.1596668156.git.jonathantanmy@google.com","subject":"[PATCH v2 1/4] sha1-file: introduce no-lazy-fetch has_object()","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-08-05T23:06:49Z","receivedAt":"2020-08-05T23:07:06Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"There have been a few bugs wherein Git fetches missing objects whenever\nthe existence of an object is checked, even though it does not need to\nperform such a fetch. To resolve these bugs, we could look at all the\nplaces that has_object_file() (or a similar function) is used. As a\nfirst step, introduce a new function has_object() that checks for the\nexistence of an object, with a default behavior of not fetching if the\nobject is missing and the repository is a partial clone. As we verify\neach has_object_file() (or similar) usage, we can replace it with\nhas_object(), and we will know that we are done when we can delete\nhas_object_file() (and the other similar functions).\n\nAlso, the new function has_object() has more appropriate defaults:\nbesides not fetching, it also does not recheck packed storage.\n\nSigned-off-by: Jonathan Tan \n---\n object-store.h | 25 +++++++++++++++++++++++--\n sha1-file.c | 12 ++++++++++++\n 2 files changed, 35 insertions(+), 2 deletions(-)\n\ndiff --git a/object-store.h b/object-store.h\nindex f439d47af8..c4fc9dd74e 100644\n--- a/object-store.h\n+++ b/object-store.h\n@@ -239,12 +239,33 @@ int read_loose_object(const char *path,\n \t\t unsigned long *size,\n \t\t void **contents);\n \n+/* Retry packed storage after checking packed and loose storage */\n+#define HAS_OBJECT_RECHECK_PACKED 1\n+\n+/*\n+ * Returns 1 if the object exists. This function will not lazily fetch objects\n+ * in a partial clone.\n+ */\n+int has_object(struct repository *r, const struct object_id *oid,\n+\t unsigned flags);\n+\n+/*\n+ * These macros and functions are deprecated. If checking existence for an\n+ * object that is likely to be missing and/or whose absence is relatively\n+ * inconsequential (or is consequential but the caller is prepared to handle\n+ * it), use has_object(), which has better defaults (no lazy fetch in a partial\n+ * clone and no rechecking of packed storage). In the unlikely event that a\n+ * caller needs to assert existence of an object that it fully expects to\n+ * exist, and wants to trigger a lazy fetch in a partial clone, use\n+ * oid_object_info_extended() with a NULL struct object_info.\n+ *\n+ * These functions can be removed once all callers have migrated to\n+ * has_object() and/or oid_object_info_extended().\n+ */\n #ifndef NO_THE_REPOSITORY_COMPATIBILITY_MACROS\n #define has_sha1_file_with_flags(sha1, flags) repo_has_sha1_file_with_flags(the_repository, sha1, flags)\n #define has_sha1_file(sha1) repo_has_sha1_file(the_repository, sha1)\n #endif\n-\n-/* Same as the above, except for struct object_id. */\n int repo_has_object_file(struct repository *r, const struct object_id *oid);\n int repo_has_object_file_with_flags(struct repository *r,\n \t\t\t\t const struct object_id *oid, int flags);\ndiff --git a/sha1-file.c b/sha1-file.c\nindex ccd34dd9e8..ff444d7abb 100644\n--- a/sha1-file.c\n+++ b/sha1-file.c\n@@ -1988,6 +1988,18 @@ int force_object_loose(const struct object_id *oid, time_t mtime)\n \treturn ret;\n }\n \n+int has_object(struct repository *r, const struct object_id *oid,\n+\t unsigned flags)\n+{\n+\tint quick = !(flags & HAS_OBJECT_RECHECK_PACKED);\n+\tunsigned object_info_flags = OBJECT_INFO_SKIP_FETCH_OBJECT |\n+\t\t(quick ? OBJECT_INFO_QUICK : 0);\n+\n+\tif (!startup_info->have_repository)\n+\t\treturn 0;\n+\treturn oid_object_info_extended(r, oid, NULL, object_info_flags) >= 0;\n+}\n+\n int repo_has_object_file_with_flags(struct repository *r,\n \t\t\t\t const struct object_id *oid, int flags)\n {\n-- \n2.28.0.236.gb10cc79966-goog\n\n"},{"id":"403004","messageId":"1b5e1f02d29b990497ad274e1e67b072feb77eec.1596668156.git.jonathantanmy@google.com","threadId":"53932","inReplyTo":"cover.1596668156.git.jonathantanmy@google.com","subject":"[PATCH v2 3/4] pack-objects: no fetch when allow-{any,promisor}","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-08-05T23:06:51Z","receivedAt":"2020-08-05T23:07:11Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"The options --missing=allow-{any,promisor} were introduced in caf3827e2f\n(\"rev-list: add list-objects filtering support\", 2017-11-22) with the\nfollowing note in the commit message:\n\n This patch introduces handling of missing objects to help\n debugging and development of the \"partial clone\" mechanism,\n and once the mechanism is implemented, for a power user to\n perform operations that are missing-object aware without\n incurring the cost of checking if a missing link is expected.\n\nThe idea that these options are missing-object aware (and thus do not\nneed to lazily fetch objects, unlike unaware commands that assume that\nall objects are present) are assumed in later commits such as 07ef3c6604\n(\"fetch test: use more robust test for filtered objects\", 2020-01-15).\n\nHowever, the current implementations of these options use\nhas_object_file(), which indeed lazily fetches missing objects. Teach\nthese implementations not to do so. Also, update the documentation of\nthese options to be clearer.\n\nSigned-off-by: Jonathan Tan \n---\n Documentation/git-pack-objects.txt | 11 +++++++----\n builtin/pack-objects.c | 4 ++--\n 2 files changed, 9 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/git-pack-objects.txt b/Documentation/git-pack-objects.txt\nindex eaa2f2a404..54d715ead1 100644\n--- a/Documentation/git-pack-objects.txt\n+++ b/Documentation/git-pack-objects.txt\n@@ -270,15 +270,18 @@ So does `git bundle` (see linkgit:git-bundle[1]) when it creates a bundle.\n \tThis option specifies how missing objects are handled.\n +\n The form '--missing=error' requests that pack-objects stop with an error if\n-a missing object is encountered. This is the default action.\n+a missing object is encountered. If the repository is a partial clone, an\n+attempt to fetch missing objects will be made before declaring them missing.\n+This is the default action.\n +\n The form '--missing=allow-any' will allow object traversal to continue\n-if a missing object is encountered. Missing objects will silently be\n-omitted from the results.\n+if a missing object is encountered. No fetch of a missing object will occur.\n+Missing objects will silently be omitted from the results.\n +\n The form '--missing=allow-promisor' is like 'allow-any', but will only\n allow object traversal to continue for EXPECTED promisor missing objects.\n-Unexpected missing object will raise an error.\n+No fetch of a missing object will occur. An unexpected missing object will\n+raise an error.\n \n --exclude-promisor-objects::\n \tOmit objects that are known to be in the promisor remote. (This\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 7016b28485..2a2afb7cb1 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3016,7 +3016,7 @@ static void show_object__ma_allow_any(struct object *obj, const char *name, void\n \t * Quietly ignore ALL missing objects. This avoids problems with\n \t * staging them now and getting an odd error later.\n \t */\n-\tif (!has_object_file(&obj->oid))\n+\tif (!has_object(the_repository, &obj->oid, 0))\n \t\treturn;\n \n \tshow_object(obj, name, data);\n@@ -3030,7 +3030,7 @@ static void show_object__ma_allow_promisor(struct object *obj, const char *name,\n \t * Quietly ignore EXPECTED missing objects. This avoids problems with\n \t * staging them now and getting an odd error later.\n \t */\n-\tif (!has_object_file(&obj->oid) && is_promisor_object(&obj->oid))\n+\tif (!has_object(the_repository, &obj->oid, 0) && is_promisor_object(&obj->oid))\n \t\treturn;\n \n \tshow_object(obj, name, data);\n-- \n2.28.0.236.gb10cc79966-goog\n\n"},{"id":"403005","messageId":"efa3b0719d628bbbd3923725c2d8e0c0d0018876.1596668156.git.jonathantanmy@google.com","threadId":"53932","inReplyTo":"cover.1596668156.git.jonathantanmy@google.com","subject":"[PATCH v2 4/4] fsck: do not lazy fetch known non-promisor object","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-08-05T23:06:52Z","receivedAt":"2020-08-05T23:07:13Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"There is a call to has_object_file(), which lazily fetches missing\nobjects in a partial clone, when the object is known to not be\na promisor object. Change that call to has_object(), which does not do\nany lazy fetching.\n\nSigned-off-by: Jonathan Tan \n---\n builtin/fsck.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex 37aa07da78..fbf26cafcf 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -168,7 +168,7 @@ static int mark_object(struct object *obj, int type, void *data, struct fsck_opt\n \t\treturn 0;\n \n \tif (!(obj->flags & HAS_OBJ)) {\n-\t\tif (parent && !has_object_file(&obj->oid)) {\n+\t\tif (parent && !has_object(the_repository, &obj->oid, 1)) {\n \t\t\tprintf_ln(_(\"broken link from %7s %s\\n\"\n \t\t\t\t \" to %7s %s\"),\n \t\t\t\t printable_type(&parent->oid, parent->type),\n-- \n2.28.0.236.gb10cc79966-goog\n\n"},{"id":"403006","messageId":"fa09987fb010042fdcbd3ce539b783b92bb640d6.1596668156.git.jonathantanmy@google.com","threadId":"53932","inReplyTo":"cover.1596668156.git.jonathantanmy@google.com","subject":"[PATCH v2 2/4] apply: do not lazy fetch when applying binary","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-08-05T23:06:50Z","receivedAt":"2020-08-05T23:07:38Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When applying a binary patch, as an optimization, \"apply\" checks if the\npostimage is already present. During this fetch, it is perfectly\nexpected for the postimage not to be present, so there is no need to\nlazy-fetch missing objects. Teach \"apply\" not to lazy-fetch in this\ncase.\n\nSigned-off-by: Jonathan Tan \n---\n apply.c | 2 +-\n t/t4150-am.sh | 16 ++++++++++++++++\n 2 files changed, 17 insertions(+), 1 deletion(-)\n\ndiff --git a/apply.c b/apply.c\nindex 8bff604dbe..402d80602a 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -3178,7 +3178,7 @@ static int apply_binary(struct apply_state *state,\n \t\treturn 0; /* deletion patch */\n \t}\n \n-\tif (has_object_file(&oid)) {\n+\tif (has_object(the_repository, &oid, 0)) {\n \t\t/* We already have the postimage */\n \t\tenum object_type type;\n \t\tunsigned long size;\ndiff --git a/t/t4150-am.sh b/t/t4150-am.sh\nindex bda4586a79..94a2c76522 100755\n--- a/t/t4150-am.sh\n+++ b/t/t4150-am.sh\n@@ -1133,4 +1133,20 @@ test_expect_success 'am and .gitattibutes' '\n \t)\n '\n \n+test_expect_success 'apply binary blob in partial clone' '\n+\tprintf \"\\\\000\" >binary &&\n+\tgit add binary &&\n+\tgit commit -m \"binary blob\" &&\n+\tgit format-patch --stdout -m HEAD^ >patch &&\n+\n+\ttest_create_repo server &&\n+\ttest_config -C server uploadpack.allowfilter 1 &&\n+\ttest_config -C server uploadpack.allowanysha1inwant 1 &&\n+\tgit clone --filter=blob:none \"file://$(pwd)/server\" client &&\n+\ttest_when_finished \"rm -rf client\" &&\n+\n+\t# Exercise to make sure that it works\n+\tgit -C client am ../patch\n+'\n+\n test_done\n-- \n2.28.0.236.gb10cc79966-goog\n\n"},{"id":"403089","messageId":"xmqqwo2bk0ib.fsf@gitster.c.googlers.com","threadId":"53932","inReplyTo":"cover.1596668156.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 0/4] No-lazy-fetch has_object() and some fixes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-06T20:00:44Z","receivedAt":"2020-08-06T20:00:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> Since v1, I've took a look at 3 other cases that use has_object_file():\n> 2 don't need lazy-fetching (patches 3 and 4 in this set) and 1 does -\n> \"cat-file -e\" - although I think this is a special case. So I still\n> think that not lazy-fetching when checking object existence is more\n> likely, and should be privileged with the shorter function name\n> (has_object() instead of has_object_locally()).\n\nThanks. has_object then.\n\n>\n> Changes from v1:\n> - Patch split into 2 (patch 1 and patch 2)\n> - 2 additional patches that fix bugs by making use of the new function\n>\n> Jonathan Tan (4):\n> sha1-file: introduce no-lazy-fetch has_object()\n> apply: do not lazy fetch when applying binary\n> pack-objects: no fetch when allow-{any,promisor}\n> fsck: do not lazy fetch known non-promisor object\n>\n> Documentation/git-pack-objects.txt | 11 +++++++----\n> apply.c | 2 +-\n> builtin/fsck.c | 2 +-\n> builtin/pack-objects.c | 4 ++--\n> object-store.h | 25 +++++++++++++++++++++++--\n> sha1-file.c | 12 ++++++++++++\n> t/t4150-am.sh | 16 ++++++++++++++++\n> 7 files changed, 62 insertions(+), 10 deletions(-)\n"}]}