Subject: Re: [PATCH] checkout: eliminate unnecessary merge for trivial checkout

From: Junio C Hamano <[email-masked]>
Date: Thu, 08 Sep 2016 14:22:16 -0700

Ben Peart <[email-masked]> writes:

> Teach git to avoid unnecessary merge during trivial checkout.  When
> running 'git checkout -b foo' git follows a common code path through
> the expensive merge_working_tree even when it is unnecessary.

I would be lying if I said I am not sympathetic to the cause, but...

> +	/*
> +	 * 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?

Even the main condition is unclear.  It wants to see that old and
new have exactly the same commit, but shouldn't the "the result of
the two-way merge is known to be no-op" logic equally apply if the
old and two trees are the same?