| Message ID | 20200521020712.1620993-2-sandals@crustytoothpaste.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Improve Fix code coverage for checkout | expand |
"brian m. carlson" <sandals@crustytoothpaste.net> writes: > ... The only case in which we do not have a commit object is when > invoking git switch with --orphan. Moreover, we can only hit this code > path without a commit object additionally with either --force or > --discard-changes. It was easy for me to trace the codepath to see when these options are used we end up with no commit object, but I ran out of time trying to see if the "forced orphan" is the only way to end up with a NULL in new_branch_info->commit. Assuming that is true, of course the following perfectly makes sense. > In such a case, there is no point initializing the checkout metadata > with a commit or tree because (a) there is no commit, only the empty > tree, and (b) we will never use the data, since no files will be smudged > when checking out a branch with no files. Pass the all-zeros object ID > in this case, since we just need some value which is a valid pointer. > > Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> > --- > builtin/checkout.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) Thanks. > diff --git a/builtin/checkout.c b/builtin/checkout.c > index e9d111bb83..62b5e371bc 100644 > --- a/builtin/checkout.c > +++ b/builtin/checkout.c > @@ -621,9 +621,7 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o, > opts.src_index = &the_index; > opts.dst_index = &the_index; > init_checkout_metadata(&opts.meta, info->refname, > - info->commit ? &info->commit->object.oid : > - is_null_oid(&info->oid) ? &tree->object.oid : > - &info->oid, > + info->commit ? &info->commit->object.oid : &null_oid, > NULL); > parse_tree(tree); > init_tree_desc(&tree_desc, tree->buffer, tree->size);
On 2020-05-21 at 17:35:22, Junio C Hamano wrote: > "brian m. carlson" <sandals@crustytoothpaste.net> writes: > > > ... The only case in which we do not have a commit object is when > > invoking git switch with --orphan. Moreover, we can only hit this code > > path without a commit object additionally with either --force or > > --discard-changes. > > It was easy for me to trace the codepath to see when these options > are used we end up with no commit object, but I ran out of time > trying to see if the "forced orphan" is the only way to end up with > a NULL in new_branch_info->commit. Assuming that is true, of course > the following perfectly makes sense. I believe it is. The only case in which we have a NULL commit as far as I can tell is with switch and an orphan, and in merge_working_tree we call reset_tree either if the changes are discarded or unpack_trees couldn't do a trivial merge. Since I'm pretty sure unpack_trees can indeed merge with the empty tree, we would only call reset_trees with --discard-changes or --force. And reset_tree is only called from merge_working_tree. In addition, I did try other situations plus the entire testsuite with my erroneous first patch and was unable to cause a segfault anywhere (which would have been a trivial NULL dereference) in case I missed something, which leads me to believe that this is in fact the only situation in which this occurs.
diff --git a/builtin/checkout.c b/builtin/checkout.c index e9d111bb83..62b5e371bc 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -621,9 +621,7 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o, opts.src_index = &the_index; opts.dst_index = &the_index; init_checkout_metadata(&opts.meta, info->refname, - info->commit ? &info->commit->object.oid : - is_null_oid(&info->oid) ? &tree->object.oid : - &info->oid, + info->commit ? &info->commit->object.oid : &null_oid, NULL); parse_tree(tree); init_tree_desc(&tree_desc, tree->buffer, tree->size);
When we call init_checkout_metadata in reset_tree, we want to pass the object ID of the commit in question so that it can be passed to filters, or if there is no commit, the tree. We anticipated this latter case, which can occur elsewhere in the checkout code, but it cannot occur here. The only case in which we do not have a commit object is when invoking git switch with --orphan. Moreover, we can only hit this code path without a commit object additionally with either --force or --discard-changes. In such a case, there is no point initializing the checkout metadata with a commit or tree because (a) there is no commit, only the empty tree, and (b) we will never use the data, since no files will be smudged when checking out a branch with no files. Pass the all-zeros object ID in this case, since we just need some value which is a valid pointer. Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> --- builtin/checkout.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)