{"thread":{"id":"52542","subject":"[PATCH] sha1-file: remove OBJECT_INFO_SKIP_CACHED","startedAt":"2019-12-30T21:10:34Z","lastAt":"2020-01-07T11:22:15Z","messageCount":13,"participants":["Jonathan Tan","Junio C Hamano","Jonathan Nieder","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"389104","messageId":"20191230211027.37002-1-jonathantanmy@google.com","threadId":"52542","inReplyTo":null,"subject":"[PATCH] sha1-file: remove OBJECT_INFO_SKIP_CACHED","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-12-30T21:10:27Z","receivedAt":"2019-12-30T21:10:34Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"In a partial clone, if a user provides the hash of the empty tree (\"git\nmktree </dev/null\" - for SHA-1, this is 4b825d...) to a command which\nrequires that that object be parsed, for example:\n\n  git diff-tree 4b825d <a non-empty tree>\n\nthen Git will lazily fetch the empty tree. This fetch would merely be\ninconvenient if the promisor remote could supply that tree, but at\n$DAYJOB we discovered that some repositories do not (e.g. [1]).\n\nThere are 2 functions: repo_has_object_file() which does not consult\nfind_cached_object() (which, among other things, knows about the empty\ntree); and repo_read_object_file() which does. This issue occurs\nbecause, as an optimization to avoid reading blobs into memory,\nparse_object() calls repo_has_object_file() before\nrepo_read_object_file(). In the case of a regular repository (that is,\nnot a partial clone), repo_has_object_file() will return false for the\nempty tree (thus bypassing the optimization) and repo_read_object_file()\nwill nevertheless succeed, thus things coincidentally work. But in a\npartial clone, repo_has_object_file() triggers a lazy fetch of the\nmissing empty tree. This optimization was introduced by 090ea12671\n(\"parse_object: avoid putting whole blob in core\", 2012-03-07), and the\nempty-tree special-case dichotomy between repo_has_object_file() (then,\nhas_sha1_file()) and repo_read_object_file() (then, sha1_object_info())\nhas existed since then.\n\n(The flag OBJECT_INFO_SKIP_CACHED, introduced later in dfdd4afcf9\n(\"sha1_file: teach sha1_object_info_extended more flags\", 2017-06-26)\nand used in e83e71c5e1 (\"sha1_file: refactor has_sha1_file_with_flags\",\n2017-06-26), was introduced to preserve the empty-tree special-case\ndichotomy.)\n\nThe best solution to the problem introduced at the start of this commit\nmessage seems to be to eliminate this dichotomy. Not only will this fix\nthe problem, but it reduces a potential avenue of surprise for future\ndevelopers of Git - it will no longer be the case that\nrepo_has_object_file() doesn't know about empty trees, but\nrepo_read_object_file() does. A cost is that repo_has_object_file() will\nnow need to oideq upon each invocation, but that is trivial compared to\nthe filesystem lookup or the pack index search required anyway. (And if\nfind_cached_object() needs to do more because of previous invocations to\npretend_object_file(), all the more reason to be consistent in whether\nwe present cached objects.) Therefore, remove OBJECT_INFO_SKIP_CACHED.\n\n[1] https://dart.googlesource.com/json_rpc_2\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\nI put a lot of historical references which makes the commit message\nrather long - let me know if you think that some can be omitted.\n---\n object-store.h |  2 --\n sha1-file.c    | 38 ++++++++++++++++++--------------------\n 2 files changed, 18 insertions(+), 22 deletions(-)\n\ndiff --git a/object-store.h b/object-store.h\nindex 55ee639350..61b8b13e3b 100644\n--- a/object-store.h\n+++ b/object-store.h\n@@ -292,8 +292,6 @@ struct object_info {\n #define OBJECT_INFO_LOOKUP_REPLACE 1\n /* Allow reading from a loose object file of unknown/bogus type */\n #define OBJECT_INFO_ALLOW_UNKNOWN_TYPE 2\n-/* Do not check cached storage */\n-#define OBJECT_INFO_SKIP_CACHED 4\n /* Do not retry packed storage after checking packed and loose storage */\n #define OBJECT_INFO_QUICK 8\n /* Do not check loose object */\ndiff --git a/sha1-file.c b/sha1-file.c\nindex 188de57634..03ae9ae93a 100644\n--- a/sha1-file.c\n+++ b/sha1-file.c\n@@ -1417,6 +1417,7 @@ int oid_object_info_extended(struct repository *r, const struct object_id *oid,\n \t\t\t     struct object_info *oi, unsigned flags)\n {\n \tstatic struct object_info blank_oi = OBJECT_INFO_INIT;\n+\tstruct cached_object *co;\n \tstruct pack_entry e;\n \tint rtype;\n \tconst struct object_id *real = oid;\n@@ -1431,24 +1432,22 @@ int oid_object_info_extended(struct repository *r, const struct object_id *oid,\n \tif (!oi)\n \t\toi = &blank_oi;\n \n-\tif (!(flags & OBJECT_INFO_SKIP_CACHED)) {\n-\t\tstruct cached_object *co = find_cached_object(real);\n-\t\tif (co) {\n-\t\t\tif (oi->typep)\n-\t\t\t\t*(oi->typep) = co->type;\n-\t\t\tif (oi->sizep)\n-\t\t\t\t*(oi->sizep) = co->size;\n-\t\t\tif (oi->disk_sizep)\n-\t\t\t\t*(oi->disk_sizep) = 0;\n-\t\t\tif (oi->delta_base_sha1)\n-\t\t\t\thashclr(oi->delta_base_sha1);\n-\t\t\tif (oi->type_name)\n-\t\t\t\tstrbuf_addstr(oi->type_name, type_name(co->type));\n-\t\t\tif (oi->contentp)\n-\t\t\t\t*oi->contentp = xmemdupz(co->buf, co->size);\n-\t\t\toi->whence = OI_CACHED;\n-\t\t\treturn 0;\n-\t\t}\n+\tco = find_cached_object(real);\n+\tif (co) {\n+\t\tif (oi->typep)\n+\t\t\t*(oi->typep) = co->type;\n+\t\tif (oi->sizep)\n+\t\t\t*(oi->sizep) = co->size;\n+\t\tif (oi->disk_sizep)\n+\t\t\t*(oi->disk_sizep) = 0;\n+\t\tif (oi->delta_base_sha1)\n+\t\t\thashclr(oi->delta_base_sha1);\n+\t\tif (oi->type_name)\n+\t\t\tstrbuf_addstr(oi->type_name, type_name(co->type));\n+\t\tif (oi->contentp)\n+\t\t\t*oi->contentp = xmemdupz(co->buf, co->size);\n+\t\toi->whence = OI_CACHED;\n+\t\treturn 0;\n \t}\n \n \twhile (1) {\n@@ -1932,8 +1931,7 @@ int repo_has_object_file_with_flags(struct repository *r,\n {\n \tif (!startup_info->have_repository)\n \t\treturn 0;\n-\treturn oid_object_info_extended(r, oid, NULL,\n-\t\t\t\t\tflags | OBJECT_INFO_SKIP_CACHED) >= 0;\n+\treturn oid_object_info_extended(r, oid, NULL, flags) >= 0;\n }\n \n int repo_has_object_file(struct repository *r,\n-- \n2.24.1.735.g03f4e72817-goog\n\n"},{"id":"389106","messageId":"xmqqsgl1fq03.fsf@gitster-ct.c.googlers.com","threadId":"52542","inReplyTo":"20191230211027.37002-1-jonathantanmy@google.com","subject":"Re: [PATCH] sha1-file: remove OBJECT_INFO_SKIP_CACHED","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-30T21:43:56Z","receivedAt":"2019-12-30T21:44:04Z","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> In a partial clone, if a user provides the hash of the empty tree (\"git\n> mktree </dev/null\" - for SHA-1, this is 4b825d...) to a command which\n> requires that that object be parsed, for example:\n>\n>   git diff-tree 4b825d <a non-empty tree>\n>\n> then Git will lazily fetch the empty tree.\n\nThat sounds like a bug.  Shouldn't some objects like empty tree and\nempty blob whose names are hardcoded come internally without being\nfetched from anywhere?\n\n> There are 2 functions: repo_has_object_file() which does not consult\n> find_cached_object() (which, among other things, knows about the empty\n> tree); and repo_read_object_file() which does. This issue occurs\n> because, as an optimization to avoid reading blobs into memory,\n> parse_object() calls repo_has_object_file() before\n> repo_read_object_file(). In the case of a regular repository (that is,\n> not a partial clone), repo_has_object_file() will return false for the\n> empty tree (thus bypassing the optimization) and...\n\nOK, that bypassing already sounds wrong.  \n\n> ... it will no longer be the case that\n> repo_has_object_file() doesn't know about empty trees, but\n> repo_read_object_file() does.\n\nYup, that sounds like the right solution to the bug, with or without\nlazy cloning.\n"},{"id":"389110","messageId":"20191230220155.GF57251@google.com","threadId":"52542","inReplyTo":"20191230211027.37002-1-jonathantanmy@google.com","subject":"Re: [PATCH] sha1-file: remove OBJECT_INFO_SKIP_CACHED","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2019-12-30T22:01:55Z","receivedAt":"2019-12-30T22:02:01Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJonathan Tan wrote:\n\n> In a partial clone, if a user provides the hash of the empty tree (\"git\n> mktree </dev/null\" - for SHA-1, this is 4b825d...) to a command which\n> requires that that object be parsed, for example:\n>\n>   git diff-tree 4b825d <a non-empty tree>\n>\n> then Git will lazily fetch the empty tree. This fetch would merely be\n> inconvenient if the promisor remote could supply that tree, but at\n> $DAYJOB we discovered that some repositories do not (e.g. [1]).\n\nOoh, I think there's something subtle hiding in this paragraph.\n\nWhen I first read it, I thought you meant that the repositories are\nnot self-contained --- that they contain references to the empty tree\nbut do not fulfill \"want\"s for them.  I don't believe that's what you\nmean, though.\n\nMy second reading is the repository genuinely doesn't contain the\nempty tree but different Git server implementations handle that\ndifferently.  I tried to reproduce this with\n\n\tempty_tree=$(git mktree </dev/null)\n\tgit init --bare x\n\tgit clone --filter=blob:none file://$(pwd)/x y\n\tcd y\n\techo hi >README\n\tgit add README\n\tgit commit -m 'nonempty tree'\n\tGIT_TRACE=1 git diff-tree \"$empty_tree\" HEAD\n\nand indeed, it looks like Git serves the empty tree even from\nrepositories that don't contain it.  By comparison, JGit does not do\nthe same trick.  So we don't need to refer to a specific repository in\nthe wild to reproduce this.\n\nAll that said, having to fetch this object in the first place is\nunexpected.  The question of the promisor remote serving it is only\nrelevant from the point of view of \"why didn't we discover this\nsooner?\"\n\n> There are 2 functions: repo_has_object_file() which does not consult\n> find_cached_object() (which, among other things, knows about the empty\n> tree); and repo_read_object_file() which does.\n\nHm, where does this dichotomy come from?  E.g. is the latter a\nlower-level function used by the former?\n\n>                                                This issue occurs\n> because,\n\nnit: on first reading I had trouble figuring out what \"this issue\"\nrefers to here.\n\n[...]\n>          as an optimization to avoid reading blobs into memory,\n> parse_object() calls repo_has_object_file() before\n> repo_read_object_file(). In the case of a regular repository (that is,\n> not a partial clone), repo_has_object_file() will return false for the\n> empty tree (thus bypassing the optimization) and repo_read_object_file()\n> will nevertheless succeed, thus things coincidentally work.\n\nThis might be easier to understand if phrased in terms of the\nintention behind the code instead of the specific call stacks used.\nSee f06ab027 for an example of this kind of thing.  For example:\n\n  Applications within and outside of Git benefit from being able to\n  assume that every repository contains the empty tree as an object\n  (see, for example, commit 9abd46a347 \"Make sure the empty tree\n  exists when needed in merge-recursive\", 2006-12-07).  To this end,\n  since 346245a1bb (hard-code the empty tree object, 2008-02-13), Git\n  has made the empty tree available in all repositories via\n  find_cached_object, which all object access paths can use.\n\n  Object existence checks (has_object_file), however, do not use\n  find_cached_object.  <describe reason here>\n\n>                                                             But in a\n> partial clone, repo_has_object_file() triggers a lazy fetch of the\n> missing empty tree.\n\n  This particularly affects partial clones: has_object_file does not\n  only report false in this case but contacts the promisor remote in\n  order to obtain that answer.  The cost of these repeated negative\n  lookups can add up.\n\n  For example, in an optimization introduced in 090ea12671\n  (\"parse_object: avoid putting whole blob in core\", 2012-03-07),\n  object parsing uses has_object_file before read_object_file to check\n  for a fast-path, so this negative lookup is triggered whenever we\n  try to parse the absent empty tree.\n\nWhen I state it this way, it's not obvious why this particular caller\nof has_object_file is more relevant than others.  Did I miss some\nsubtlety?\n\n[...]\n>                                            This fetch would merely be\n> inconvenient if the promisor remote could supply that tree, but at\n> $DAYJOB we discovered that some repositories do not (e.g. [1]).\n\n  If the promisor remote is running standard Git then it *does* have a\n  copy of the empty tree, via the cached object itself.  This\n  guarantee is not a documented part of the protocol, however, and\n  other Git servers do not implement it.\n\n> The best solution to the problem introduced at the start of this commit\n> message seems to be to eliminate this dichotomy.\n\nIndeed.  Can we justify the change even more simply in those terms?\n\n  Object existence checks (has_object_file), however, do not use\n  find_cached_object.  <describe reason here>\n\n  This makes the API unnecessarily difficult to reason about.\n  Instead, let's consistently view the empty tree as a genuine part of\n  the repository, even in has_object_file.  As a side effect, this\n  allows us to simplify the common 'has_object_file ||\n  find_cached_object' pattern to a more simple existence check.\n\n[...]\n>                               A cost is that repo_has_object_file() will\n> now need to oideq upon each invocation, but that is trivial compared to\n> the filesystem lookup or the pack index search required anyway. (And if\n> find_cached_object() needs to do more because of previous invocations to\n> pretend_object_file(), all the more reason to be consistent in whether\n> we present cached objects.) Therefore, remove OBJECT_INFO_SKIP_CACHED.\n\nThanks for discussing the possible costs, and I agree that they're\ntrivial relative to the I/O that these functions already incur.\n\n[...]\n>  object-store.h |  2 --\n>  sha1-file.c    | 38 ++++++++++++++++++--------------------\n>  2 files changed, 18 insertions(+), 22 deletions(-)\n\nAs hinted above, we should be able to simplify away has_sha1_file ||\nfind_cached_object checks in this change.\n\nThanks and hpoe that helps,\nJonathan\n"},{"id":"389122","messageId":"20191231003903.36486-1-jonathantanmy@google.com","threadId":"52542","inReplyTo":"20191230220155.GF57251@google.com","subject":"Re: [PATCH] sha1-file: remove OBJECT_INFO_SKIP_CACHED","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-12-31T00:39:03Z","receivedAt":"2019-12-31T00:39:11Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Ooh, I think there's something subtle hiding in this paragraph.\n> \n> When I first read it, I thought you meant that the repositories are\n> not self-contained --- that they contain references to the empty tree\n> but do not fulfill \"want\"s for them.  I don't believe that's what you\n> mean, though.\n> \n> My second reading is the repository genuinely doesn't contain the\n> empty tree but different Git server implementations handle that\n> differently.  I tried to reproduce this with\n> \n> \tempty_tree=$(git mktree </dev/null)\n> \tgit init --bare x\n> \tgit clone --filter=blob:none file://$(pwd)/x y\n> \tcd y\n> \techo hi >README\n> \tgit add README\n> \tgit commit -m 'nonempty tree'\n> \tGIT_TRACE=1 git diff-tree \"$empty_tree\" HEAD\n> \n> and indeed, it looks like Git serves the empty tree even from\n> repositories that don't contain it.  By comparison, JGit does not do\n> the same trick.  So we don't need to refer to a specific repository in\n> the wild to reproduce this.\n> \n> All that said, having to fetch this object in the first place is\n> unexpected.  The question of the promisor remote serving it is only\n> relevant from the point of view of \"why didn't we discover this\n> sooner?\"\n\nYes, that's true. I'll reword it to emphasize the spurious fetching (and\nmention some implementations like JGit not serving those, which\nexacerbates the problem).\n\nI think we didn't discover this sooner because not many people directly\nenter the empty tree on the command line :-) (but this is a problem that\nwe should solve, most certainly).\n\n> > There are 2 functions: repo_has_object_file() which does not consult\n> > find_cached_object() (which, among other things, knows about the empty\n> > tree); and repo_read_object_file() which does.\n> \n> Hm, where does this dichotomy come from?  E.g. is the latter a\n> lower-level function used by the former?\n\nI don't know the reason for the dichotomy - perhaps it was an oversight.\nThe relevant commits are:\n\nd66b37bb19 (\"Add pretend_sha1_file() interface.\", 2007-02-05)\nAdds a way to pretend that some objects are present by including them in\na cache - empty-tree-pervasiveness is built on top of this later. Only\nread_sha1_file() was updated to make use of this; has_sha1_file() and\nsha1_object_info() were already present at this time, but were not\nupdated. (Maybe the latter 2 had no need for pretending?)\n\n346245a1bb (\"hard-code the empty tree object\", 2008-02-13)\nEmpty tree pervasiveness built on top of the cache.\n\nc4d9986f5f (\"sha1_object_info: examine cached_object store too\",\n2011-02-07)\nsha1_object_info() was updated to use the cache. has_sha1_file() is\nnever mentioned.\n\n> > as an optimization to avoid reading blobs into memory,\n> > parse_object() calls repo_has_object_file() before\n> > repo_read_object_file(). In the case of a regular repository (that is,\n> > not a partial clone), repo_has_object_file() will return false for the\n> > empty tree (thus bypassing the optimization) and repo_read_object_file()\n> > will nevertheless succeed, thus things coincidentally work.\n> \n> This might be easier to understand if phrased in terms of the\n> intention behind the code instead of the specific call stacks used.\n> See f06ab027 for an example of this kind of thing.  For example:\n> \n>   Applications within and outside of Git benefit from being able to\n>   assume that every repository contains the empty tree as an object\n>   (see, for example, commit 9abd46a347 \"Make sure the empty tree\n>   exists when needed in merge-recursive\", 2006-12-07).  To this end,\n>   since 346245a1bb (hard-code the empty tree object, 2008-02-13), Git\n>   has made the empty tree available in all repositories via\n>   find_cached_object, which all object access paths can use.\n> \n>   Object existence checks (has_object_file), however, do not use\n>   find_cached_object.  <describe reason here>\n> \n> When I state it this way, it's not obvious why this particular caller\n> of has_object_file is more relevant than others.  Did I miss some\n> subtlety?\n\nThis particular caller is what caused us to notice this problem.\n\n> Indeed.  Can we justify the change even more simply in those terms?\n> \n>   Object existence checks (has_object_file), however, do not use\n>   find_cached_object.  <describe reason here>\n> \n>   This makes the API unnecessarily difficult to reason about.\n>   Instead, let's consistently view the empty tree as a genuine part of\n>   the repository, even in has_object_file.  As a side effect, this\n>   allows us to simplify the common 'has_object_file ||\n>   find_cached_object' pattern to a more simple existence check.\n\nOK, let me see if I can rewrite it to emphasize the reasoning-about-API\npart. I still want to include the user-facing part that caused us to\nobserve this problem, but I can deemphasize it.\n\nThis might be a moot point, but what do you mean by the\n\"'has_object_file || find_cached_object' pattern\"?\n"},{"id":"389123","messageId":"20191231010303.GD13606@google.com","threadId":"52542","inReplyTo":"20191231003903.36486-1-jonathantanmy@google.com","subject":"Re: [PATCH] sha1-file: remove OBJECT_INFO_SKIP_CACHED","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2019-12-31T01:03:03Z","receivedAt":"2019-12-31T01:03:09Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Tan wrote:\n> Jonathan Nieder wrote:\n\n>> Hm, where does this dichotomy come from?  E.g. is the latter a\n>> lower-level function used by the former?\n>\n> I don't know the reason for the dichotomy - perhaps it was an oversight.\n\nAh, that makes sense.\n\n[...]\n> This might be a moot point, but what do you mean by the\n> \"'has_object_file || find_cached_object' pattern\"?\n\nOh!  Thanks, I should have been more precise.  And calling it a pattern\nwas probably overreaching --- I can only find one instance, in\npretend_object_file:\n\n\tif (has_object_file(oid) || find_cached_object(oid))\n\t\treturn 0;\n\nSince there's only one, I was on the wrong track.  (Except that with\nthis change, that can indeed change to\n\n\tif (has_object_file(oid))\n\t\treturn 0;\n\nThanks,\nJonathan\n"},{"id":"389174","messageId":"20200102201535.179417-1-jonathantanmy@google.com","threadId":"52542","inReplyTo":"20191230211027.37002-1-jonathantanmy@google.com","subject":"[PATCH] sha1-file: remove OBJECT_INFO_SKIP_CACHED","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-01-02T20:15:35Z","receivedAt":"2020-01-02T20:15:41Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"In a partial clone, if a user provides the hash of the empty tree (\"git\nmktree </dev/null\" - for SHA-1, this is 4b825d...) to a command which\nrequires that that object be parsed, for example:\n\n  git diff-tree 4b825d <a non-empty tree>\n\nthen Git will lazily fetch the empty tree, unnecessarily, because\nparsing of that object invokes repo_has_object_file(), which does not\nspecial-case the empty tree.\n\nInstead, teach repo_has_object_file() to consult find_cached_object()\n(which handles the empty tree), thus bringing it in line with the rest\nof the object-store-accessing functions. A cost is that\nrepo_has_object_file() will now need to oideq upon each invocation, but\nthat is trivial compared to the filesystem lookup or the pack index\nsearch required anyway. (And if find_cached_object() needs to do more\nbecause of previous invocations to pretend_object_file(), all the more\nreason to be consistent in whether we present cached objects.)\n\nAs a historical note, the function now known as repo_read_object_file()\nwas taught the empty tree in 346245a1bb (\"hard-code the empty tree\nobject\", 2008-02-13), and the function now known as oid_object_info()\nwas taught the empty tree in c4d9986f5f (\"sha1_object_info: examine\ncached_object store too\", 2011-02-07). repo_has_object_file() was never\nupdated, perhaps due to oversight. The flag OBJECT_INFO_SKIP_CACHED,\nintroduced later in dfdd4afcf9 (\"sha1_file: teach\nsha1_object_info_extended more flags\", 2017-06-26) and used in\ne83e71c5e1 (\"sha1_file: refactor has_sha1_file_with_flags\", 2017-06-26),\nwas introduced to preserve this difference in empty-tree handling, but\nnow it can be removed.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\nDifference from v1: updated commit message in response to Jonathan\nNieder's feedback. Hopefully I didn't remove too much.\n---\n object-store.h |  2 --\n sha1-file.c    | 38 ++++++++++++++++++--------------------\n 2 files changed, 18 insertions(+), 22 deletions(-)\n\ndiff --git a/object-store.h b/object-store.h\nindex 55ee639350..61b8b13e3b 100644\n--- a/object-store.h\n+++ b/object-store.h\n@@ -292,8 +292,6 @@ struct object_info {\n #define OBJECT_INFO_LOOKUP_REPLACE 1\n /* Allow reading from a loose object file of unknown/bogus type */\n #define OBJECT_INFO_ALLOW_UNKNOWN_TYPE 2\n-/* Do not check cached storage */\n-#define OBJECT_INFO_SKIP_CACHED 4\n /* Do not retry packed storage after checking packed and loose storage */\n #define OBJECT_INFO_QUICK 8\n /* Do not check loose object */\ndiff --git a/sha1-file.c b/sha1-file.c\nindex 188de57634..03ae9ae93a 100644\n--- a/sha1-file.c\n+++ b/sha1-file.c\n@@ -1417,6 +1417,7 @@ int oid_object_info_extended(struct repository *r, const struct object_id *oid,\n \t\t\t     struct object_info *oi, unsigned flags)\n {\n \tstatic struct object_info blank_oi = OBJECT_INFO_INIT;\n+\tstruct cached_object *co;\n \tstruct pack_entry e;\n \tint rtype;\n \tconst struct object_id *real = oid;\n@@ -1431,24 +1432,22 @@ int oid_object_info_extended(struct repository *r, const struct object_id *oid,\n \tif (!oi)\n \t\toi = &blank_oi;\n \n-\tif (!(flags & OBJECT_INFO_SKIP_CACHED)) {\n-\t\tstruct cached_object *co = find_cached_object(real);\n-\t\tif (co) {\n-\t\t\tif (oi->typep)\n-\t\t\t\t*(oi->typep) = co->type;\n-\t\t\tif (oi->sizep)\n-\t\t\t\t*(oi->sizep) = co->size;\n-\t\t\tif (oi->disk_sizep)\n-\t\t\t\t*(oi->disk_sizep) = 0;\n-\t\t\tif (oi->delta_base_sha1)\n-\t\t\t\thashclr(oi->delta_base_sha1);\n-\t\t\tif (oi->type_name)\n-\t\t\t\tstrbuf_addstr(oi->type_name, type_name(co->type));\n-\t\t\tif (oi->contentp)\n-\t\t\t\t*oi->contentp = xmemdupz(co->buf, co->size);\n-\t\t\toi->whence = OI_CACHED;\n-\t\t\treturn 0;\n-\t\t}\n+\tco = find_cached_object(real);\n+\tif (co) {\n+\t\tif (oi->typep)\n+\t\t\t*(oi->typep) = co->type;\n+\t\tif (oi->sizep)\n+\t\t\t*(oi->sizep) = co->size;\n+\t\tif (oi->disk_sizep)\n+\t\t\t*(oi->disk_sizep) = 0;\n+\t\tif (oi->delta_base_sha1)\n+\t\t\thashclr(oi->delta_base_sha1);\n+\t\tif (oi->type_name)\n+\t\t\tstrbuf_addstr(oi->type_name, type_name(co->type));\n+\t\tif (oi->contentp)\n+\t\t\t*oi->contentp = xmemdupz(co->buf, co->size);\n+\t\toi->whence = OI_CACHED;\n+\t\treturn 0;\n \t}\n \n \twhile (1) {\n@@ -1932,8 +1931,7 @@ int repo_has_object_file_with_flags(struct repository *r,\n {\n \tif (!startup_info->have_repository)\n \t\treturn 0;\n-\treturn oid_object_info_extended(r, oid, NULL,\n-\t\t\t\t\tflags | OBJECT_INFO_SKIP_CACHED) >= 0;\n+\treturn oid_object_info_extended(r, oid, NULL, flags) >= 0;\n }\n \n int repo_has_object_file(struct repository *r,\n-- \n2.24.1.735.g03f4e72817-goog\n\n"},{"id":"389175","messageId":"20200102201630.180969-1-jonathantanmy@google.com","threadId":"52542","inReplyTo":"20191230211027.37002-1-jonathantanmy@google.com","subject":"[PATCH v2] sha1-file: remove OBJECT_INFO_SKIP_CACHED","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-01-02T20:16:30Z","receivedAt":"2020-01-02T20:16:36Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"In a partial clone, if a user provides the hash of the empty tree (\"git\nmktree </dev/null\" - for SHA-1, this is 4b825d...) to a command which\nrequires that that object be parsed, for example:\n\n  git diff-tree 4b825d <a non-empty tree>\n\nthen Git will lazily fetch the empty tree, unnecessarily, because\nparsing of that object invokes repo_has_object_file(), which does not\nspecial-case the empty tree.\n\nInstead, teach repo_has_object_file() to consult find_cached_object()\n(which handles the empty tree), thus bringing it in line with the rest\nof the object-store-accessing functions. A cost is that\nrepo_has_object_file() will now need to oideq upon each invocation, but\nthat is trivial compared to the filesystem lookup or the pack index\nsearch required anyway. (And if find_cached_object() needs to do more\nbecause of previous invocations to pretend_object_file(), all the more\nreason to be consistent in whether we present cached objects.)\n\nAs a historical note, the function now known as repo_read_object_file()\nwas taught the empty tree in 346245a1bb (\"hard-code the empty tree\nobject\", 2008-02-13), and the function now known as oid_object_info()\nwas taught the empty tree in c4d9986f5f (\"sha1_object_info: examine\ncached_object store too\", 2011-02-07). repo_has_object_file() was never\nupdated, perhaps due to oversight. The flag OBJECT_INFO_SKIP_CACHED,\nintroduced later in dfdd4afcf9 (\"sha1_file: teach\nsha1_object_info_extended more flags\", 2017-06-26) and used in\ne83e71c5e1 (\"sha1_file: refactor has_sha1_file_with_flags\", 2017-06-26),\nwas introduced to preserve this difference in empty-tree handling, but\nnow it can be removed.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\nForgot to add v2 to the other email, so resending it with the correct\nemail subject.\n\nDifference from v1: updated commit message in response to Jonathan\nNieder's feedback. Hopefully I didn't remove too much.\n---\n object-store.h |  2 --\n sha1-file.c    | 38 ++++++++++++++++++--------------------\n 2 files changed, 18 insertions(+), 22 deletions(-)\n\ndiff --git a/object-store.h b/object-store.h\nindex 55ee639350..61b8b13e3b 100644\n--- a/object-store.h\n+++ b/object-store.h\n@@ -292,8 +292,6 @@ struct object_info {\n #define OBJECT_INFO_LOOKUP_REPLACE 1\n /* Allow reading from a loose object file of unknown/bogus type */\n #define OBJECT_INFO_ALLOW_UNKNOWN_TYPE 2\n-/* Do not check cached storage */\n-#define OBJECT_INFO_SKIP_CACHED 4\n /* Do not retry packed storage after checking packed and loose storage */\n #define OBJECT_INFO_QUICK 8\n /* Do not check loose object */\ndiff --git a/sha1-file.c b/sha1-file.c\nindex 188de57634..03ae9ae93a 100644\n--- a/sha1-file.c\n+++ b/sha1-file.c\n@@ -1417,6 +1417,7 @@ int oid_object_info_extended(struct repository *r, const struct object_id *oid,\n \t\t\t     struct object_info *oi, unsigned flags)\n {\n \tstatic struct object_info blank_oi = OBJECT_INFO_INIT;\n+\tstruct cached_object *co;\n \tstruct pack_entry e;\n \tint rtype;\n \tconst struct object_id *real = oid;\n@@ -1431,24 +1432,22 @@ int oid_object_info_extended(struct repository *r, const struct object_id *oid,\n \tif (!oi)\n \t\toi = &blank_oi;\n \n-\tif (!(flags & OBJECT_INFO_SKIP_CACHED)) {\n-\t\tstruct cached_object *co = find_cached_object(real);\n-\t\tif (co) {\n-\t\t\tif (oi->typep)\n-\t\t\t\t*(oi->typep) = co->type;\n-\t\t\tif (oi->sizep)\n-\t\t\t\t*(oi->sizep) = co->size;\n-\t\t\tif (oi->disk_sizep)\n-\t\t\t\t*(oi->disk_sizep) = 0;\n-\t\t\tif (oi->delta_base_sha1)\n-\t\t\t\thashclr(oi->delta_base_sha1);\n-\t\t\tif (oi->type_name)\n-\t\t\t\tstrbuf_addstr(oi->type_name, type_name(co->type));\n-\t\t\tif (oi->contentp)\n-\t\t\t\t*oi->contentp = xmemdupz(co->buf, co->size);\n-\t\t\toi->whence = OI_CACHED;\n-\t\t\treturn 0;\n-\t\t}\n+\tco = find_cached_object(real);\n+\tif (co) {\n+\t\tif (oi->typep)\n+\t\t\t*(oi->typep) = co->type;\n+\t\tif (oi->sizep)\n+\t\t\t*(oi->sizep) = co->size;\n+\t\tif (oi->disk_sizep)\n+\t\t\t*(oi->disk_sizep) = 0;\n+\t\tif (oi->delta_base_sha1)\n+\t\t\thashclr(oi->delta_base_sha1);\n+\t\tif (oi->type_name)\n+\t\t\tstrbuf_addstr(oi->type_name, type_name(co->type));\n+\t\tif (oi->contentp)\n+\t\t\t*oi->contentp = xmemdupz(co->buf, co->size);\n+\t\toi->whence = OI_CACHED;\n+\t\treturn 0;\n \t}\n \n \twhile (1) {\n@@ -1932,8 +1931,7 @@ int repo_has_object_file_with_flags(struct repository *r,\n {\n \tif (!startup_info->have_repository)\n \t\treturn 0;\n-\treturn oid_object_info_extended(r, oid, NULL,\n-\t\t\t\t\tflags | OBJECT_INFO_SKIP_CACHED) >= 0;\n+\treturn oid_object_info_extended(r, oid, NULL, flags) >= 0;\n }\n \n int repo_has_object_file(struct repository *r,\n-- \n2.24.1.735.g03f4e72817-goog\n\n"},{"id":"389180","messageId":"xmqqftgxedtk.fsf@gitster-ct.c.googlers.com","threadId":"52542","inReplyTo":"20200102201630.180969-1-jonathantanmy@google.com","subject":"Re: [PATCH v2] sha1-file: remove OBJECT_INFO_SKIP_CACHED","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-02T21:41:27Z","receivedAt":"2020-01-02T21:41:33Z","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> As a historical note, the function now known as repo_read_object_file()\n> was taught the empty tree in 346245a1bb (\"hard-code the empty tree\n> object\", 2008-02-13), and the function now known as oid_object_info()\n> was taught the empty tree in c4d9986f5f (\"sha1_object_info: examine\n> cached_object store too\", 2011-02-07). repo_has_object_file() was never\n> updated, perhaps due to oversight. The flag OBJECT_INFO_SKIP_CACHED,\n> introduced later in dfdd4afcf9 (\"sha1_file: teach\n> sha1_object_info_extended more flags\", 2017-06-26) and used in\n> e83e71c5e1 (\"sha1_file: refactor has_sha1_file_with_flags\", 2017-06-26),\n> was introduced to preserve this difference in empty-tree handling, but\n> now it can be removed.\n\nI am not 100% sure if the implication of this change is safe to\nallow us to say \"now it can be\".\n\nThe has_object_file() helper wanted to say \"no\" when given a\nnon-existing object registered via the pretend_object_file(),\npresumably because we wanted to allow a use pattern like:\n\n - prepare an in-core representation of an object we tentatively\n   expect, but not absolutely sure, to be necessary.\n\n - perform operations, using the object data obtained via\n   read_object() API, which is capable of yielding data even for\n   such \"pretend\" objects (perhaps we are creating a tentative merge\n   parents during a recursive merge).\n\n - write out final set of objects by enumerating those that do not\n   really exist yet (via has_object_file() API).\n\nTeaching about the empty tree to has_object_file() is a good thing\n(especially because we do not necessarily write an empty tree object\nto our repositories), but as a collateral damage of doing so, we\nmake such use pattern impossible.  \n\nIt is not a large loss---the third bullet in the above list can just\nbe made to unconditionally call write_object_file() without\nfiltering with has_object_file() and write_object_file() will apply\nthe right optimization anyway, so it probably is OK.\n\nWill queue.\n\nThanks.\n"},{"id":"389203","messageId":"20200104001331.GA130883@google.com","threadId":"52542","inReplyTo":"20200102201630.180969-1-jonathantanmy@google.com","subject":"Re: [PATCH v2] sha1-file: remove OBJECT_INFO_SKIP_CACHED","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-01-04T00:13:31Z","receivedAt":"2020-01-04T00:13:36Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Tan wrote:\n\n> In a partial clone, if a user provides the hash of the empty tree (\"git\n> mktree </dev/null\" - for SHA-1, this is 4b825d...) to a command which\n> requires that that object be parsed, for example:\n>\n>   git diff-tree 4b825d <a non-empty tree>\n>\n> then Git will lazily fetch the empty tree, unnecessarily, because\n> parsing of that object invokes repo_has_object_file(), which does not\n> special-case the empty tree.\n>\n> Instead, teach repo_has_object_file() to consult find_cached_object()\n> (which handles the empty tree), thus bringing it in line with the rest\n> of the object-store-accessing functions. A cost is\n\nLovely, thank you.\n\n> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>\n> ---\n>  object-store.h |  2 --\n>  sha1-file.c    | 38 ++++++++++++++++++--------------------\n>  2 files changed, 18 insertions(+), 22 deletions(-)\n\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\nTo follow up on Junio's hint in his review: callers can inject\nadditional cached objects by using pretend_object_file.  Junio\ndescribed how this would make sense as a mechanism for building\nthe virtual ancestor object, but we don't do that.  In fact, the\nonly caller is fake_working_tree_commit in \"git blame\", a read-only\ncode path. *phew*\n\n-- >8 --\nSubject: sha1-file: document how to use pretend_object_file\n\nLike in-memory alternates, pretend_object_file contains a trap for the\nunwary: careless callers can use it to create references to an object\nthat does not exist in the on-disk object store.\n\nAdd a comment documenting how to use the function without risking such\nproblems.\n\nThe only current caller is blame, which uses pretend_object_file to\ncreate an in-memory commit representing the working tree state.\nNoticed during a discussion of how to safely use this function in\noperations like \"git merge\" which, unlike blame, are not read-only.\n\nInspired-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\n object-store.h | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/object-store.h b/object-store.h\nindex 55ee639350..d0fc7b091b 100644\n--- a/object-store.h\n+++ b/object-store.h\n@@ -208,6 +208,14 @@ int hash_object_file_literally(const void *buf, unsigned long len,\n \t\t\t       const char *type, struct object_id *oid,\n \t\t\t       unsigned flags);\n \n+/*\n+ * Add an object file to the in-memory object store, without writing it\n+ * to disk.\n+ *\n+ * Callers are responsible for calling write_object_file to record the\n+ * object in persistent storage before writing any other new objects\n+ * that reference it.\n+ */\n int pretend_object_file(void *, unsigned long, enum object_type,\n \t\t\tstruct object_id *oid);\n \n-- \n2.24.1.735.g03f4e72817\n\n"},{"id":"389287","messageId":"20200106211449.GA980197@coredump.intra.peff.net","threadId":"52542","inReplyTo":"xmqqftgxedtk.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] sha1-file: remove OBJECT_INFO_SKIP_CACHED","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-01-06T21:14:49Z","receivedAt":"2020-01-06T21:14:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 02, 2020 at 01:41:27PM -0800, Junio C Hamano wrote:\n\n> Jonathan Tan <jonathantanmy@google.com> writes:\n> \n> > As a historical note, the function now known as repo_read_object_file()\n> > was taught the empty tree in 346245a1bb (\"hard-code the empty tree\n> > object\", 2008-02-13), and the function now known as oid_object_info()\n> > was taught the empty tree in c4d9986f5f (\"sha1_object_info: examine\n> > cached_object store too\", 2011-02-07). repo_has_object_file() was never\n> > updated, perhaps due to oversight. The flag OBJECT_INFO_SKIP_CACHED,\n> > introduced later in dfdd4afcf9 (\"sha1_file: teach\n> > sha1_object_info_extended more flags\", 2017-06-26) and used in\n> > e83e71c5e1 (\"sha1_file: refactor has_sha1_file_with_flags\", 2017-06-26),\n> > was introduced to preserve this difference in empty-tree handling, but\n> > now it can be removed.\n> \n> I am not 100% sure if the implication of this change is safe to\n> allow us to say \"now it can be\".\n> \n> The has_object_file() helper wanted to say \"no\" when given a\n> non-existing object registered via the pretend_object_file(),\n> presumably because we wanted to allow a use pattern like:\n> \n>  - prepare an in-core representation of an object we tentatively\n>    expect, but not absolutely sure, to be necessary.\n> \n>  - perform operations, using the object data obtained via\n>    read_object() API, which is capable of yielding data even for\n>    such \"pretend\" objects (perhaps we are creating a tentative merge\n>    parents during a recursive merge).\n> \n>  - write out final set of objects by enumerating those that do not\n>    really exist yet (via has_object_file() API).\n> \n> Teaching about the empty tree to has_object_file() is a good thing\n> (especially because we do not necessarily write an empty tree object\n> to our repositories), but as a collateral damage of doing so, we\n> make such use pattern impossible.  \n> \n> It is not a large loss---the third bullet in the above list can just\n> be made to unconditionally call write_object_file() without\n> filtering with has_object_file() and write_object_file() will apply\n> the right optimization anyway, so it probably is OK.\n\nI agree that whoever called pretend_object_file() can be careful and\nwrite out the final set of objects itself via write_object_file(). But\nI'd worry a bit about a caller who doesn't necessarily realize that they\nneed to do that. E.g., imagine we call pretend_object_file() for some\nblob oid, expecting it to be read-only. And then in the same process,\nsome other bit of the code writes out a tree that mentions that blob.\nOops, that tree is now corrupt after we exit the process. And IMHO\nneither the pretend-caller nor the tree-writer are to blame; the problem\nis that they shared global state they were not expecting.\n\nThis is pretty far-fetched given that the only user of\npretend_object_file() is in git-blame right now. But it does give me\npause. Overall, though, I'm more inclined to say that we should be\ndropping SKIP_CACHED here and considering pretend_object_file() to be a\nbit dangerous (i.e., to keep it in mind if somebody proposes more\ncalls).\n\nAnother point of reference (in favor of Jonathan's patch):\n\n  https://lore.kernel.org/git/20190304174053.GA27497@sigill.intra.peff.net/\n\nis a bug that would not have happened if this patch had been applied\n(there's also some discussion of the greater issue, but nothing that wasn't\nalready brought up here, I think).\n\n-Peff\n"},{"id":"389289","messageId":"20200106211730.GB980197@coredump.intra.peff.net","threadId":"52542","inReplyTo":"20200104001331.GA130883@google.com","subject":"Re: [PATCH v2] sha1-file: remove OBJECT_INFO_SKIP_CACHED","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-01-06T21:17:30Z","receivedAt":"2020-01-06T21:17:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 03, 2020 at 04:13:31PM -0800, Jonathan Nieder wrote:\n\n> To follow up on Junio's hint in his review: callers can inject\n> additional cached objects by using pretend_object_file.  Junio\n> described how this would make sense as a mechanism for building\n> the virtual ancestor object, but we don't do that.  In fact, the\n> only caller is fake_working_tree_commit in \"git blame\", a read-only\n> code path. *phew*\n> \n> -- >8 --\n> Subject: sha1-file: document how to use pretend_object_file\n> [...]\n> +/*\n> + * Add an object file to the in-memory object store, without writing it\n> + * to disk.\n> + *\n> + * Callers are responsible for calling write_object_file to record the\n> + * object in persistent storage before writing any other new objects\n> + * that reference it.\n> + */\n>  int pretend_object_file(void *, unsigned long, enum object_type,\n>  \t\t\tstruct object_id *oid);\n>  \n\nI think this is an improvement over the status quo, but it's still a\npotential trap for code which happens to run in the same process (see my\nother email in the thread).\n\nShould the message perhaps be even more scary?\n\n-Peff\n"},{"id":"389304","messageId":"20200106234753.GB92456@google.com","threadId":"52542","inReplyTo":"20200106211730.GB980197@coredump.intra.peff.net","subject":"Re: [PATCH v2] sha1-file: remove OBJECT_INFO_SKIP_CACHED","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-01-06T23:47:53Z","receivedAt":"2020-01-06T23:48:00Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJeff King wrote:\n> On Fri, Jan 03, 2020 at 04:13:31PM -0800, Jonathan Nieder wrote:\n\n>> To follow up on Junio's hint in his review: callers can inject\n>> additional cached objects by using pretend_object_file.  Junio\n>> described how this would make sense as a mechanism for building\n>> the virtual ancestor object, but we don't do that.  In fact, the\n>> only caller is fake_working_tree_commit in \"git blame\", a read-only\n>> code path. *phew*\n>>\n>> -- >8 --\n>> Subject: sha1-file: document how to use pretend_object_file\n>> [...]\n>> +/*\n>> + * Add an object file to the in-memory object store, without writing it\n>> + * to disk.\n>> + *\n>> + * Callers are responsible for calling write_object_file to record the\n>> + * object in persistent storage before writing any other new objects\n>> + * that reference it.\n>> + */\n>>  int pretend_object_file(void *, unsigned long, enum object_type,\n>>  \t\t\tstruct object_id *oid);\n>\n> I think this is an improvement over the status quo, but it's still a\n> potential trap for code which happens to run in the same process (see my\n> other email in the thread).\n>\n> Should the message perhaps be even more scary?\n\nA pet peeve of mine is warning volume escalation: if it becomes common\nfor us to say\n\n * Warning: callers are reponsible for [...]\n\nthen new warnings trying to stand out might say\n\n * WARNING: callers are responsible for [...]\n\nand then after we are desensitized to that, we may switch to\n\n * WARNING WARNING WARNING, not the usual blah-blah: callers are\n\nand so on.  The main way I have found to counteract that is to make\nthe \"dangerous curve\" markers context-specific enough that people\ndon't learn to ignore them.  After all, sometimes a concurrency\nwarning is important to me, at other times warnings about clarity may\nbe what attract my interest, and so on.\n\nI don't have a good suggestion here.  Perhaps \"Callers are responsible\nfor\" is too slow and something more terse would help?\n\n /*\n  * Adds an object to the in-memory object store, without writing it\n  * to disk.\n  *\n  * Use with caution!  Unless you call write_object_file to record the\n  * in-memory object to persistent storage, any other new objects that\n  * reference it will point to a missing (in memory only) object,\n  * resulting in a corrupt repository.\n  */\n\nIt would be even better if we have some automated way to catch this\nkind of issue.  Should tests run \"git fsck\" after each test?  Should\nwrite_object_file have a paranoid mode that checks integrity?\n\nI don't know an efficient way to do that.  Ultimately I am comfortable\ncounting on reviewers to be aware of this kind of pitfall.  While\nnonlocal invariants are always hard to maintain, this pitfall is\ninherent in the semantics of the function, so I am not too worried\nthat reviewers will overlook it.\n\nA less error-prone interface would tie the result of\npretend_object_file to a short-lived overlay on the_repository without\naffecting global state.  We could even enforce read-only access in\nthat overlay.  I don't think the \"struct repository\" interface and\ncallers are ready for that yet, though.\n\nThanks,\nJonathan\n"},{"id":"389353","messageId":"20200107112213.GD1073219@coredump.intra.peff.net","threadId":"52542","inReplyTo":"20200106234753.GB92456@google.com","subject":"Re: [PATCH v2] sha1-file: remove OBJECT_INFO_SKIP_CACHED","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-01-07T11:22:13Z","receivedAt":"2020-01-07T11:22:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 06, 2020 at 03:47:53PM -0800, Jonathan Nieder wrote:\n\n> >> + * Callers are responsible for calling write_object_file to record the\n> >> + * object in persistent storage before writing any other new objects\n> >> + * that reference it.\n> >> + */\n> >>  int pretend_object_file(void *, unsigned long, enum object_type,\n> >>  \t\t\tstruct object_id *oid);\n> >\n> > I think this is an improvement over the status quo, but it's still a\n> > potential trap for code which happens to run in the same process (see my\n> > other email in the thread).\n> >\n> > Should the message perhaps be even more scary?\n> \n> A pet peeve of mine is warning volume escalation: if it becomes common\n> for us to say\n> \n>  * Warning: callers are reponsible for [...]\n> \n> then new warnings trying to stand out might say\n> \n>  * WARNING: callers are responsible for [...]\n> \n> and then after we are desensitized to that, we may switch to\n> \n>  * WARNING WARNING WARNING, not the usual blah-blah: callers are\n> \n> and so on.  The main way I have found to counteract that is to make\n> the \"dangerous curve\" markers context-specific enough that people\n> don't learn to ignore them.  After all, sometimes a concurrency\n> warning is important to me, at other times warnings about clarity may\n> be what attract my interest, and so on.\n\nI meant less about the number of capital letters, and more that we\nshould be saying \"this interface is dangerous; don't use it\". Because\nit's not just \"callers are responsible\". It's \"this can cause subtle\nand hard-to-debug issues because it's writing to global state\".\n\nMy preferred solution would actually be to rip it out entirely, but we'd\nneed some solution for git-blame, the sole caller. Possibly it could\ninsert the value straight into the diff_filespec. But according to the\nthread that I linked earlier, I poked at that last year but it didn't\nlook trivial.\n\n> I don't have a good suggestion here.  Perhaps \"Callers are responsible\n> for\" is too slow and something more terse would help?\n> \n>  /*\n>   * Adds an object to the in-memory object store, without writing it\n>   * to disk.\n>   *\n>   * Use with caution!  Unless you call write_object_file to record the\n>   * in-memory object to persistent storage, any other new objects that\n>   * reference it will point to a missing (in memory only) object,\n>   * resulting in a corrupt repository.\n>   */\n\nYeah, that's more what I had in mind.\n\n> It would be even better if we have some automated way to catch this\n> kind of issue.  Should tests run \"git fsck\" after each test?  Should\n> write_object_file have a paranoid mode that checks integrity?\n> \n> I don't know an efficient way to do that.  Ultimately I am comfortable\n> counting on reviewers to be aware of this kind of pitfall.  While\n> nonlocal invariants are always hard to maintain, this pitfall is\n> inherent in the semantics of the function, so I am not too worried\n> that reviewers will overlook it.\n\nYeah, given the scope of the problem (we have a single caller, and this\nmechanism is over a decade old) I'm fine with review as the enforcement\nmechanism, too.\n\n> A less error-prone interface would tie the result of\n> pretend_object_file to a short-lived overlay on the_repository without\n> affecting global state.  We could even enforce read-only access in\n> that overlay.  I don't think the \"struct repository\" interface and\n> callers are ready for that yet, though.\n\nI agree that would be better, though it's still kind-of global (in that\nthe repository object is effectively a global for most processes).\n\n-Peff\n"}]}