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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 8, 2016, 21:22 UTC
Message-ID: <xmqqh99qf5o7.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20160908204431.14612-1-benpeart@microsoft.com>

Ben Peart <peartben@gmail.com> 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...

Show 14 quoted lines

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