{"thread":{"id":"49655","subject":"[PATCH] revision.c: drop missing objects from cmdline","startedAt":"2018-10-23T21:57:55Z","lastAt":"2018-12-06T01:12:50Z","messageCount":7,"participants":["Matthew DeVore","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"361333","messageId":"20181023215745.245333-1-matvore@google.com","threadId":"49655","inReplyTo":null,"subject":"[PATCH] revision.c: drop missing objects from cmdline","fromName":"Matthew DeVore","fromEmail":"matvore@google.com","sentAt":"2018-10-23T21:57:45Z","receivedAt":"2018-10-23T21:57:55Z","isPatch":true,"sender":{"key":"matvore@google.com","avatar":"https://avatars.githubusercontent.com/u/946637?v=4"},"body":"No code which reads cmdline in struct rev_info can handle NULL objects\nin cmdline.rev[i].item, so stop adding them to the cmdline.rev array.\nObjects in cmdline are NULL when the given object is promisor and\n--exclude-promisor-objects is enabled.\n\nThis new behavior avoids a segmentation fault in the added test case in\nt0410.\n\nWe could simply die if add_rev_cmdline is called with a NULL item,\n(rather than warn if --exclude-promisor-objects is set) but because the\namended test case already expects the command to finish successfully,\ndifference and show a warning. Note that this command:\n\n\tgit rev-list --objects --missing=print $missing_hash\n\nAlready fails with a \"fatal: bad object HASH\" message and this patch\ndoes not change that.\n\nSigned-off-by: Matthew DeVore <matvore@google.com>\n---\n revision.c               | 12 ++++++++++++\n t/t0410-partial-clone.sh | 11 ++++++++++-\n 2 files changed, 22 insertions(+), 1 deletion(-)\n\ndiff --git a/revision.c b/revision.c\nindex a1ddb9e11c..8724dca2e2 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1148,6 +1148,18 @@ static void add_rev_cmdline(struct rev_info *revs,\n \t\t\t    int whence,\n \t\t\t    unsigned flags)\n {\n+\tif (!item) {\n+\t\t/*\n+\t\t * item is likely a promisor object returned from get_reference.\n+\t\t */\n+\t\tif (revs->exclude_promisor_objects) {\n+\t\t\twarning(_(\"ignoring missing object %s\"), name);\n+\t\t\treturn;\n+\t\t} else {\n+\t\t\tdie(_(\"missing object %s\"), name);\n+\t\t}\n+\t}\n+\n \tstruct rev_cmdline_info *info = &revs->cmdline;\n \tunsigned int nr = info->nr;\n \ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex ba3887f178..e02cd3f818 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -366,7 +366,16 @@ test_expect_success 'rev-list accepts missing and promised objects on command li\n \n \tgit -C repo config core.repositoryformatversion 1 &&\n \tgit -C repo config extensions.partialclone \"arbitrary string\" &&\n-\tgit -C repo rev-list --exclude-promisor-objects --objects \"$COMMIT\" \"$TREE\" \"$BLOB\"\n+\n+\tprintf \"warning: ignoring missing object %s\\n\" \\\n+\t       \"$COMMIT\" \"$TREE\" \"$BLOB\" >expect &&\n+\tgit -C repo rev-list --objects \\\n+\t\t--exclude-promisor-objects \"$COMMIT\" \"$TREE\" \"$BLOB\" 2>actual &&\n+\ttest_cmp expect actual &&\n+\n+\tgit -C repo rev-list --objects-edge-aggressive \\\n+\t\t--exclude-promisor-objects \"$COMMIT\" \"$TREE\" \"$BLOB\" 2>actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success 'gc repacks promisor objects separately from non-promisor objects' '\n-- \n2.19.1.568.g152ad8e336-goog\n\n"},{"id":"361361","messageId":"xmqqa7n4osgi.fsf@gitster-ct.c.googlers.com","threadId":"49655","inReplyTo":"20181023215745.245333-1-matvore@google.com","subject":"Re: [PATCH] revision.c: drop missing objects from cmdline","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-24T04:54:05Z","receivedAt":"2018-10-24T04:54:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthew DeVore <matvore@google.com> writes:\n\n> No code which reads cmdline in struct rev_info can handle NULL objects\n> in cmdline.rev[i].item, so stop adding them to the cmdline.rev array.\n\n\"The code is not prepared to have cmdline.rev[].item that is NULL\"\nis something everybody would understand and agree with, but that\ndoes not automatically lead to \"so ignoring or rejecting and dying\nis OK\", though.  The cmdline thing is used for the commands to learn\nthe end-user intent that cannot be learned by the resulting objects\nin the object array (e.g. the user may have said 'master' but the\npending[] (and later revs.commits) would only have the object names,\nand some callers would want to know if it was a branch name, a\nrefname refs/heads/master, or the hexadecimal object name), so\nunless absolutely needed, I'm hesitant to take a change that loses\ninformation (e.g. the user named this object that is not locally\navailable, we cannot afford to add it to the pending[] and add it to\nrevs.commits to traverse from there, but we still want to know what\nobject was given by the user).\n\n> Objects in cmdline are NULL when the given object is promisor and\n> --exclude-promisor-objects is enabled.\n\nA \"promisor\" is a remote repository.  It promises certain objects\nthat you do not have are later retrievable from it.  The way you can\nsee if the promisor promised to later give you an object is to see\nif that missing object is reachable from an object in a packfile the\npromisor gave you earlier.  \n\n\"The given object\" is never a \"promisor\", so I am not sure what the\nabove wants to say.  Is \n\n    When an object is given on the command line and if it is missing\n    from the local repository, add_rev_cmdline() receives NULL in\n    its \"item\" parameter.\n\nwhat you meant?  Is that the _only_ case in which \"item\" could be\nNULL, or is it also true for any missing object due to repository\ncorruption?\n"},{"id":"361567","messageId":"CAMfpvhJ4_5EtcTiaU6T1T9qPd=kBBvcVpaUFC_AAy4VBo7hv5w@mail.gmail.com","threadId":"49655","inReplyTo":"xmqqa7n4osgi.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] revision.c: drop missing objects from cmdline","fromName":"Matthew DeVore","fromEmail":"matvore@google.com","sentAt":"2018-10-25T23:13:40Z","receivedAt":"2018-10-25T23:13:55Z","isPatch":true,"sender":{"key":"matvore@google.com","avatar":"https://avatars.githubusercontent.com/u/946637?v=4"},"body":"On Tue, Oct 23, 2018 at 9:54 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Matthew DeVore <matvore@google.com> writes:\n>\n> > No code which reads cmdline in struct rev_info can handle NULL objects\n> > in cmdline.rev[i].item, so stop adding them to the cmdline.rev array.\n>\n> \"The code is not prepared to have cmdline.rev[].item that is NULL\"\n> is something everybody would understand and agree with, but that\n> does not automatically lead to \"so ignoring or rejecting and dying\n> is OK\", though.  The cmdline thing is used for the commands to learn\n> the end-user intent that cannot be learned by the resulting objects\n> in the object array (e.g. the user may have said 'master' but the\n> pending[] (and later revs.commits) would only have the object names,\n> and some callers would want to know if it was a branch name, a\n> refname refs/heads/master, or the hexadecimal object name), so\n> unless absolutely needed, I'm hesitant to take a change that loses\n> information (e.g. the user named this object that is not locally\n> available, we cannot afford to add it to the pending[] and add it to\n> revs.commits to traverse from there, but we still want to know what\n> object was given by the user).\nHmm, when you explain the purpose of cmdline, it's obvious now that it\ndoesn't make sense to mechanically drop items from it. I'm sending\nanother version of this patch which uses a more focused approach and\nis a bit simpler.\n\n>\n> > Objects in cmdline are NULL when the given object is promisor and\n> > --exclude-promisor-objects is enabled.\n>\n> A \"promisor\" is a remote repository.  It promises certain objects\n> that you do not have are later retrievable from it.  The way you can\n> see if the promisor promised to later give you an object is to see\n> if that missing object is reachable from an object in a packfile the\n> promisor gave you earlier.\n>\n> \"The given object\" is never a \"promisor\", so I am not sure what the\n> above wants to say.  Is\n>\n>     When an object is given on the command line and if it is missing\n>     from the local repository, add_rev_cmdline() receives NULL in\n>     its \"item\" parameter.\n>\n> what you meant?  Is that the _only_ case in which \"item\" could be\n> NULL, or is it also true for any missing object due to repository\n> corruption?\n\nYes, that is what I meant. I believe for corruption there is an actual\nerror shown with die() or the like, though I am not certain.\n"},{"id":"361580","messageId":"20181025235314.63495-1-matvore@google.com","threadId":"49655","inReplyTo":"20181023215745.245333-1-matvore@google.com","subject":"[PATCH v2] list-objects.c: don't segfault for missing cmdline objects","fromName":"Matthew DeVore","fromEmail":"matvore@google.com","sentAt":"2018-10-25T23:53:14Z","receivedAt":"2018-10-25T23:53:21Z","isPatch":true,"sender":{"key":"matvore@google.com","avatar":"https://avatars.githubusercontent.com/u/946637?v=4"},"body":"When a command is invoked with both --exclude-promisor-objects,\n--objects-edge-aggressive, and a missing object on the command line,\nthe rev_info.cmdline array could get a NULL pointer for the value of\nan 'item' field. Prevent dereferencing of a NULL pointer in that\nsituation.\n\nThere are a few other places in the code where rev_info.cmdline is read\nand the code doesn't handle NULL objects, but I couldn't prove to myself\nthat any of them needed to change except this one (since it may not\nactually be possible to reach the other code paths with\nrev_info.cmdline[] set to NULL).\n\nSigned-off-by: Matthew DeVore <matvore@google.com>\n---\n list-objects.c           | 3 ++-\n t/t0410-partial-clone.sh | 6 +++++-\n 2 files changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/list-objects.c b/list-objects.c\nindex c41cc80db5..27ed2c6cab 100644\n--- a/list-objects.c\n+++ b/list-objects.c\n@@ -245,7 +245,8 @@ void mark_edges_uninteresting(struct rev_info *revs, show_edge_fn show_edge)\n \t\tfor (i = 0; i < revs->cmdline.nr; i++) {\n \t\t\tstruct object *obj = revs->cmdline.rev[i].item;\n \t\t\tstruct commit *commit = (struct commit *)obj;\n-\t\t\tif (obj->type != OBJ_COMMIT || !(obj->flags & UNINTERESTING))\n+\t\t\tif (!obj || obj->type != OBJ_COMMIT ||\n+\t\t\t    !(obj->flags & UNINTERESTING))\n \t\t\t\tcontinue;\n \t\t\tmark_tree_uninteresting(revs->repo,\n \t\t\t\t\t\tget_commit_tree(commit));\ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex ba3887f178..e52291e674 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -366,7 +366,11 @@ test_expect_success 'rev-list accepts missing and promised objects on command li\n \n \tgit -C repo config core.repositoryformatversion 1 &&\n \tgit -C repo config extensions.partialclone \"arbitrary string\" &&\n-\tgit -C repo rev-list --exclude-promisor-objects --objects \"$COMMIT\" \"$TREE\" \"$BLOB\"\n+\n+\tgit -C repo rev-list --objects \\\n+\t\t--exclude-promisor-objects \"$COMMIT\" \"$TREE\" \"$BLOB\" &&\n+\tgit -C repo rev-list --objects-edge-aggressive \\\n+\t\t--exclude-promisor-objects \"$COMMIT\" \"$TREE\" \"$BLOB\"\n '\n \n test_expect_success 'gc repacks promisor objects separately from non-promisor objects' '\n-- \n2.19.1.568.g152ad8e336-goog\n\n"},{"id":"361853","messageId":"xmqqo9bdaa63.fsf@gitster-ct.c.googlers.com","threadId":"49655","inReplyTo":"20181025235314.63495-1-matvore@google.com","subject":"Re: [PATCH v2] list-objects.c: don't segfault for missing cmdline objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-29T00:06:28Z","receivedAt":"2018-10-29T00:17:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthew DeVore <matvore@google.com> writes:\n\n> When a command is invoked with both --exclude-promisor-objects,\n> --objects-edge-aggressive, and a missing object on the command line,\n> the rev_info.cmdline array could get a NULL pointer for the value of\n> an 'item' field. Prevent dereferencing of a NULL pointer in that\n> situation.\n\nThanks.\n\n> There are a few other places in the code where rev_info.cmdline is read\n> and the code doesn't handle NULL objects, but I couldn't prove to myself\n> that any of them needed to change except this one (since it may not\n> actually be possible to reach the other code paths with\n> rev_info.cmdline[] set to NULL).\n>\n> Signed-off-by: Matthew DeVore <matvore@google.com>\n> ---\n>  list-objects.c           | 3 ++-\n>  t/t0410-partial-clone.sh | 6 +++++-\n>  2 files changed, 7 insertions(+), 2 deletions(-)\n>\n> diff --git a/list-objects.c b/list-objects.c\n> index c41cc80db5..27ed2c6cab 100644\n> --- a/list-objects.c\n> +++ b/list-objects.c\n> @@ -245,7 +245,8 @@ void mark_edges_uninteresting(struct rev_info *revs, show_edge_fn show_edge)\n>  \t\tfor (i = 0; i < revs->cmdline.nr; i++) {\n>  \t\t\tstruct object *obj = revs->cmdline.rev[i].item;\n>  \t\t\tstruct commit *commit = (struct commit *)obj;\n> -\t\t\tif (obj->type != OBJ_COMMIT || !(obj->flags & UNINTERESTING))\n> +\t\t\tif (!obj || obj->type != OBJ_COMMIT ||\n> +\t\t\t    !(obj->flags & UNINTERESTING))\n>  \t\t\t\tcontinue;\n>  \t\t\tmark_tree_uninteresting(revs->repo,\n>  \t\t\t\t\t\tget_commit_tree(commit));\n> diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\n> index ba3887f178..e52291e674 100755\n> --- a/t/t0410-partial-clone.sh\n> +++ b/t/t0410-partial-clone.sh\n> @@ -366,7 +366,11 @@ test_expect_success 'rev-list accepts missing and promised objects on command li\n>  \n>  \tgit -C repo config core.repositoryformatversion 1 &&\n>  \tgit -C repo config extensions.partialclone \"arbitrary string\" &&\n> -\tgit -C repo rev-list --exclude-promisor-objects --objects \"$COMMIT\" \"$TREE\" \"$BLOB\"\n> +\n> +\tgit -C repo rev-list --objects \\\n> +\t\t--exclude-promisor-objects \"$COMMIT\" \"$TREE\" \"$BLOB\" &&\n> +\tgit -C repo rev-list --objects-edge-aggressive \\\n> +\t\t--exclude-promisor-objects \"$COMMIT\" \"$TREE\" \"$BLOB\"\n>  '\n>  \n>  test_expect_success 'gc repacks promisor objects separately from non-promisor objects' '\n"},{"id":"364648","messageId":"20181205214346.106217-1-matvore@google.com","threadId":"49655","inReplyTo":"20181023215745.245333-1-matvore@google.com","subject":"[PATCH v3] list-objects.c: don't segfault for missing cmdline objects","fromName":"Matthew DeVore","fromEmail":"matvore@google.com","sentAt":"2018-12-05T21:43:46Z","receivedAt":"2018-12-05T21:44:07Z","isPatch":true,"sender":{"key":"matvore@google.com","avatar":"https://avatars.githubusercontent.com/u/946637?v=4"},"body":"When a command is invoked with both --exclude-promisor-objects,\n--objects-edge-aggressive, and a missing object on the command line,\nthe rev_info.cmdline array could get a NULL pointer for the value of\nan 'item' field. Prevent dereferencing of a NULL pointer in that\nsituation.\n\nProperly handle --ignore-missing. If it is not passed, die when an\nobject is missing. Otherwise, just silently ignore it.\n\nSigned-off-by: Matthew DeVore <matvore@google.com>\n---\n revision.c               |  2 ++\n t/t0410-partial-clone.sh | 16 ++++++++++++++--\n 2 files changed, 16 insertions(+), 2 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 13e0519c02..293303b67d 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1729,6 +1729,8 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi\n \tif (!cant_be_filename)\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 \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 ba3887f178..169f7f10a7 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -349,7 +349,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 accepts missing and promised objects on command line' '\n+test_expect_success 'rev-list dies for missing objects on cmd line' '\n \trm -rf repo &&\n \ttest_create_repo repo &&\n \ttest_commit -C repo foo &&\n@@ -366,7 +366,19 @@ test_expect_success 'rev-list accepts missing and promised objects on command li\n \n \tgit -C repo config core.repositoryformatversion 1 &&\n \tgit -C repo config extensions.partialclone \"arbitrary string\" &&\n-\tgit -C repo rev-list --exclude-promisor-objects --objects \"$COMMIT\" \"$TREE\" \"$BLOB\"\n+\n+\tfor OBJ in \"$COMMIT\" \"$TREE\" \"$BLOB\"; do\n+\t\ttest_must_fail git -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\t\t--exclude-promisor-objects \"$OBJ\"\n+\tdone\n '\n \n test_expect_success 'gc repacks promisor objects separately from non-promisor objects' '\n-- \n2.20.0.rc1.387.gf8505762e3-goog\n\n"},{"id":"364667","messageId":"xmqqsgzba25x.fsf@gitster-ct.c.googlers.com","threadId":"49655","inReplyTo":"20181205214346.106217-1-matvore@google.com","subject":"Re: [PATCH v3] list-objects.c: don't segfault for missing cmdline objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-12-06T01:12:42Z","receivedAt":"2018-12-06T01:12:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthew DeVore <matvore@google.com> writes:\n\n> When a command is invoked with both --exclude-promisor-objects,\n> --objects-edge-aggressive, and a missing object on the command line,\n> the rev_info.cmdline array could get a NULL pointer for the value of\n> an 'item' field. Prevent dereferencing of a NULL pointer in that\n> situation.\n>\n> Properly handle --ignore-missing. If it is not passed, die when an\n> object is missing. Otherwise, just silently ignore it.\n>\n> Signed-off-by: Matthew DeVore <matvore@google.com>\n\nThanks for an update.  Will replace.\n\n> ---\n>  revision.c               |  2 ++\n>  t/t0410-partial-clone.sh | 16 ++++++++++++++--\n>  2 files changed, 16 insertions(+), 2 deletions(-)\n>\n> diff --git a/revision.c b/revision.c\n> index 13e0519c02..293303b67d 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -1729,6 +1729,8 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi\n>  \tif (!cant_be_filename)\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>  \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 ba3887f178..169f7f10a7 100755\n> --- a/t/t0410-partial-clone.sh\n> +++ b/t/t0410-partial-clone.sh\n> @@ -349,7 +349,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 accepts missing and promised objects on command line' '\n> +test_expect_success 'rev-list dies for missing objects on cmd line' '\n>  \trm -rf repo &&\n>  \ttest_create_repo repo &&\n>  \ttest_commit -C repo foo &&\n> @@ -366,7 +366,19 @@ test_expect_success 'rev-list accepts missing and promised objects on command li\n>  \n>  \tgit -C repo config core.repositoryformatversion 1 &&\n>  \tgit -C repo config extensions.partialclone \"arbitrary string\" &&\n> -\tgit -C repo rev-list --exclude-promisor-objects --objects \"$COMMIT\" \"$TREE\" \"$BLOB\"\n> +\n> +\tfor OBJ in \"$COMMIT\" \"$TREE\" \"$BLOB\"; do\n> +\t\ttest_must_fail git -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\t\t--exclude-promisor-objects \"$OBJ\"\n> +\tdone\n>  '\n>  \n>  test_expect_success 'gc repacks promisor objects separately from non-promisor objects' '\n"}]}