On Thu, Sep 08, 2016 at 02:22:16PM -0700, Junio C Hamano wrote:
> > + /*
> > + * Optimize the performance of checkout when the current and
> > + * new branch have the same OID and avoid the trivial merge.
> > + * For example, a "git checkout -b foo" just needs to create
> > + * the new ref and report the stats.
> > + */
> > + if (!old.commit || !new->commit
> > + || oidcmp(&old.commit->object.oid, &new->commit->object.oid)
> > + || !opts->new_branch || opts->new_branch_force || opts->new_orphan_branch
> > + || opts->patch_mode || opts->merge || opts->force || opts->force_detach
> > + || opts->writeout_stage || !opts->overwrite_ignore
> > + || opts->ignore_skipworktree || opts->ignore_other_worktrees
> > + || opts->new_branch_log || opts->branch_exists || opts->prefix
> > + || opts->source_tree) {
>
> ... this is a maintenance nightmare in that any new option we will
> add later will need to consider what this "optimization" is trying
> (not) to skip. The first two lines (i.e. we need a real checkout if
> we cannot positively say that old and new commits are the same
> object) are clear, but no explanation was given for all the other
> random conditions this if condition checks. What if opts->something
> was not listed (or "listed" for that matter) in the list above--it
> is totally unclear if it was missed by mistake (or "added by
> mistake") or deliberately excluded (or "deliberately added").
>
> For example, why is opts->prefix there? If
>
> git checkout -b new-branch HEAD
>
> should be able to omit the two-way merge, shouldn't
>
> cd t && git checkout -b new-branch HEAD
>
> also be able to?
I was just writing another reply, but I think our complaints may have
dovetailed.
My issue is that the condition above is an unreadable mass. It would be
really nice to pull it out into a helper function, and then all of the
items could be split out and commented independently, like:
static int needs_working_tree_merge(const struct checkout_opts *opts,
const struct branch_info *old,
const struct branch_info *new)
{
/*
* We must do the merge if we are actually moving to a new
* commit.
*/
if (!old->commit || !new->commit ||
oidcmp(&old.commit->object.oid, &new->commit->object.oid))
return 1;
/* Option "foo" is not compatible because of... */
if (opts->foo)
return 1;
... etc ...
}
That still leaves your "what if opts->something is not listed" question
open, but at least it makes it easier to comment on it in the code.
-Peff
PS I didn't think hard on whether the conditions above make _sense_. My
first goal would be to get more communication about them individually,
and then we can evaluate them.