revision.c: drop missing objects from cmdline
PATCH 1/1 revision.c: drop missing objects from cmdline
No code which reads cmdline in struct rev_info can handle NULL objects in cmdline.rev[i].item, so stop adding them to the cmdline.rev array. Objects in cmdline are NULL when the given object is promisor and --exclude-promisor-objects is enabled.
This new behavior avoids a segmentation fault in the added test case in t0410.
We could simply die if add_rev_cmdline is called with a NULL item, (rather than warn if --exclude-promisor-objects is set) but because the amended test case already expects the command to finish successfully, difference and show a warning. Note that this command:
git rev-list --objects --missing=print $missing_hash
Already fails with a "fatal: bad object HASH" message and this patch does not change that.
--- revision.c | 12 ++++++++++++ t/t0410-partial-clone.sh | 11 ++++++++++- 2 files changed, 22 insertions(+), 1 deletion(-)
revision.c +12 -0
@@ -1148,6 +1148,18 @@ static void add_rev_cmdline(struct rev_info *revs, int whence, unsigned flags) { +if (!item) { +/* +* item is likely a promisor object returned from get_reference. +*/ +if (revs->exclude_promisor_objects) { +warning (_ ("ignoring missing object %s" ), name); +return ; + }else { +die (_ ("missing object %s" ), name); + } + } +struct rev_cmdline_info *info = &revs->cmdline;unsigned int nr = info->nr;
t/t0410-partial-clone.sh +10 -1
@@ -366,7 +366,16 @@ test_expect_success 'rev-list accepts missing and promised objects on command li git -C repo config core.repositoryformatversion 1 && git -C repo config extensions.partialclone "arbitrary string" && - git -C repo rev-list --exclude-promisor-objects --objects "$COMMIT" "$TREE" "$BLOB" + + printf "warning: ignoring missing object %s\n" \ + "$COMMIT" "$TREE" "$BLOB" >expect && + git -C repo rev-list --objects \ + --exclude-promisor-objects "$COMMIT" "$TREE" "$BLOB" 2>actual && + test_cmp expect actual && + + git -C repo rev-list --objects-edge-aggressive \ + --exclude-promisor-objects "$COMMIT" "$TREE" "$BLOB" 2>actual && + test_cmp expect actual ' test_expect_success 'gc repacks promisor objects separately from non-promisor objects' '
Signature · 1 line
2.19.1.568.g152ad8e336-goog
Quoted 2 lines · Matthew DeVore
No code which reads cmdline in struct
rev_infocan handle NULL objects in cmdline.rev[i].item, so stop adding them to the cmdline.rev array.
"The code is not prepared to have cmdline.rev[].item that is NULL" is something everybody would understand and agree with, but that does not automatically lead to "so ignoring or rejecting and dying is OK", though. The cmdline thing is used for the commands to learn the end-user intent that cannot be learned by the resulting objects in the object array (e.g. the user may have said 'master' but the pending[] (and later revs.commits) would only have the object names, and some callers would want to know if it was a branch name, a refname refs/heads/master, or the hexadecimal object name), so unless absolutely needed, I'm hesitant to take a change that loses information (e.g. the user named this object that is not locally available, we cannot afford to add it to the pending[] and add it to revs.commits to traverse from there, but we still want to know what object was given by the user).
Quoted 2 lines
Objects in cmdline are NULL when the given object is promisor and --exclude-promisor-objects is enabled.
A "promisor" is a remote repository. It promises certain objects that you do not have are later retrievable from it. The way you can see if the promisor promised to later give you an object is to see if that missing object is reachable from an object in a packfile the promisor gave you earlier.
"The given object" is never a "promisor", so I am not sure what the above wants to say. Is
When an object is given on the command line and if it is missing from the local repository, add_rev_cmdline() receives NULL in its "item" parameter.
what you meant? Is that the _only_ case in which "item" could be NULL, or is it also true for any missing object due to repository corruption?
Quoted 6 lines · Junio C Hamano
Quoted 2 lines · Matthew DeVore
No code which reads cmdline in struct
rev_infocan handle NULL objects in cmdline.rev[i].item, so stop adding them to the cmdline.rev array.
Quoted 14 lines
"The code is not prepared to have cmdline.rev[].item that is NULL" is something everybody would understand and agree with, but that does not automatically lead to "so ignoring or rejecting and dying is OK", though. The cmdline thing is used for the commands to learn the end-user intent that cannot be learned by the resulting objects in the object array (e.g. the user may have said 'master' but the pending[] (and later revs.commits) would only have the object names, and some callers would want to know if it was a branch name, a refname refs/heads/master, or the hexadecimal object name), so unless absolutely needed, I'm hesitant to take a change that loses information (e.g. the user named this object that is not locally available, we cannot afford to add it to the pending[] and add it to revs.commits to traverse from there, but we still want to know what object was given by the user).
Hmm, when you explain the purpose of cmdline, it's obvious now that it doesn't make sense to mechanically drop items from it. I'm sending another version of this patch which uses a more focused approach and is a bit simpler.
Quoted 13 lines
Quoted 3 lines
Objects in cmdline are NULL when the given object is promisor and --exclude-promisor-objects is enabled.
A "promisor" is a remote repository. It promises certain objects that you do not have are later retrievable from it. The way you can see if the promisor promised to later give you an object is to see if that missing object is reachable from an object in a packfile the promisor gave you earlier.
"The given object" is never a "promisor", so I am not sure what the above wants to say. Is
Quoted 7 lines
When an object is given on the command line and if it is missing from the local repository,
add_rev_cmdline()receives NULL in its "item" parameter.what you meant? Is that the _only_ case in which "item" could be NULL, or is it also true for any missing object due to repository corruption?
Yes, that is what I meant. I believe for corruption there is an actual error shown with die() or the like, though I am not certain.
PATCH v2 1/1 list-objects.c: don't segfault for missing cmdline objects
In reply to Matthew DeVoreWhen a command is invoked with both --exclude-promisor-objects, --objects-edge-aggressive, and a missing object on the command line, the rev_info.cmdline array could get a NULL pointer for the value of an 'item' field. Prevent dereferencing of a NULL pointer in that situation.
There are a few other places in the code where rev_info.cmdline is read and the code doesn't handle NULL objects, but I couldn't prove to myself that any of them needed to change except this one (since it may not actually be possible to reach the other code paths with rev_info.cmdline[] set to NULL).
--- list-objects.c | 3 ++- t/t0410-partial-clone.sh | 6 +++++- 2 files changed, 7 insertions(+), 2 deletions(-)
list-objects.c +2 -1
@@ -245,7 +245,8 @@ void mark_edges_uninteresting(struct rev_info *revs, show_edge_fn show_edge)for (i =0 ; i < revs->cmdline.nr; i++) {struct object *obj = revs->cmdline.rev[i].item;struct commit *commit = (struct commit *)obj; -if (obj->type !=OBJ_COMMIT || !(obj->flags &UNINTERESTING )) +if (!obj || obj->type !=OBJ_COMMIT || + !(obj->flags &UNINTERESTING ))continue ;mark_tree_uninteresting (revs->repo,get_commit_tree (commit));
t/t0410-partial-clone.sh +5 -1
@@ -366,7 +366,11 @@ test_expect_success 'rev-list accepts missing and promised objects on command li git -C repo config core.repositoryformatversion 1 && git -C repo config extensions.partialclone "arbitrary string" && - git -C repo rev-list --exclude-promisor-objects --objects "$COMMIT" "$TREE" "$BLOB" + + git -C repo rev-list --objects \ + --exclude-promisor-objects "$COMMIT" "$TREE" "$BLOB" && + git -C repo rev-list --objects-edge-aggressive \ + --exclude-promisor-objects "$COMMIT" "$TREE" "$BLOB" ' test_expect_success 'gc repacks promisor objects separately from non-promisor objects' '
Signature · 1 line
2.19.1.568.g152ad8e336-goog
Re: [PATCH v2] list-objects.c: don't segfault for missing cmdline objects
Quoted 5 lines · Matthew DeVore
When a command is invoked with both --exclude-promisor-objects, --objects-edge-aggressive, and a missing object on the command line, the rev_info.cmdline array could get a NULL pointer for the value of an 'item' field. Prevent dereferencing of a NULL pointer in that situation.
Thanks.
Quoted 43 lines
There are a few other places in the code where rev_info.cmdline is read and the code doesn't handle NULL objects, but I couldn't prove to myself that any of them needed to change except this one (since it may not actually be possible to reach the other code paths with rev_info.cmdline[] set to NULL).
Signed-off-by Matthew DeVore <[email protected]>--- list-objects.c | 3 ++- t/t0410-partial-clone.sh | 6 +++++- 2 files changed, 7 insertions(+), 2 deletions(-)list-objects.c +2
-1@@ -245,7 +245,8 @@ void mark_edges_uninteresting(struct rev_info *revs, show_edge_fn show_edge)for (i =0 ; i < revs->cmdline.nr; i++) {struct object *obj = revs->cmdline.rev[i].item;struct commit *commit = (struct commit *)obj; -if (obj->type !=OBJ_COMMIT || !(obj->flags &UNINTERESTING )) +if (!obj || obj->type !=OBJ_COMMIT || + !(obj->flags &UNINTERESTING ))continue ;mark_tree_uninteresting (revs->repo,get_commit_tree (commit));t/t0410-partial-clone.sh +5
-1@@ -366,7 +366,11 @@ test_expect_success 'rev-list accepts missing and promised objects on command li git -C repo config core.repositoryformatversion 1 && git -C repo config extensions.partialclone "arbitrary string" && - git -C repo rev-list --exclude-promisor-objects --objects "$COMMIT" "$TREE" "$BLOB" + + git -C repo rev-list --objects \ + --exclude-promisor-objects "$COMMIT" "$TREE" "$BLOB" && + git -C repo rev-list --objects-edge-aggressive \ + --exclude-promisor-objects "$COMMIT" "$TREE" "$BLOB" ' test_expect_success 'gc repacks promisor objects separately from non-promisor objects' '
PATCH v3 1/1 list-objects.c: don't segfault for missing cmdline objects
In reply to Matthew DeVoreWhen a command is invoked with both --exclude-promisor-objects, --objects-edge-aggressive, and a missing object on the command line, the rev_info.cmdline array could get a NULL pointer for the value of an 'item' field. Prevent dereferencing of a NULL pointer in that situation.
Properly handle --ignore-missing. If it is not passed, die when an object is missing. Otherwise, just silently ignore it.
--- revision.c | 2 ++ t/t0410-partial-clone.sh | 16 ++++++++++++++-- 2 files changed, 16 insertions(+), 2 deletions(-)
revision.c +2 -0
@@ -1729,6 +1729,8 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsiif (!cant_be_filename)verify_non_filename (revs->prefix, arg); object =get_reference (revs, arg, &oid, flags ^ local_flags); +if (!object) +return revs->ignore_missing ?0 :-1 ;add_rev_cmdline (revs, object, arg_,REV_CMD_REV , flags ^ local_flags);add_pending_object_with_path (revs, object, arg, oc.mode, oc.path);free (oc.path);
t/t0410-partial-clone.sh +14 -2
@@ -349,7 +349,7 @@ test_expect_success 'rev-list stops traversal at promisor commit, tree, and blob grep $(git -C repo rev-parse bar) out # sanity check that some walking was done ' -test_expect_success 'rev-list accepts missing and promised objects on command line' ' +test_expect_success 'rev-list dies for missing objects on cmd line' ' rm -rf repo && test_create_repo repo && test_commit -C repo foo &&
@@ -366,7 +366,19 @@ test_expect_success 'rev-list accepts missing and promised objects on command li git -C repo config core.repositoryformatversion 1 && git -C repo config extensions.partialclone "arbitrary string" && - git -C repo rev-list --exclude-promisor-objects --objects "$COMMIT" "$TREE" "$BLOB" + + for OBJ in "$COMMIT" "$TREE" "$BLOB"; do + test_must_fail git -C repo rev-list --objects \ + --exclude-promisor-objects "$OBJ" && + test_must_fail git -C repo rev-list --objects-edge-aggressive \ + --exclude-promisor-objects "$OBJ" && + + # Do not die or crash when --ignore-missing is passed. + git -C repo rev-list --ignore-missing --objects \ + --exclude-promisor-objects "$OBJ" && + git -C repo rev-list --ignore-missing --objects-edge-aggressive \ + --exclude-promisor-objects "$OBJ" + done ' test_expect_success 'gc repacks promisor objects separately from non-promisor objects' '
Signature · 1 line
2.20.0.rc1.387.gf8505762e3-goog
Re: [PATCH v3] list-objects.c: don't segfault for missing cmdline objects
Quoted 10 lines · Matthew DeVore
When a command is invoked with both --exclude-promisor-objects, --objects-edge-aggressive, and a missing object on the command line, the rev_info.cmdline array could get a NULL pointer for the value of an 'item' field. Prevent dereferencing of a NULL pointer in that situation.
Properly handle --ignore-missing. If it is not passed, die when an object is missing. Otherwise, just silently ignore it.
Signed-off-by Matthew DeVore <[email protected]>
Thanks for an update. Will replace.
Quoted 52 lines
--- revision.c | 2 ++ t/t0410-partial-clone.sh | 16 ++++++++++++++-- 2 files changed, 16 insertions(+), 2 deletions(-)revision.c +2
-0@@ -1729,6 +1729,8 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsiif (!cant_be_filename)verify_non_filename (revs->prefix, arg); object =get_reference (revs, arg, &oid, flags ^ local_flags); +if (!object) +return revs->ignore_missing ?0 :-1 ;add_rev_cmdline (revs, object, arg_,REV_CMD_REV , flags ^ local_flags);add_pending_object_with_path (revs, object, arg, oc.mode, oc.path);free (oc.path);t/t0410-partial-clone.sh +14
-2@@ -349,7 +349,7 @@ test_expect_success 'rev-list stops traversal at promisor commit, tree, and blob grep $(git -C repo rev-parse bar) out # sanity check that some walking was done ' -test_expect_success 'rev-list accepts missing and promised objects on command line' ' +test_expect_success 'rev-list dies for missing objects on cmd line' ' rm -rf repo && test_create_repo repo && test_commit -C repo foo &&@@ -366,7 +366,19 @@ test_expect_success 'rev-list accepts missing and promised objects on command li git -C repo config core.repositoryformatversion 1 && git -C repo config extensions.partialclone "arbitrary string" && - git -C repo rev-list --exclude-promisor-objects --objects "$COMMIT" "$TREE" "$BLOB" + + for OBJ in "$COMMIT" "$TREE" "$BLOB"; do + test_must_fail git -C repo rev-list --objects \ + --exclude-promisor-objects "$OBJ" && + test_must_fail git -C repo rev-list --objects-edge-aggressive \ + --exclude-promisor-objects "$OBJ" && + + # Do not die or crash when --ignore-missing is passed. + git -C repo rev-list --ignore-missing --objects \ + --exclude-promisor-objects "$OBJ" && + git -C repo rev-list --ignore-missing --objects-edge-aggressive \ + --exclude-promisor-objects "$OBJ" + done ' test_expect_success 'gc repacks promisor objects separately from non-promisor objects' '