{"thread":{"id":"52531","subject":"[PATCH] revision: allow missing promisor objects on CLI","startedAt":"2019-12-28T00:34:37Z","lastAt":"2020-03-25T20:50:43Z","messageCount":12,"participants":["Jonathan Tan","Junio C Hamano","Jonathan Nieder","Emily Shaffer"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"389016","messageId":"20191228003430.241283-1-jonathantanmy@google.com","threadId":"52531","inReplyTo":null,"subject":"[PATCH] revision: allow missing promisor objects on CLI","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-12-28T00:34:30Z","receivedAt":"2019-12-28T00:34:37Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Commit 4cf67869b2 (\"list-objects.c: don't segfault for missing cmdline\nobjects\", 2018-12-06) prevented some segmentation faults from occurring\nby tightening handling of missing objects provided through the CLI: if\n--ignore-missing is set, then it is OK (and the missing object ignored,\njust like one would if encountered in traversal).\n\nHowever, in the case that --ignore-missing is not set but\n--exclude-promisor-objects is set, there is still no distinction between\nthe case wherein the missing object is a promisor object and the case\nwherein it is not. This is unnecessarily restrictive, since if a missing\npromisor object is encountered in traversal, it is ignored; likewise it\nshould be ignored if provided through the CLI. Therefore, distinguish\nbetween these 2 cases. (As a bonus, the code is now simpler.)\n\n(Note that this only affects handling of missing promisor objects.\nHandling of non-missing promisor objects is already done by setting all\nof them to UNINTERESTING in prepare_revision_walk().)\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\nFor those curious, I discovered this while trying to extend the\ncheck-targets-only optimization done in dfa33a298d (\"clone: do faster\nobject check for partial clones\", 2019-04-21) to fetches as well. While\ninvestigating my previous work of adding check-target functionality in\naddition to the usual connectivity check (for correctness, not for\nperformance) in 35f9e3e5e7 (\"fetch: in partial clone, check presence of\ntargets\", 2018-09-21), I discovered that at current master, the check\nwas somehow no longer needed because rev-list dies on missing objects on\nCLI. But I don't think that the current behavior is obvious, hence this\ncommit (which also restores the need for the check-target\nfunctionality).\n---\n revision.c               | 10 +++++++++-\n t/t0410-partial-clone.sh | 10 ++--------\n 2 files changed, 11 insertions(+), 9 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 8136929e23..345615e300 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1907,7 +1907,15 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi\n \t\tverify_non_filename(revs->prefix, arg);\n \tobject = get_reference(revs, arg, &oid, flags ^ local_flags);\n \tif (!object)\n-\t\treturn revs->ignore_missing ? 0 : -1;\n+\t\t/*\n+\t\t * Either this object is missing and ignore_missing is true, or\n+\t\t * this object is a (missing) promisor object and\n+\t\t * exclude_promisor_objects is true. In any case, we are\n+\t\t * allowed to skip processing of this object; this object will\n+\t\t * not appear in output and cannot be used as a source of\n+\t\t * UNINTERESTING ancestors (since it is missing).\n+\t\t */\n+\t\treturn 0;\n \tadd_rev_cmdline(revs, object, arg_, REV_CMD_REV, flags ^ local_flags);\n \tadd_pending_object_with_path(revs, object, arg, oc.mode, oc.path);\n \tfree(oc.path);\ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex a3988bd4b8..fd28f5402a 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -416,15 +416,9 @@ test_expect_success 'rev-list dies for missing objects on cmd line' '\n \tgit -C repo config extensions.partialclone \"arbitrary string\" &&\n \n \tfor OBJ in \"$COMMIT\" \"$TREE\" \"$BLOB\"; do\n-\t\ttest_must_fail git -C repo rev-list --objects \\\n+\t\tgit -C repo rev-list --objects \\\n \t\t\t--exclude-promisor-objects \"$OBJ\" &&\n-\t\ttest_must_fail git -C repo rev-list --objects-edge-aggressive \\\n-\t\t\t--exclude-promisor-objects \"$OBJ\" &&\n-\n-\t\t# Do not die or crash when --ignore-missing is passed.\n-\t\tgit -C repo rev-list --ignore-missing --objects \\\n-\t\t\t--exclude-promisor-objects \"$OBJ\" &&\n-\t\tgit -C repo rev-list --ignore-missing --objects-edge-aggressive \\\n+\t\tgit -C repo rev-list --objects-edge-aggressive \\\n \t\t\t--exclude-promisor-objects \"$OBJ\"\n \tdone\n '\n-- \n2.24.1.735.g03f4e72817-goog\n\n"},{"id":"389017","messageId":"xmqqlfqxhzvu.fsf@gitster-ct.c.googlers.com","threadId":"52531","inReplyTo":"20191228003430.241283-1-jonathantanmy@google.com","subject":"Re: [PATCH] revision: allow missing promisor objects on CLI","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-28T03:50:45Z","receivedAt":"2019-12-28T04:00:16Z","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>  \tobject = get_reference(revs, arg, &oid, flags ^ local_flags);\n>  \tif (!object)\n> -\t\treturn revs->ignore_missing ? 0 : -1;\n> +\t\t/*\n> +\t\t * Either this object is missing and ignore_missing is true, or\n> +\t\t * this object is a (missing) promisor object and\n> +\t\t * exclude_promisor_objects is true.\n\nI had to guess and dig where these assertions are coming from; we\nshould not force future readers of the code to.\n\nAt least this comment must say why these assertions hold.  Say\nsomething like \"get_reference() yields NULL on only such and such\ncases\" before concluding with \"and in any of these cases, we can\nsafely ignore it because ...\".\n\nI think the two cases the comment covers are safe for this caller to\nsilently return 0.  Another case get_reference() yields NULL is when\noid_object_info() says it is a commit but it turns out that the\nobject is found by repo_parse_commit() to be a non-commit, isn't it?\nI am not sure if it is safe for this caller to just return 0.  There\nmay be some other \"unusual-but-not-fatal\" cases where get_reference()\ndoes not hit a die() but returns NULL.\n\nThanks.\n\n"},{"id":"389084","messageId":"20191230183801.28538-1-jonathantanmy@google.com","threadId":"52531","inReplyTo":"xmqqlfqxhzvu.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] revision: allow missing promisor objects on CLI","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-12-30T18:38:01Z","receivedAt":"2019-12-30T18:38:07Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Jonathan Tan <jonathantanmy@google.com> writes:\n> \n> >  \tobject = get_reference(revs, arg, &oid, flags ^ local_flags);\n> >  \tif (!object)\n> > -\t\treturn revs->ignore_missing ? 0 : -1;\n> > +\t\t/*\n> > +\t\t * Either this object is missing and ignore_missing is true, or\n> > +\t\t * this object is a (missing) promisor object and\n> > +\t\t * exclude_promisor_objects is true.\n> \n> I had to guess and dig where these assertions are coming from; we\n> should not force future readers of the code to.\n> \n> At least this comment must say why these assertions hold.  Say\n> something like \"get_reference() yields NULL on only such and such\n> cases\" before concluding with \"and in any of these cases, we can\n> safely ignore it because ...\".\n\nOK, will do.\n\n> I think the two cases the comment covers are safe for this caller to\n> silently return 0.  Another case get_reference() yields NULL is when\n> oid_object_info() says it is a commit but it turns out that the\n> object is found by repo_parse_commit() to be a non-commit, isn't it?\n> I am not sure if it is safe for this caller to just return 0.  There\n> may be some other \"unusual-but-not-fatal\" cases where get_reference()\n> does not hit a die() but returns NULL.\n\nI don't think there is any other case where get_reference() yields NULL,\nat least where I based my patch (99c33bed56 (\"Git 2.25-rc0\",\n2019-12-25)). Quoting the entire get_reference():\n\n> static struct object *get_reference(struct rev_info *revs, const char *name,\n>                                     const struct object_id *oid,\n>                                     unsigned int flags)\n> {\n>         struct object *object;\n> \n>         /*\n>          * If the repository has commit graphs, repo_parse_commit() avoids\n>          * reading the object buffer, so use it whenever possible.\n>          */\n>         if (oid_object_info(revs->repo, oid, NULL) == OBJ_COMMIT) {\n>                 struct commit *c = lookup_commit(revs->repo, oid);\n>                 if (!repo_parse_commit(revs->repo, c))\n>                         object = (struct object *) c;\n>                 else\n>                         object = NULL;\n>         } else {\n>                 object = parse_object(revs->repo, oid);\n>         }\n\nNo return statements at all prior to this line.\n\n>         if (!object) {\n>                 if (revs->ignore_missing)\n>                         return object;\n\nReturn NULL (the value of object).\n\n>                 if (revs->exclude_promisor_objects && is_promisor_object(oid))\n>                         return NULL;\n\nReturn NULL.\n\n>                 die(\"bad object %s\", name);\n\nDie (so this function invocation never returns). In conclusion, if\nobject is NULL at this point in time, get_reference() either returns\nNULL or dies.\n\n>         }\n\nSince get_reference() did not return NULL or die, object is non-NULL\nhere.\n\n>         object->flags |= flags;\n>         return object;\n\nNothing has overridden object since, so we're returning non-NULL here.\n\n> }\n\nSo I think get_reference() only returns NULL in those two safe cases.\n(Or did I miss something?)\n"},{"id":"389101","messageId":"xmqq1rslh7ur.fsf@gitster-ct.c.googlers.com","threadId":"52531","inReplyTo":"20191230183801.28538-1-jonathantanmy@google.com","subject":"Re: [PATCH] revision: allow missing promisor objects on CLI","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-30T20:33:00Z","receivedAt":"2019-12-30T20:33:05Z","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>> Jonathan Tan <jonathantanmy@google.com> writes:\n>> \n>> >  \tobject = get_reference(revs, arg, &oid, flags ^ local_flags);\n>> >  \tif (!object)\n>> > -\t\treturn revs->ignore_missing ? 0 : -1;\n>> > +\t\t/*\n>> > +\t\t * Either this object is missing and ignore_missing is true, or\n>> > +\t\t * this object is a (missing) promisor object and\n>> > +\t\t * exclude_promisor_objects is true.\n>> \n>> I had to guess and dig where these assertions are coming from; we\n>> should not force future readers of the code to.\n>> \n>> At least this comment must say why these assertions hold.  Say\n>> something like \"get_reference() yields NULL on only such and such\n>> cases\" before concluding with \"and in any of these cases, we can\n>> safely ignore it because ...\".\n>\n> OK, will do.\n>\n>> I think the two cases the comment covers are safe for this caller to\n>> silently return 0.  Another case get_reference() yields NULL is when\n>> oid_object_info() says it is a commit but it turns out that the\n>> object is found by repo_parse_commit() to be a non-commit, isn't it?\n>> I am not sure if it is safe for this caller to just return 0.  There\n>> may be some other \"unusual-but-not-fatal\" cases where get_reference()\n>> does not hit a die() but returns NULL.\n>\n> I don't think there is any other case where get_reference() yields NULL,\n> at least where I based my patch (99c33bed56 (\"Git 2.25-rc0\",\n> 2019-12-25)). Quoting the entire get_reference():\n>\n>> static struct object *get_reference(struct rev_info *revs, const char *name,\n>>                                     const struct object_id *oid,\n>>                                     unsigned int flags)\n>> {\n>>         struct object *object;\n>> \n>>         /*\n>>          * If the repository has commit graphs, repo_parse_commit() avoids\n>>          * reading the object buffer, so use it whenever possible.\n>>          */\n>>         if (oid_object_info(revs->repo, oid, NULL) == OBJ_COMMIT) {\n>>                 struct commit *c = lookup_commit(revs->repo, oid);\n>>                 if (!repo_parse_commit(revs->repo, c))\n>>                         object = (struct object *) c;\n>>                 else\n>>                         object = NULL;\n\nThis is the case where oid must be COMMIT from oid_object_info()'s\npoint of view, but repo_parse_commit() finds it as a non-commit, and\nobject becomes NULL.  This is quite different from the normal lazy\nclone case where exclude-promisor-objects etc. wants to cover, that\nthe object whose name is oid is truly missing because it can be\nfetched later from elsewhere.  Instead, we have found that there is\nan inconsistency in the data we have about the object, iow, a\npossible corruption.\n\n>>         if (!object) {\n>>                 if (revs->ignore_missing)\n>>                         return object;\n>\n> Return NULL (the value of object).\n>\n>>                 if (revs->exclude_promisor_objects && is_promisor_object(oid))\n>>                         return NULL;\n>\n> Return NULL.\n>\n>>                 die(\"bad object %s\", name);\n>\n> Die (so this function invocation never returns). In conclusion, if\n> object is NULL at this point in time, get_reference() either returns\n> NULL or dies.\n\nAnd when !object, this does not die if\n\n - ignore-missing is in effect, or\n - exclude-promisor is in effect and this is a promisor object that\n   is missing from the local repository.\n\nand instead return NULL.\n\nIt just felt funny that the \"we found something fishy about the\nasked-for object\" case is treated the same way for the purpose of\nignore-missing and exclude-promisor.  The asked-for objet is\ncertainly not missing (i.e. we know more than we want to know about\nthe object---some part of our database says it is a commit and some\nother part says it is not).\n"},{"id":"389118","messageId":"20191230234453.255082-1-jonathantanmy@google.com","threadId":"52531","inReplyTo":"20191228003430.241283-1-jonathantanmy@google.com","subject":"[PATCH v2] revision: allow missing promisor objects on CLI","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-12-30T23:44:53Z","receivedAt":"2019-12-30T23:45:14Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Commit 4cf67869b2 (\"list-objects.c: don't segfault for missing cmdline\nobjects\", 2018-12-06) prevented some segmentation faults from occurring\nby tightening handling of missing objects provided through the CLI: if\n--ignore-missing is set, then it is OK (and the missing object ignored,\njust like one would if encountered in traversal).\n\nHowever, in the case that --ignore-missing is not set but\n--exclude-promisor-objects is set, there is still no distinction between\nthe case wherein the missing object is a promisor object and the case\nwherein it is not. This is unnecessarily restrictive, since if a missing\npromisor object is encountered in traversal, it is ignored; likewise it\nshould be ignored if provided through the CLI. Therefore, distinguish\nbetween these 2 cases. (As a bonus, the code is now simpler.)\n\n(Note that this only affects handling of missing promisor objects.\nHandling of non-missing promisor objects is already done by setting all\nof them to UNINTERESTING in prepare_revision_walk().)\n\nAdditionally, clarify in get_reference() that error messages are already\nbeing printed by the functions called (parse_object(),\nrepo_parse_commit(), and parse_commit_buffer() - invoked by the latter).\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\nChanges from v1: Improved code comments and commit message\n\n> This is the case where oid must be COMMIT from oid_object_info()'s\n> point of view, but repo_parse_commit() finds it as a non-commit, and\n> object becomes NULL.  This is quite different from the normal lazy\n> clone case where exclude-promisor-objects etc. wants to cover, that\n> the object whose name is oid is truly missing because it can be\n> fetched later from elsewhere.  Instead, we have found that there is\n> an inconsistency in the data we have about the object, iow, a\n> possible corruption.\n\nThanks! I should have looked at the first half of get_reference() more\ncarefully.\n\nIf there is corruption in the form of hash mismatch, parse_object() will\nprint a message and then return NULL, leaving get_reference() to handle\nit - and treat it as missing in this case. It seems reasonable to me to\nhandle the repo_parse_commit() failure in a similar way. I've added\ncomments to clarify that error messages are being printed.\n---\n revision.c               | 23 ++++++++++++++++++++++-\n t/t0410-partial-clone.sh | 10 ++--------\n 2 files changed, 24 insertions(+), 9 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 8136929e23..af1e31b4fc 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -370,8 +370,18 @@ static struct object *get_reference(struct rev_info *revs, const char *name,\n \t\tif (!repo_parse_commit(revs->repo, c))\n \t\t\tobject = (struct object *) c;\n \t\telse\n+\t\t\t/*\n+\t\t\t * There is something wrong with the commit.\n+\t\t\t * repo_parse_commit() will have already printed an\n+\t\t\t * error message. For our purposes, treat as missing.\n+\t\t\t */\n \t\t\tobject = NULL;\n \t} else {\n+\t\t/*\n+\t\t * There is something wrong with the object. parse_object()\n+\t\t * will have already printed an error message. For our\n+\t\t * purposes, treat as missing.\n+\t\t */\n \t\tobject = parse_object(revs->repo, oid);\n \t}\n \n@@ -1907,7 +1917,18 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi\n \t\tverify_non_filename(revs->prefix, arg);\n \tobject = get_reference(revs, arg, &oid, flags ^ local_flags);\n \tif (!object)\n-\t\treturn revs->ignore_missing ? 0 : -1;\n+\t\t/*\n+\t\t * If this object is corrupt, get_reference() prints an error\n+\t\t * message and treats it as missing.\n+\t\t *\n+\t\t * get_reference() returns NULL only if this object is missing\n+\t\t * and ignore_missing is true, or this object is a (missing)\n+\t\t * promisor object and exclude_promisor_objects is true. In\n+\t\t * both these cases, we can safely ignore this object because\n+\t\t * this object will not appear in output and cannot be used as\n+\t\t * a source of UNINTERESTING ancestors (since it is missing).\n+\t\t */\n+\t\treturn 0;\n \tadd_rev_cmdline(revs, object, arg_, REV_CMD_REV, flags ^ local_flags);\n \tadd_pending_object_with_path(revs, object, arg, oc.mode, oc.path);\n \tfree(oc.path);\ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex a3988bd4b8..fd28f5402a 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -416,15 +416,9 @@ test_expect_success 'rev-list dies for missing objects on cmd line' '\n \tgit -C repo config extensions.partialclone \"arbitrary string\" &&\n \n \tfor OBJ in \"$COMMIT\" \"$TREE\" \"$BLOB\"; do\n-\t\ttest_must_fail git -C repo rev-list --objects \\\n+\t\tgit -C repo rev-list --objects \\\n \t\t\t--exclude-promisor-objects \"$OBJ\" &&\n-\t\ttest_must_fail git -C repo rev-list --objects-edge-aggressive \\\n-\t\t\t--exclude-promisor-objects \"$OBJ\" &&\n-\n-\t\t# Do not die or crash when --ignore-missing is passed.\n-\t\tgit -C repo rev-list --ignore-missing --objects \\\n-\t\t\t--exclude-promisor-objects \"$OBJ\" &&\n-\t\tgit -C repo rev-list --ignore-missing --objects-edge-aggressive \\\n+\t\tgit -C repo rev-list --objects-edge-aggressive \\\n \t\t\t--exclude-promisor-objects \"$OBJ\"\n \tdone\n '\n-- \n2.24.1.735.g03f4e72817-goog\n\n"},{"id":"389120","messageId":"20191231000957.GB13606@google.com","threadId":"52531","inReplyTo":"20191230234453.255082-1-jonathantanmy@google.com","subject":"Re: [PATCH v2] revision: allow missing promisor objects on CLI","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2019-12-31T00:09:57Z","receivedAt":"2019-12-31T00:10:02Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Tan wrote:\n\n> Commit 4cf67869b2 (\"list-objects.c: don't segfault for missing cmdline\n> objects\", 2018-12-06) prevented some segmentation faults from occurring\n> by tightening handling of missing objects provided through the CLI: if\n> --ignore-missing is set, then it is OK (and the missing object ignored,\n> just like one would if encountered in traversal).\n>\n> However, in the case that --ignore-missing is not set but\n> --exclude-promisor-objects is set, there is still no distinction between\n> the case wherein the missing object is a promisor object and the case\n> wherein it is not. This is unnecessarily restrictive, since if a missing\n> promisor object is encountered in traversal, it is ignored; likewise it\n> should be ignored if provided through the CLI. Therefore, distinguish\n> between these 2 cases. (As a bonus, the code is now simpler.)\n\nnit about tenses, not worth a reroll on its own: \"As a bonus, this\nmakes the code simpler\" (since commit messages describe the status quo\nbefore the patch in the present tense).\n\n[...]\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -370,8 +370,18 @@ static struct object *get_reference(struct rev_info *revs, const char *name,\n>  \t\tif (!repo_parse_commit(revs->repo, c))\n>  \t\t\tobject = (struct object *) c;\n>  \t\telse\n> +\t\t\t/*\n> +\t\t\t * There is something wrong with the commit.\n> +\t\t\t * repo_parse_commit() will have already printed an\n> +\t\t\t * error message. For our purposes, treat as missing.\n> +\t\t\t */\n>  \t\t\tobject = NULL;\n>  \t} else {\n> +\t\t/*\n> +\t\t * There is something wrong with the object. parse_object()\n\nIf we got here, we are in the !commit case, which is not inherently wrong,\nright?  Is the intent something like the following?\n\n\tif (oid_object_info(...) == OBJ_COMMIT && !repo_parse_commit(...)) {\n\t\t... good ...\n\t} else if (parse_object(...)) {\n\t\t... good ...\n\t} else {\n\t\t/*\n\t\t * An error occured while parsing the object.\n\t\t * parse_commit or parse_object has already printed an\n\t\t * error message.  For our purposes, it's safe to\n\t\t * assume the object as missing.\n\t\t */\n\t\tobject = NULL;\n\t}\n\nThis might be easiest to understand as a separate mini-function.\nSomething like\n\n /*\n  * Like parse_object, but optimized by reading commits from the\n  * commit graph.\n  *\n  * If the repository has commit graphs, repo_parse_commit() avoids\n  * reading the object buffer, so use it whenever possible.\n  */\n static struct object *parse_object_probably_commit(\n\t\tstruct repository *r, const struct object_id *oid)\n {\n\tstruct commit *c;\n\n\tif (oid_object_info(r, oid, NULL) != OBJ_COMMIT)\n\t\treturn parse_object(r, oid);\n\n\tc = lookup_commit(r, oid);\n\tif (repo_parse_commit(r, c))\n\t\treturn NULL;\n\treturn (struct object *) c;\n }\n\n static struct object *get_reference(struct rev_info *revs, ...)\n {\n\tstruct object *object = parse_object_probably_commit(revs->repo, oid);\n\tif (!object)\n\t\t/*\n\t\t * An error occured while parsing the object.\n\t\t * parse_object_probably_commit has already printed an\n\t\t * error message.  For our purposes, it's safe to\n\t\t * assume the object as missing.\n\t\t */\n\t\t;\n\nBy the way, why doesn't parse_object itself check the commit graph for\ncommit objects?\n\n[...]\n> @@ -1907,7 +1917,18 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi\n>  \t\tverify_non_filename(revs->prefix, arg);\n>  \tobject = get_reference(revs, arg, &oid, flags ^ local_flags);\n>  \tif (!object)\n> -\t\treturn revs->ignore_missing ? 0 : -1;\n> +\t\t/*\n> +\t\t * If this object is corrupt, get_reference() prints an error\n> +\t\t * message and treats it as missing.\n\nBy \"and treats it as missing\" does this mean \"and we should treat it\nas missing\"?  (Forgive my ignorance.)\n\nWhy do we treat corrupt objects as missing?  Is this for graceful\ndegredation when trying to recover data from a corrupt repository?\n\nThanks,\nJonathan\n"},{"id":"389178","messageId":"20200102204917.188987-1-jonathantanmy@google.com","threadId":"52531","inReplyTo":"20191231000957.GB13606@google.com","subject":"Re: [PATCH v2] revision: allow missing promisor objects on CLI","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-01-02T20:49:17Z","receivedAt":"2020-01-02T20:49:22Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> > However, in the case that --ignore-missing is not set but\n> > --exclude-promisor-objects is set, there is still no distinction between\n> > the case wherein the missing object is a promisor object and the case\n> > wherein it is not. This is unnecessarily restrictive, since if a missing\n> > promisor object is encountered in traversal, it is ignored; likewise it\n> > should be ignored if provided through the CLI. Therefore, distinguish\n> > between these 2 cases. (As a bonus, the code is now simpler.)\n> \n> nit about tenses, not worth a reroll on its own: \"As a bonus, this\n> makes the code simpler\" (since commit messages describe the status quo\n> before the patch in the present tense).\n\nOK.\n\n> > --- a/revision.c\n> > +++ b/revision.c\n> > @@ -370,8 +370,18 @@ static struct object *get_reference(struct rev_info *revs, const char *name,\n> >  \t\tif (!repo_parse_commit(revs->repo, c))\n> >  \t\t\tobject = (struct object *) c;\n> >  \t\telse\n> > +\t\t\t/*\n> > +\t\t\t * There is something wrong with the commit.\n> > +\t\t\t * repo_parse_commit() will have already printed an\n> > +\t\t\t * error message. For our purposes, treat as missing.\n> > +\t\t\t */\n> >  \t\t\tobject = NULL;\n> >  \t} else {\n> > +\t\t/*\n> > +\t\t * There is something wrong with the object. parse_object()\n> \n> If we got here, we are in the !commit case, which is not inherently wrong,\n> right?\n\n[snip]\n\nAh, good catch. It should be \"If parse_object() returns NULL, ...\"\n\n> By the way, why doesn't parse_object itself check the commit graph for\n> commit objects?\n\nBecause that's how I coded it in ec0c5798ee (\"revision: use commit graph\nin get_reference()\", 2018-12-28). In the commit message, I said:\n\n> Another way to accomplish this effect would be to modify parse_object()\n> to use the commit graph if possible; however, I did not want to change\n> parse_object()'s current behavior of always checking the object\n> signature of the returned object.\n\nBut that doesn't mean that we cannot change it.\n\n> By \"and treats it as missing\" does this mean \"and we should treat it\n> as missing\"?  (Forgive my ignorance.)\n\nI don't fully know if we should, hence my specific wording :-P but see\nmy answer to your next question.\n\n> Why do we treat corrupt objects as missing?  Is this for graceful\n> degredation when trying to recover data from a corrupt repository?\n\nThis (at least, treating wrong-hash objects the same as missing) has\nbeen true since acdeec62cb (\"Don't ever return corrupt objects from\n\"parse_object()\"\", 2007-03-20) (and that commit message has no\nexplanation). I think this is best - I consider corrupt objects to be\nsimilar to missing objects with respect to repository corruption, so for\nme it is logical to treat them the same way.\n"},{"id":"389618","messageId":"cover.1578781770.git.jonathantanmy@google.com","threadId":"52531","inReplyTo":"20191228003430.241283-1-jonathantanmy@google.com","subject":"[PATCH v3 0/2] Un-regress rev-list --exclude-promisor-objects","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-01-11T22:34:54Z","receivedAt":"2020-01-11T22:35:46Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"I took another look at this and tried to simplify things. The main\npoints were:\n\n - there is a real bug\n - it can be fixed by relying on get_reference() more\n    - but there was some discussion about what get_reference() does, so\n      I added some documentation first\n\nHopefully those main points were adequately conveyed in the new commit\nmessages, and I didn't oversimplify things.\n\nThere was some discussion about whether get_reference() should treat\ncorrupt objects as missing. After some thought, I think the best\nargument for doing so is that this has been its behavior for some time,\nand have wrote that in the first commit.\n\nJonathan Tan (2):\n  revision: document get_reference()\n  revision: un-regress --exclude-promisor-objects\n\n revision.c               | 12 +++++++++++-\n t/t0410-partial-clone.sh | 12 +++---------\n 2 files changed, 14 insertions(+), 10 deletions(-)\n\n-- \n2.25.0.rc1.283.g88dfdc4193-goog\n\n"},{"id":"389619","messageId":"6dce9c79be3b9264f832852e9068347f42cf3ee0.1578781770.git.jonathantanmy@google.com","threadId":"52531","inReplyTo":"cover.1578781770.git.jonathantanmy@google.com","subject":"[PATCH v3 1/2] revision: document get_reference()","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-01-11T22:34:55Z","receivedAt":"2020-01-11T22:35:46Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"In particular, document the behavior when the object is corrupt. The\nexisting behavior when parse_object() encounters a hash mismatch has\nbeen there since cc243c3ceb (\"show: --ignore-missing\", 2011-05-19), and\nthe existing behavior when the code disagrees on whether an object is a\ncommit has been there since ec0c5798ee (\"revision: use commit graph in\nget_reference()\", 2018-12-28).\n---\n revision.c | 10 ++++++++++\n 1 file changed, 10 insertions(+)\n\ndiff --git a/revision.c b/revision.c\nindex 8136929e23..91ca194388 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -355,6 +355,16 @@ void add_head_to_pending(struct rev_info *revs)\n \tadd_pending_object(revs, obj, \"HEAD\");\n }\n \n+/*\n+ * Returns the object corresponding to \"oid\" and sets the given flags on\n+ * it.\n+ *\n+ * If that object is missing or corrupt, this function returns NULL if\n+ * \"revs\" permits it (that is, if revs->ignore_missing is true or if\n+ * revs->exclude_promisor_objects is true and the object is a promisor\n+ * object), and dies otherwise. Note that corrupt objects are treated\n+ * like missing objects, to preserve existing behavior.\n+ */\n static struct object *get_reference(struct rev_info *revs, const char *name,\n \t\t\t\t    const struct object_id *oid,\n \t\t\t\t    unsigned int flags)\n-- \n2.25.0.rc1.283.g88dfdc4193-goog\n\n"},{"id":"389620","messageId":"af19f5486f87f23e2a0c390de2d8710cdbad2d49.1578781770.git.jonathantanmy@google.com","threadId":"52531","inReplyTo":"cover.1578781770.git.jonathantanmy@google.com","subject":"[PATCH v3 2/2] revision: un-regress --exclude-promisor-objects","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-01-11T22:34:56Z","receivedAt":"2020-01-11T22:35:46Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Before commit 4cf67869b2 (\"list-objects.c: don't segfault for missing\ncmdline objects\", 2018-12-06),\n\n  git rev-list --exclude-promisor-objects $A_MISSING_PROMISOR_OBJECT\n\nsucceeds. But after that commit, this invocation produces a non-zero\nresult.\n\nRestore this functionality: since get_reference() already does what we\nneed, we can just use its return value; skip the arg if the return value\nis NULL, and use it otherwise (if the arg is invalid, get_reference()\nalready dies). With this commit, --exclude-promisor-objects treats both\npromisor objects passed through the CLI and promisor objects found\nthrough traversal in the same say: it excludes them, so it does not\nmatter whether they're missing or not.\n---\n revision.c               |  2 +-\n t/t0410-partial-clone.sh | 12 +++---------\n 2 files changed, 4 insertions(+), 10 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 91ca194388..0659a09b02 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1917,7 +1917,7 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi\n \t\tverify_non_filename(revs->prefix, arg);\n \tobject = get_reference(revs, arg, &oid, flags ^ local_flags);\n \tif (!object)\n-\t\treturn revs->ignore_missing ? 0 : -1;\n+\t\treturn 0;\n \tadd_rev_cmdline(revs, object, arg_, REV_CMD_REV, flags ^ local_flags);\n \tadd_pending_object_with_path(revs, object, arg, oc.mode, oc.path);\n \tfree(oc.path);\ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex a3988bd4b8..b251985e82 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -397,7 +397,7 @@ test_expect_success 'rev-list stops traversal at promisor commit, tree, and blob\n \tgrep $(git -C repo rev-parse bar) out  # sanity check that some walking was done\n '\n \n-test_expect_success 'rev-list dies for missing objects on cmd line' '\n+test_expect_success 'rev-list accepts missing and promised objects on command line ' '\n \trm -rf repo &&\n \ttest_create_repo repo &&\n \ttest_commit -C repo foo &&\n@@ -416,15 +416,9 @@ test_expect_success 'rev-list dies for missing objects on cmd line' '\n \tgit -C repo config extensions.partialclone \"arbitrary string\" &&\n \n \tfor OBJ in \"$COMMIT\" \"$TREE\" \"$BLOB\"; do\n-\t\ttest_must_fail git -C repo rev-list --objects \\\n+\t\tgit -C repo rev-list --objects \\\n \t\t\t--exclude-promisor-objects \"$OBJ\" &&\n-\t\ttest_must_fail git -C repo rev-list --objects-edge-aggressive \\\n-\t\t\t--exclude-promisor-objects \"$OBJ\" &&\n-\n-\t\t# Do not die or crash when --ignore-missing is passed.\n-\t\tgit -C repo rev-list --ignore-missing --objects \\\n-\t\t\t--exclude-promisor-objects \"$OBJ\" &&\n-\t\tgit -C repo rev-list --ignore-missing --objects-edge-aggressive \\\n+\t\tgit -C repo rev-list --objects-edge-aggressive \\\n \t\t\t--exclude-promisor-objects \"$OBJ\"\n \tdone\n '\n-- \n2.25.0.rc1.283.g88dfdc4193-goog\n\n"},{"id":"394008","messageId":"20200325204617.GA53368@google.com","threadId":"52531","inReplyTo":"6dce9c79be3b9264f832852e9068347f42cf3ee0.1578781770.git.jonathantanmy@google.com","subject":"Re: [PATCH v3 1/2] revision: document get_reference()","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2020-03-25T20:46:17Z","receivedAt":"2020-03-25T20:46:29Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Sat, Jan 11, 2020 at 02:34:55PM -0800, Jonathan Tan wrote:\n> In particular, document the behavior when the object is corrupt. The\n> existing behavior when parse_object() encounters a hash mismatch has\n> been there since cc243c3ceb (\"show: --ignore-missing\", 2011-05-19), and\n> the existing behavior when the code disagrees on whether an object is a\n> commit has been there since ec0c5798ee (\"revision: use commit graph in\n> get_reference()\", 2018-12-28).\n\nDoes \"disagreement on whether an object is a commit\" count as corrupt\nobject? Otherwise I'm not seeing mention of it in the comment that was\nadded.\n\n> ---\n>  revision.c | 10 ++++++++++\n>  1 file changed, 10 insertions(+)\n> \n> diff --git a/revision.c b/revision.c\n> index 8136929e23..91ca194388 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -355,6 +355,16 @@ void add_head_to_pending(struct rev_info *revs)\n>  \tadd_pending_object(revs, obj, \"HEAD\");\n>  }\n>  \n> +/*\n> + * Returns the object corresponding to \"oid\" and sets the given flags on\n> + * it.\n> + *\n> + * If that object is missing or corrupt, this function returns NULL if\n> + * \"revs\" permits it (that is, if revs->ignore_missing is true or if\n> + * revs->exclude_promisor_objects is true and the object is a promisor\n> + * object), and dies otherwise. Note that corrupt objects are treated\n\nThe parenthetical is hard to parse. Is it \"(revs->ignore_missing is\ntrue) or (revs->exclude_promisor_objects is true and the object is a\npromisor)\" or something else? Maybe an extra comma,\n\n  (that is, if revs->ignore_missing is true, or if\n  revs->exclude_promisor_objects is true and the object is a promisor\n  object)\n\n> + * like missing objects, to preserve existing behavior.\n> + */\n>  static struct object *get_reference(struct rev_info *revs, const char *name,\n>  \t\t\t\t    const struct object_id *oid,\n>  \t\t\t\t    unsigned int flags)\n> -- \n> 2.25.0.rc1.283.g88dfdc4193-goog\n> \n"},{"id":"394009","messageId":"20200325205035.GB53368@google.com","threadId":"52531","inReplyTo":"af19f5486f87f23e2a0c390de2d8710cdbad2d49.1578781770.git.jonathantanmy@google.com","subject":"Re: [PATCH v3 2/2] revision: un-regress --exclude-promisor-objects","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2020-03-25T20:50:35Z","receivedAt":"2020-03-25T20:50:43Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Sat, Jan 11, 2020 at 02:34:56PM -0800, Jonathan Tan wrote:\n> Before commit 4cf67869b2 (\"list-objects.c: don't segfault for missing\n> cmdline objects\", 2018-12-06),\n> \n>   git rev-list --exclude-promisor-objects $A_MISSING_PROMISOR_OBJECT\n> \n> succeeds. But after that commit, this invocation produces a non-zero\n> result.\n> \n> Restore this functionality: since get_reference() already does what we\n> need, we can just use its return value; skip the arg if the return value\n> is NULL, and use it otherwise (if the arg is invalid, get_reference()\n> already dies). With this commit, --exclude-promisor-objects treats both\n> promisor objects passed through the CLI and promisor objects found\n> through traversal in the same say: it excludes them, so it does not\n> matter whether they're missing or not.\n\nSince the return code is changing I'm kind of worried about what other\nimpacts this will have. What's the call tree for handle_revision_arg\nlook like?\n\nIt looks like it's called in a couple places by revision.c and by\nbuiltin/pack-objects.c, but because of the way args are packed up in\nstruct rev_info, it's hard to tell when those callers will be affected\nor not.\n\n> ---\n>  revision.c               |  2 +-\n>  t/t0410-partial-clone.sh | 12 +++---------\n>  2 files changed, 4 insertions(+), 10 deletions(-)\n> \n> diff --git a/revision.c b/revision.c\n> index 91ca194388..0659a09b02 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -1917,7 +1917,7 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi\n>  \t\tverify_non_filename(revs->prefix, arg);\n>  \tobject = get_reference(revs, arg, &oid, flags ^ local_flags);\n>  \tif (!object)\n> -\t\treturn revs->ignore_missing ? 0 : -1;\n> +\t\treturn 0;\n>  \tadd_rev_cmdline(revs, object, arg_, REV_CMD_REV, flags ^ local_flags);\n>  \tadd_pending_object_with_path(revs, object, arg, oc.mode, oc.path);\n>  \tfree(oc.path);\n> diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\n> index a3988bd4b8..b251985e82 100755\n> --- a/t/t0410-partial-clone.sh\n> +++ b/t/t0410-partial-clone.sh\n> @@ -397,7 +397,7 @@ test_expect_success 'rev-list stops traversal at promisor commit, tree, and blob\n>  \tgrep $(git -C repo rev-parse bar) out  # sanity check that some walking was done\n>  '\n>  \n> -test_expect_success 'rev-list dies for missing objects on cmd line' '\n> +test_expect_success 'rev-list accepts missing and promised objects on command line ' '\n>  \trm -rf repo &&\n>  \ttest_create_repo repo &&\n>  \ttest_commit -C repo foo &&\n> @@ -416,15 +416,9 @@ test_expect_success 'rev-list dies for missing objects on cmd line' '\n>  \tgit -C repo config extensions.partialclone \"arbitrary string\" &&\n>  \n>  \tfor OBJ in \"$COMMIT\" \"$TREE\" \"$BLOB\"; do\n> -\t\ttest_must_fail git -C repo rev-list --objects \\\n> +\t\tgit -C repo rev-list --objects \\\n>  \t\t\t--exclude-promisor-objects \"$OBJ\" &&\n> -\t\ttest_must_fail git -C repo rev-list --objects-edge-aggressive \\\n> -\t\t\t--exclude-promisor-objects \"$OBJ\" &&\n> -\n> -\t\t# Do not die or crash when --ignore-missing is passed.\n> -\t\tgit -C repo rev-list --ignore-missing --objects \\\n> -\t\t\t--exclude-promisor-objects \"$OBJ\" &&\n> -\t\tgit -C repo rev-list --ignore-missing --objects-edge-aggressive \\\n> +\t\tgit -C repo rev-list --objects-edge-aggressive \\\n>  \t\t\t--exclude-promisor-objects \"$OBJ\"\n\nIt seems to me the -ignore-missing tests should still pass and therefore\nshouldn't be removed, no? But then I looked a little harder, and it\nlooks like before the test said, \"This call fails, but with\n--ignore-missing it does not fail\" - and now the test just says \"This\ncall does not fail\". So it looks OK to me.\n\n>  \tdone\n>  '\n> -- \n> 2.25.0.rc1.283.g88dfdc4193-goog\n> \n"}]}