{"thread":{"id":"53897","subject":"[PATCH] sha1-file: make pretend_object_file() not prefetch","startedAt":"2020-07-21T22:50:28Z","lastAt":"2020-07-23T18:06:22Z","messageCount":5,"participants":["Jonathan Tan","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"401830","messageId":"20200721225020.1352772-1-jonathantanmy@google.com","threadId":"53897","inReplyTo":null,"subject":"[PATCH] sha1-file: make pretend_object_file() not prefetch","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-07-21T22:50:20Z","receivedAt":"2020-07-21T22:50:28Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When pretend_object_file() is invoked with an object that does not exist\n(as is the typical case), there is no need to fetch anything from the\npromisor remote, because the caller already knows what the object is\nsupposed to contain. Therefore, suppress the fetch. (The\nOBJECT_INFO_QUICK flag is added for the same reason.)\n\nThis was noticed at $DAYJOB when \"blame\" was run on a file that had\nuncommitted modifications.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\nThis is another instance of the issue Junio raised here [1]:\n\n> Fixing issues hit by real users reactively is a necessary and good\n> thing, but this is not the first time we patch callers of\n> has_object_file() for this kind of \"we are merely trying to\n> determine the boundary of what we have, so that we know what we need\n> to add to this repository\" queries, I am afraid.\n>\n> Perhaps it is a good idea to sweep all the hits from \"git grep -e\n> has_object_file \\*.c\" and audit the codebase to see if there are\n> other problematic ones?\n\nTo make progress towards solving this, I'm thinking of making a new\nfunction has_object() that takes a repository, an object ID, and flags,\nand only supports one flag: HAS_OBJECT_RECHECK_PACKED (which has the\nopposite meaning of OBJECT_INFO_QUICK). Checks that should not fetch\nshould use has_object(), and checks that should fetch (if they exist - I\nthink that most would want additional information about the object, so\nthey would use oid_object_info() or similar already) should use\noid_object_info() or a similar function. That way we can see how many\nuses of has_object_file() we have checked, and at the same time make\nbehavior we usually want into the default behavior. Any opinions?\n\n[1] https://lore.kernel.org/git/xmqqpn8wie21.fsf@gitster.c.googlers.com/\n---\n sha1-file.c      |  3 ++-\n t/t8002-blame.sh | 11 +++++++++++\n 2 files changed, 13 insertions(+), 1 deletion(-)\n\ndiff --git a/sha1-file.c b/sha1-file.c\nindex ccd34dd9e8..45fdb8415c 100644\n--- a/sha1-file.c\n+++ b/sha1-file.c\n@@ -1600,7 +1600,8 @@ int pretend_object_file(void *buf, unsigned long len, enum object_type type,\n \tstruct cached_object *co;\n \n \thash_object_file(the_hash_algo, buf, len, type_name(type), oid);\n-\tif (has_object_file(oid) || find_cached_object(oid))\n+\tif (has_object_file_with_flags(oid, OBJECT_INFO_QUICK | OBJECT_INFO_SKIP_FETCH_OBJECT) ||\n+\t    find_cached_object(oid))\n \t\treturn 0;\n \tALLOC_GROW(cached_objects, cached_object_nr + 1, cached_object_alloc);\n \tco = &cached_objects[cached_object_nr++];\ndiff --git a/t/t8002-blame.sh b/t/t8002-blame.sh\nindex eea048e52c..2ed6aaae35 100755\n--- a/t/t8002-blame.sh\n+++ b/t/t8002-blame.sh\n@@ -122,4 +122,15 @@ test_expect_success '--exclude-promisor-objects does not BUG-crash' '\n \ttest_must_fail git blame --exclude-promisor-objects one\n '\n \n+test_expect_success 'blame with uncommitted edits in partial clone does not crash' '\n+\tgit init server &&\n+\techo foo >server/file.txt &&\n+\tgit -C server add file.txt &&\n+\tgit -C server commit -m file &&\n+\n+\tgit clone --filter=blob:none \"file://$(pwd)/server\" client &&\n+\techo bar >>client/file.txt &&\n+\tgit -C client blame file.txt\n+'\n+\n test_done\n-- \n2.28.0.rc0.105.gf9edc3c819-goog\n\n"},{"id":"401832","messageId":"xmqq8sfc78og.fsf@gitster.c.googlers.com","threadId":"53897","inReplyTo":"20200721225020.1352772-1-jonathantanmy@google.com","subject":"Re: [PATCH] sha1-file: make pretend_object_file() not prefetch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-21T23:25:03Z","receivedAt":"2020-07-21T23:25:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> To make progress towards solving this, I'm thinking of making a new\n> function has_object() that takes a repository, an object ID, and flags,\n> and only supports one flag: HAS_OBJECT_RECHECK_PACKED (which has the\n> opposite meaning of OBJECT_INFO_QUICK). Checks that should not fetch\n> should use has_object(), and checks that should fetch (if they exist - I\n> think that most would want additional information about the object, so\n> they would use oid_object_info() or similar already) should use\n> oid_object_info() or a similar function. That way we can see how many\n> uses of has_object_file() we have checked, and at the same time make\n> behavior we usually want into the default behavior. Any opinions?\n\nYes, a new function (or a pair of functions) makes it easy to see\nhow much progress we made and also gives us easier greppability.\n\n"},{"id":"401833","messageId":"xmqq4kq078ks.fsf@gitster.c.googlers.com","threadId":"53897","inReplyTo":"20200721225020.1352772-1-jonathantanmy@google.com","subject":"Re: [PATCH] sha1-file: make pretend_object_file() not prefetch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-21T23:27:15Z","receivedAt":"2020-07-21T23:27:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> When pretend_object_file() is invoked with an object that does not exist\n> (as is the typical case), there is no need to fetch anything from the\n> promisor remote, because the caller already knows what the object is\n> supposed to contain. Therefore, suppress the fetch. (The\n> OBJECT_INFO_QUICK flag is added for the same reason.)\n\nYes, \"pretend\" is also a way to lie about the contents IIRC, so even\nif the object is available elsewhere, we should *not* fetch from the\npromisor.  Makes sense to me.\n"},{"id":"401930","messageId":"20200723174747.GA3975154@coredump.intra.peff.net","threadId":"53897","inReplyTo":"xmqq4kq078ks.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] sha1-file: make pretend_object_file() not prefetch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-07-23T17:47:47Z","receivedAt":"2020-07-23T17:47:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 21, 2020 at 04:27:15PM -0700, Junio C Hamano wrote:\n\n> Jonathan Tan <jonathantanmy@google.com> writes:\n> \n> > When pretend_object_file() is invoked with an object that does not exist\n> > (as is the typical case), there is no need to fetch anything from the\n> > promisor remote, because the caller already knows what the object is\n> > supposed to contain. Therefore, suppress the fetch. (The\n> > OBJECT_INFO_QUICK flag is added for the same reason.)\n> \n> Yes, \"pretend\" is also a way to lie about the contents IIRC, so even\n> if the object is available elsewhere, we should *not* fetch from the\n> promisor.  Makes sense to me.\n\nI agree this patch is fine, but I wonder if it could go even further. If\nwe are pretending some particular contents, shouldn't we override\nanything that might be in the object database? I.e., could we eliminate\nthis has_object_file() entirely?\n\nThat should be OK for the same reason that it's OK to use QUICK.\n\nThere's only one caller of this function (git-blame), which I think\nwould be happy with such a change.\n\n-Peff\n"},{"id":"401950","messageId":"xmqq5zaertrd.fsf@gitster.c.googlers.com","threadId":"53897","inReplyTo":"20200723174747.GA3975154@coredump.intra.peff.net","subject":"Re: [PATCH] sha1-file: make pretend_object_file() not prefetch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-23T18:06:14Z","receivedAt":"2020-07-23T18:06:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I agree this patch is fine, but I wonder if it could go even further. If\n> we are pretending some particular contents, shouldn't we override\n> anything that might be in the object database? I.e., could we eliminate\n> this has_object_file() entirely?\n>\n> That should be OK for the same reason that it's OK to use QUICK.\n>\n> There's only one caller of this function (git-blame), which I think\n> would be happy with such a change.\n\nI actually have to take that \"we could even lie to say that content\nthat does not hash to X is object X\" back---that was never the\nintention of this mechanism.\n\nIt was to ensure that operations that are supposedly read-only can\navoid writing into the repository---\"blame\" uses it to pretend as if\nthe working tree file already has a corresponding object in the\nobject store, IIRC.\n\nThe only reason why hash_object_file() is used here is to allow us\ndiscarding the memory held for that working tree copy if it happens\nto match what is stored in the object database.  The saving would be\nwithin a few hundred kilobytes to a single digit megabyte range at\nmost, hopefully, so we could drop it (oh, saying \"a single digit\nmegabyte\" reminds me that my first Linux computer was 486dx with\n4MB ram---on that box, it may have mattered).\n\n"}]}