| Message ID | 20200520014156.1570124-1-sandals@crustytoothpaste.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/1] builtin/checkout: simplify metadata initialization | expand |
"brian m. carlson" <sandals@crustytoothpaste.net> writes: > 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, since reset_tree is called only (indirectly) via switch_branches, > which requires that we have a valid commit. switch_branches dies if we > lack a name and cannot produce a commit from HEAD, and its caller dies > if we do have a branch name but still lack a commit pointer. > > Since we know we must always have a valid commit structure in this case, > let's remove the dead code paths and just refer to the commit structure. > This simplifies the code and makes it easier for the reader. builtin/checkout.c::merge_working_tree() has these lines in its earlier part: if (opts->new_orphan_branch && opts->orphan_from_empty_tree) { if (new_branch_info->commit) BUG("'switch --orphan' should never acc..."); new_tree = parse_tree_indirect(the_hash_algo->empty_tree); } else new_tree = get_commit_tree(new_branch_info->commit); if (opts->discard_changes) { ret = reset_tree(new_tree, opts, 1, writeout_error, new_branch_info); if (ret) return ret; ... So, when orphan && orphan-from-empty are both set, we must not have commit, and then if discard is also there, we end up passing new_brnach_info that has NULL in its commit. There are few more callers of reset_tree() in this function, which I did not trace. Perhaps the "orphan && orphan-from-empty" is a dead combination and we won't hit the codepath and that is why this change is safe? I dunno. > Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> > --- > builtin/checkout.c | 6 +----- > 1 file changed, 1 insertion(+), 5 deletions(-) > > diff --git a/builtin/checkout.c b/builtin/checkout.c > index e9d111bb83..e53bdab5b8 100644 > --- a/builtin/checkout.c > +++ b/builtin/checkout.c > @@ -620,11 +620,7 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o, > opts.verbose_update = o->show_progress; > 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, > - NULL); > + init_checkout_metadata(&opts.meta, info->refname, &info->commit->object.oid, NULL); > parse_tree(tree); > init_tree_desc(&tree_desc, tree->buffer, tree->size); > switch (unpack_trees(1, &tree_desc, &opts)) {
On 2020-05-20 at 15:17:43, Junio C Hamano wrote: > "brian m. carlson" <sandals@crustytoothpaste.net> writes: > > > 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, since reset_tree is called only (indirectly) via switch_branches, > > which requires that we have a valid commit. switch_branches dies if we > > lack a name and cannot produce a commit from HEAD, and its caller dies > > if we do have a branch name but still lack a commit pointer. > > > > Since we know we must always have a valid commit structure in this case, > > let's remove the dead code paths and just refer to the commit structure. > > This simplifies the code and makes it easier for the reader. > > builtin/checkout.c::merge_working_tree() has these lines in its > earlier part: > > if (opts->new_orphan_branch && opts->orphan_from_empty_tree) { > if (new_branch_info->commit) > BUG("'switch --orphan' should never acc..."); > new_tree = parse_tree_indirect(the_hash_algo->empty_tree); > } else > new_tree = get_commit_tree(new_branch_info->commit); > if (opts->discard_changes) { > ret = reset_tree(new_tree, opts, 1, writeout_error, new_branch_info); > if (ret) > return ret; > ... > > So, when orphan && orphan-from-empty are both set, we must not have > commit, and then if discard is also there, we end up passing > new_brnach_info that has NULL in its commit. Good point. I missed that part. > Perhaps the "orphan && orphan-from-empty" is a dead combination and > we won't hit the codepath and that is why this change is safe? I > dunno. It looks like it's only triggered from git switch with --orphan, not with git checkout. And furthermore, it seems we require --force or --discard-changes to trigger that case, which we don't have anywhere in the testsuite. I'll come up with a different patch. I'll probably just set a NULL object ID there, since if we're checking out a new orphan branch, we won't have any files to check out, and therefore there's no possibility that we'll actually use the value for a filter process (since there are no files to filter).
diff --git a/builtin/checkout.c b/builtin/checkout.c index e9d111bb83..e53bdab5b8 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -620,11 +620,7 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o, opts.verbose_update = o->show_progress; 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, - NULL); + init_checkout_metadata(&opts.meta, info->refname, &info->commit->object.oid, NULL); parse_tree(tree); init_tree_desc(&tree_desc, tree->buffer, tree->size); switch (unpack_trees(1, &tree_desc, &opts)) {
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, since reset_tree is called only (indirectly) via switch_branches, which requires that we have a valid commit. switch_branches dies if we lack a name and cannot produce a commit from HEAD, and its caller dies if we do have a branch name but still lack a commit pointer. Since we know we must always have a valid commit structure in this case, let's remove the dead code paths and just refer to the commit structure. This simplifies the code and makes it easier for the reader. Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> --- builtin/checkout.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)