{"thread":{"id":"50646","subject":"[PATCH] rev-list: allow cached objects in existence check","startedAt":"2019-03-04T17:40:57Z","lastAt":"2019-03-05T19:27:33Z","messageCount":6,"participants":["Jeff King","Jonathan Tan","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"370625","messageId":"20190304174053.GA27497@sigill.intra.peff.net","threadId":"50646","inReplyTo":null,"subject":"[PATCH] rev-list: allow cached objects in existence check","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-04T17:40:54Z","receivedAt":"2019-03-04T17:40:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This fixes a regression in 7c0fe330d5 (rev-list: handle missing tree\nobjects properly, 2018-10-05) where rev-list will now complain about the\nempty tree when it doesn't physically exist on disk.\n\nBefore that commit, we relied on the traversal code in list-objects.c to\nwalk through the trees. Since it uses parse_tree(), we'd do a normal\nobject lookup that includes looking in the set of \"cached\" objects\n(which is where our magic internal empty-tree kicks in).\n\nAfter that commit, we instead tell list-objects.c not to die on any\nmissing trees, and we check them ourselves using has_object_file(). But\nthat function uses OBJECT_INFO_SKIP_CACHED, which means we won't use our\ninternal empty tree.\n\nThis normally wouldn't come up. For most operations, Git will try to\nwrite out the empty tree object as it would any other object. And\npack-objects in a push or fetch will send the empty tree (even if it's\nvirtual on the sending side). However, there are cases where this can\nmatter. One I found in the wild:\n\n  1. The root tree of a commit became empty by deleting all files,\n     without using an index. In this case it was done using libgit2's\n     tree builder API, but as the included test shows, it can easily be\n     done with regular git using hash-object.\n\n     The resulting repo works OK, as we'd avoid walking over our own\n     reachable commits for a connectivity check.\n\n  2. Cloning with --reference pointing to the repository from (1) can\n     trigger the problem, because we tell the other side we already have\n     that commit (and hence the empty tree), but then walk over it\n     during the connectivity check (where we complain about it missing).\n\nArguably the workflow in step (1) should be more careful about writing\nthe empty tree object if we're referencing it. But this workflow did\nwork prior to 7c0fe330d5, so let's restore it.\n\nThis patch makes the minimal fix, which is to swap out a direct call to\noid_object_info_extended(), minus the SKIP_CACHED flag, instead of\ncalling has_object_file(). This is all that has_object_file() is doing\nunder the hood. And there's little danger of unrelated fallout from\nother unexpected \"cached\" objects, since there's only one call site that\nends such a cached object, and it's in git-blame.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI prepared this directly on top of 7c0fe330d5, but it should merge\ncleanly into the current tip of master.\n\nI think we might also consider just having has_object_file() respect\ncached objects. The SKIP_CACHED flag comes from Jonathan Tan's\ne83e71c5e1 (sha1_file: refactor has_sha1_file_with_flags, 2017-06-21).\nBut it was just matching the old behavior; it's not clear to me that we\nparticularly care about that, and it wasn't simply that nobody bothered\nto put the cached-object check into has_sha1_file().\n\nSome concerns/arguments against it:\n\n  - we probably would want to make sure we do not short-cut\n    write_sha1_file(). I.e., we should still write it to disk when\n    somebody wants it. But I think that works, because that function\n    uses its own check-and-freshen infrastructure.\n\n  - some callers of has_sha1_file() might care about durability between\n    processes. Because it's baked in, the empty tree is safe for that\n    (whatever follow-on process runs, it will also be baked in there).\n    But that's not necessarily true for other \"cached\" objects. I'm not\n    really that worried about it because we use it sparingly (the only\n    call to pretend_sha1_file() is in git-blame, and if it ever did ask\n    \"do we have this object\", I actually think the right answer would be\n    \"yes\").\n\n    But if this is a concern, we could perhaps have two levels of flags:\n    SKIP_CACHED and SKIP_INTERNAL.\n\n builtin/rev-list.c           |  2 +-\n t/t1060-object-corruption.sh | 10 ++++++++++\n 2 files changed, 11 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex 49d6deed70..877b6561f4 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -237,7 +237,7 @@ static inline void finish_object__ma(struct object *obj)\n static int finish_object(struct object *obj, const char *name, void *cb_data)\n {\n \tstruct rev_list_info *info = cb_data;\n-\tif (!has_object_file(&obj->oid)) {\n+\tif (oid_object_info_extended(the_repository, &obj->oid, NULL, 0) < 0) {\n \t\tfinish_object__ma(obj);\n \t\treturn 1;\n \t}\ndiff --git a/t/t1060-object-corruption.sh b/t/t1060-object-corruption.sh\nindex ac1f189fd2..807b63b473 100755\n--- a/t/t1060-object-corruption.sh\n+++ b/t/t1060-object-corruption.sh\n@@ -125,4 +125,14 @@ test_expect_success 'fetch into corrupted repo with index-pack' '\n \t)\n '\n \n+test_expect_success 'internal tree objects are not \"missing\"' '\n+\tgit init missing-empty &&\n+\t(\n+\t\tcd missing-empty &&\n+\t\tempty_tree=$(git hash-object -t tree /dev/null) &&\n+\t\tcommit=$(echo foo | git commit-tree $empty_tree) &&\n+\t\tgit rev-list --objects $commit\n+\t)\n+'\n+\n test_done\n-- \n2.21.0.684.gc9dc8b89c9\n"},{"id":"370627","messageId":"20190304191932.105204-1-jonathantanmy@google.com","threadId":"50646","inReplyTo":"20190304174053.GA27497@sigill.intra.peff.net","subject":"Re: [PATCH] rev-list: allow cached objects in existence check","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-03-04T19:19:32Z","receivedAt":"2019-03-04T19:19:46Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> This patch makes the minimal fix, which is to swap out a direct call to\n> oid_object_info_extended(), minus the SKIP_CACHED flag, instead of\n> calling has_object_file(). This is all that has_object_file() is doing\n> under the hood. And there's little danger of unrelated fallout from\n> other unexpected \"cached\" objects, since there's only one call site that\n> ends such a cached object, and it's in git-blame.\n\nThanks - this patch looks good to me.\n\n> I think we might also consider just having has_object_file() respect\n> cached objects. The SKIP_CACHED flag comes from Jonathan Tan's\n> e83e71c5e1 (sha1_file: refactor has_sha1_file_with_flags, 2017-06-21).\n> But it was just matching the old behavior; it's not clear to me that we\n> particularly care about that, and it wasn't simply that nobody bothered\n> to put the cached-object check into has_sha1_file().\n\nMaking has_object_file() respect cached objects sounds good to me too.\n\n> Some concerns/arguments against it:\n> \n>   - we probably would want to make sure we do not short-cut\n>     write_sha1_file(). I.e., we should still write it to disk when\n>     somebody wants it. But I think that works, because that function\n>     uses its own check-and-freshen infrastructure.\n\nwrite_object_file() (formerly write_sha1_file()) indeed uses its own\ncheck-and-freshen mechanism.\n\n>   - some callers of has_sha1_file() might care about durability between\n>     processes. Because it's baked in, the empty tree is safe for that\n>     (whatever follow-on process runs, it will also be baked in there).\n>     But that's not necessarily true for other \"cached\" objects. I'm not\n>     really that worried about it because we use it sparingly (the only\n>     call to pretend_sha1_file() is in git-blame, and if it ever did ask\n>     \"do we have this object\", I actually think the right answer would be\n>     \"yes\").\n> \n>     But if this is a concern, we could perhaps have two levels of flags:\n>     SKIP_CACHED and SKIP_INTERNAL.\n\nOr teach git-blame to have its own pretend mechanism, and remove the\npretend mechanism from sha1-file.c.\n\nThe last time I deeply thought of this was during the partial clone\nimplementation, so I am probably not completely up-to-date, but it seems\nto me that ideally, for reading, we would remove SKIP_CACHED completely\n(and always consult the cache), and also remove completely the ability\nto pretend (blame will have to do it by itself); and for writing, we\nwould write the empty tree whenever we do now (for backwards\ncompatibility with old versions of Git that read what we write). Both\nthe approach in this patch and making has_object_file() respect cached\nobjects are steps in that direction, so I'm OK with both.\n"},{"id":"370640","messageId":"20190304211749.GA3020@sigill.intra.peff.net","threadId":"50646","inReplyTo":"20190304191932.105204-1-jonathantanmy@google.com","subject":"Re: [PATCH] rev-list: allow cached objects in existence check","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-04T21:17:49Z","receivedAt":"2019-03-04T21:17:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 04, 2019 at 11:19:32AM -0800, Jonathan Tan wrote:\n\n> >   - some callers of has_sha1_file() might care about durability between\n> >     processes. Because it's baked in, the empty tree is safe for that\n> >     (whatever follow-on process runs, it will also be baked in there).\n> >     But that's not necessarily true for other \"cached\" objects. I'm not\n> >     really that worried about it because we use it sparingly (the only\n> >     call to pretend_sha1_file() is in git-blame, and if it ever did ask\n> >     \"do we have this object\", I actually think the right answer would be\n> >     \"yes\").\n> > \n> >     But if this is a concern, we could perhaps have two levels of flags:\n> >     SKIP_CACHED and SKIP_INTERNAL.\n> \n> Or teach git-blame to have its own pretend mechanism, and remove the\n> pretend mechanism from sha1-file.c.\n\nI think that would be ideal, but I'm not sure if it's feasible due to\nthe layering of the various modules. IOW, the blame code isn't just\npretending a fake object file for _itself_, it needs to then call into\nthe diff code, which must be able to then find that content in order to\nproduce a diff.\n\nBut maybe it is not so bad. Our diff_filespec struct does represent\nworking-tree files (as it must, since we diff them!). So it may be\npossible to feed it to the diff code at the right spot.\n\nI haven't looked closely enough to say for sure whether it's feasible or\nnot. But it does imply to me that we should go with this regression fix\nin the near-term and think about building bigger changes separately on\nmaster.\n\n> The last time I deeply thought of this was during the partial clone\n> implementation, so I am probably not completely up-to-date, but it seems\n> to me that ideally, for reading, we would remove SKIP_CACHED completely\n> (and always consult the cache), and also remove completely the ability\n> to pretend (blame will have to do it by itself); and for writing, we\n> would write the empty tree whenever we do now (for backwards\n> compatibility with old versions of Git that read what we write). Both\n> the approach in this patch and making has_object_file() respect cached\n> objects are steps in that direction, so I'm OK with both.\n\nYeah, I think our world-views are in accord. :)\n\n-Peff\n"},{"id":"370712","messageId":"xmqq4l8hv4b5.fsf@gitster-ct.c.googlers.com","threadId":"50646","inReplyTo":"20190304174053.GA27497@sigill.intra.peff.net","subject":"Re: [PATCH] rev-list: allow cached objects in existence check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-05T13:28:14Z","receivedAt":"2019-03-05T13:28:19Z","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> This fixes a regression in 7c0fe330d5 (rev-list: handle missing tree\n> objects properly, 2018-10-05) where rev-list will now complain about the\n> empty tree when it doesn't physically exist on disk.\n>\n> Before that commit, we relied on the traversal code in list-objects.c to\n> walk through the trees. Since it uses parse_tree(), we'd do a normal\n> object lookup that includes looking in the set of \"cached\" objects\n> (which is where our magic internal empty-tree kicks in).\n>\n> After that commit, we instead tell list-objects.c not to die on any\n> missing trees, and we check them ourselves using has_object_file(). But\n> that function uses OBJECT_INFO_SKIP_CACHED, which means we won't use our\n> internal empty tree.\n\nYikes.  Thanks for spotting.\n\n> This patch makes the minimal fix, which is to swap out a direct call to\n> oid_object_info_extended(), minus the SKIP_CACHED flag, instead of\n> calling has_object_file(). This is all that has_object_file() is doing\n> under the hood. And there's little danger of unrelated fallout from\n> other unexpected \"cached\" objects, since there's only one call site that\n> ends such a cached object, and it's in git-blame.\n\nOK.  That last one is not even \"cached\" but \"merely exists in-core\"\nvirtual commit, that represents the locally modified state, right?\nI do not think rev-list ever asks if these object exist, but if they\nwere asked, we should say they also exist.\n\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> I prepared this directly on top of 7c0fe330d5, but it should merge\n> cleanly into the current tip of master.\n>\n> I think we might also consider just having has_object_file() respect\n> cached objects. The SKIP_CACHED flag comes from Jonathan Tan's\n> e83e71c5e1 (sha1_file: refactor has_sha1_file_with_flags, 2017-06-21).\n> But it was just matching the old behavior; it's not clear to me that we\n> particularly care about that, and it wasn't simply that nobody bothered\n> to put the cached-object check into has_sha1_file().\n\nYup, I am fine with such a clean-up after we fix this regression.\n\n> Some concerns/arguments against it:\n>\n>   - we probably would want to make sure we do not short-cut\n>     write_sha1_file(). I.e., we should still write it to disk when\n>     somebody wants it. But I think that works, because that function\n>     uses its own check-and-freshen infrastructure.\n>\n>   - some callers of has_sha1_file() might care about durability between\n>     processes. Because it's baked in, the empty tree is safe for that\n>     (whatever follow-on process runs, it will also be baked in there).\n>     But that's not necessarily true for other \"cached\" objects. I'm not\n>     really that worried about it because we use it sparingly (the only\n>     call to pretend_sha1_file() is in git-blame, and if it ever did ask\n>     \"do we have this object\", I actually think the right answer would be\n>     \"yes\").\n\n... and I realize that I should have read ahead before writing the\nfour lines above myself ;-)  We are on the same page.\n\n>     But if this is a concern, we could perhaps have two levels of flags:\n>     SKIP_CACHED and SKIP_INTERNAL.\n>\n>  builtin/rev-list.c           |  2 +-\n>  t/t1060-object-corruption.sh | 10 ++++++++++\n>  2 files changed, 11 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/rev-list.c b/builtin/rev-list.c\n> index 49d6deed70..877b6561f4 100644\n> --- a/builtin/rev-list.c\n> +++ b/builtin/rev-list.c\n> @@ -237,7 +237,7 @@ static inline void finish_object__ma(struct object *obj)\n>  static int finish_object(struct object *obj, const char *name, void *cb_data)\n>  {\n>  \tstruct rev_list_info *info = cb_data;\n> -\tif (!has_object_file(&obj->oid)) {\n> +\tif (oid_object_info_extended(the_repository, &obj->oid, NULL, 0) < 0) {\n>  \t\tfinish_object__ma(obj);\n>  \t\treturn 1;\n>  \t}\n> diff --git a/t/t1060-object-corruption.sh b/t/t1060-object-corruption.sh\n> index ac1f189fd2..807b63b473 100755\n> --- a/t/t1060-object-corruption.sh\n> +++ b/t/t1060-object-corruption.sh\n> @@ -125,4 +125,14 @@ test_expect_success 'fetch into corrupted repo with index-pack' '\n>  \t)\n>  '\n>  \n> +test_expect_success 'internal tree objects are not \"missing\"' '\n> +\tgit init missing-empty &&\n> +\t(\n> +\t\tcd missing-empty &&\n> +\t\tempty_tree=$(git hash-object -t tree /dev/null) &&\n> +\t\tcommit=$(echo foo | git commit-tree $empty_tree) &&\n> +\t\tgit rev-list --objects $commit\n> +\t)\n> +'\n> +\n>  test_done\n"},{"id":"370713","messageId":"xmqqzhq9tpie.fsf@gitster-ct.c.googlers.com","threadId":"50646","inReplyTo":"20190304211749.GA3020@sigill.intra.peff.net","subject":"Re: [PATCH] rev-list: allow cached objects in existence check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-05T13:33:13Z","receivedAt":"2019-03-05T13:33:17Z","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>> Or teach git-blame to have its own pretend mechanism, and remove the\n>> pretend mechanism from sha1-file.c.\n>\n> I think that would be ideal, but I'm not sure if it's feasible due to\n> the layering of the various modules.\n\nSorry, but I do not get why we want command-line specific pretend\nmechanism.  When one part of the system wants to behave as if object\nX exists, doesn't that part want other parts of the system to share\nthat same world view to be consistent?\n\nI am mostly reacting to \"would be _ideal_\"; if it were \"if we have\nper-system ad-hoc pretend mechanism, things like this and that would\nbecome easier to implement, even though that is an ugly hack\", I may\nagree when I see examples of things that get easier, though.\n"},{"id":"370748","messageId":"20190305192729.GA13409@sigill.intra.peff.net","threadId":"50646","inReplyTo":"xmqqzhq9tpie.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] rev-list: allow cached objects in existence check","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-05T19:27:30Z","receivedAt":"2019-03-05T19:27:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 05, 2019 at 10:33:13PM +0900, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> >> Or teach git-blame to have its own pretend mechanism, and remove the\n> >> pretend mechanism from sha1-file.c.\n> >\n> > I think that would be ideal, but I'm not sure if it's feasible due to\n> > the layering of the various modules.\n> \n> Sorry, but I do not get why we want command-line specific pretend\n> mechanism.  When one part of the system wants to behave as if object\n> X exists, doesn't that part want other parts of the system to share\n> that same world view to be consistent?\n> \n> I am mostly reacting to \"would be _ideal_\"; if it were \"if we have\n> per-system ad-hoc pretend mechanism, things like this and that would\n> become easier to implement, even though that is an ugly hack\", I may\n> agree when I see examples of things that get easier, though.\n\nThe problem is that it's not clear how each of those other parts of the\nsystem should react to these pretend objects. E.g., they probably should\n_not_ be used in any operation that might write, since we would not want\nto create a permanent object that points to an ephemeral one.\n\nBy sticking this in sha1-file.c, it becomes hard to know who will access\nthem, or with what expectations. Things work right now because we use\nthe feature sparingly (and only from a process that's purely read-only).\nBut we're at risk of somebody later misusing it, especially if we spread\nits use to more functions like has_object_file().  If this were local to\ngit-blame, then that risk goes away.\n\nSo that's what I meant by \"ideal\".\n\nI don't think it makes anything easier (in fact, after looking, I think\nit makes things in git-blame much harder, to the point that I am not\nplanning to work on it anytime soon).\n\n-Peff\n"}]}