Re: [PATCH v1] checkout: optionally speed up "git checkout -b foo"

[Junio C Hamano <gitster@pobox.com>]

Ben Peart <Ben.Peart@microsoft.com> writes:

> From: Ben Peart <Ben.Peart@microsoft.com>
>
> If the new core.optimizecheckout config setting is set to true, speed up
> "git checkout -b foo" by avoiding the work to merge the working tree.  This
> is valid because no merge needs to occur - only creating the new branch/
> updating the refs. Any other options force it through the old code path.
>
> This change in behavior is off by default and behind the config setting so
> that users have to opt-in to the optimized behavior.




> We've been running with this patch internally for a long time but it was
> rejected when I submitted it to the mailing list before because it
> implicitly changes the behavior of checkout -b. Trying it again configured
> behind a config setting as a potential solution for other optimizations to
> checkout that could change the behavior as well.
>
> https://public-inbox.org/git/20180724042740.GB13248@sigill.intra.peff.net/T/#m75afe3ab318d23f36334cf3a6e3d058839592469

An incorrect link?  It does not look like a thread that explains
what was previously submitted but failed.  The last paragraph looks
like a fine material below the three-dash line.


> Signed-off-by: Ben Peart <Ben.Peart@microsoft.com>
> ---
>
> Notes:
>     Base Ref: master
>     Web-Diff: https://github.com/benpeart/git/commit/f43d934ce7
>     Checkout: git fetch https://github.com/benpeart/git checkout-b-v1 && git checkout f43d934ce7
>
>  Documentation/config.txt |  6 +++
>  builtin/checkout.c       | 94 ++++++++++++++++++++++++++++++++++++++++
>  cache.h                  |  1 +
>  config.c                 |  5 +++
>  environment.c            |  1 +
>  5 files changed, 107 insertions(+)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index a32172a43c..2c4f513bf1 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -911,6 +911,12 @@ core.commitGraph::
>  	Enable git commit graph feature. Allows reading from the
>  	commit-graph file.
>  
> +core.optimizedCheckout
> +	Speed up "git checkout -b foo" by skipping much of the work of a
> +	full checkout command.  This changs the behavior as it will skip
> +	merging the trees and updating the index and instead only create
> +	and switch to the new ref.

By the way, why is it a core.* thing, not checkout.* thing?

If a new feature is not necessarily recommendable for normal users
and it needs to be hidden behind an opt-in knob (I do not have a
strong opinion if that is or is not the case for this particular
feature at this point), the documentation for the knob should give a
bit more than "This chang(e)s the behavior" to the readers, I would
think, to be intellectually honest ;-).  Let's tell them what bad
things happen if we pretend that we switched the branch without
twoway merge and the index update to help them make an informed
decision.

> +static int needs_working_tree_merge(const struct checkout_opts *opts,
> +	const struct branch_info *old_branch_info,
> +	const struct branch_info *new_branch_info)
> +{
> +	/*
> +	 * We must do the merge if we are actually moving to a new
> +	 * commit tree.

What's a "commit tree"?  Shouldn't it be just a "commit"?

> +	 */
> +	if (!old_branch_info->commit || !new_branch_info->commit ||
> +		oidcmp(&old_branch_info->commit->object.oid, &new_branch_info->commit->object.oid))
> +		return 1;
> +
> +	/*
> +	 * opts->patch_mode cannot be used with switching branches so is
> +	 * not tested here
> +	 */
> +
> +	/*
> +	 * opts->quiet only impacts output so doesn't require a merge
> +	 */
> +
> +	/*
> +	 * Honor the explicit request for a three-way merge or to throw away
> +	 * local changes
> +	 */
> +	if (opts->merge || opts->force)
> +		return 1;
> +
> +	/*
> +	 * --detach is documented as "updating the index and the files in the
> +	 * working tree" but this optimization skips those steps so fall through
> +	 * to the regular code path.
> +	 */
> +	if (opts->force_detach)
> +		return 1;
> +
> +	/*
> +	 * opts->writeout_stage cannot be used with switching branches so is
> +	 * not tested here
> +	 */
> +
> +	/*
> +	 * Honor the explicit ignore requests
> +	 */
> +	if (!opts->overwrite_ignore || opts->ignore_skipworktree ||
> +		opts->ignore_other_worktrees)
> +		return 1;
> +
> +	/*
> +	 * opts->show_progress only impacts output so doesn't require a merge
> +	 */
> +
> +	/*
> +	 * If we aren't creating a new branch any changes or updates will
> +	 * happen in the existing branch.  Since that could only be updating
> +	 * the index and working directory, we don't want to skip those steps
> +	 * or we've defeated any purpose in running the command.
> +	 */
> +	if (!opts->new_branch)
> +		return 1;
> +
> +	/*
> +	 * new_branch_force is defined to "create/reset and checkout a branch"
> +	 * so needs to go through the merge to do the reset
> +	 */
> +	if (opts->new_branch_force)
> +		return 1;
> +
> +	/*
> +	 * A new orphaned branch requrires the index and the working tree to be
> +	 * adjusted to <start_point>
> +	 */
> +	if (opts->new_orphan_branch)
> +		return 1;
> +
> +	/*
> +	 * Remaining variables are not checkout options but used to track state
> +	 */
> +
> +	return 0;
> +}

This helper function alone looks like we are creating a maintenance
nightmare from a quick scan.  How are we going to keep this up to
date?

I offhand do not know how "git checkout -b foo" would behave
differently if we do not do a two-way merge between HEAD and HEAD to
update the index.  We'd still need to list the local modifications
and say "Switched to a new branch 'foo'", but that would be a minor
thing compared to the two-way merge machinery.

Was the primary reason why the patch "changes the behaviour" because
nobody could prove that needs_working_tree_merge() helper reliably
detects that "checkout -b foo" case and that case alone, and show a
way to make sure it will keep doing so in the future when other new
features are added to the command?

> @@ -479,6 +565,14 @@ static int merge_working_tree(const struct checkout_opts *opts,
>  	int ret;
>  	struct lock_file lock_file = LOCK_INIT;
>  
> +	/*
> +	 * Skip merging the trees, updating the index, and work tree only if we
> +	 * are simply creating a new branch via "git checkout -b foo."  Any
> +	 * other options or usage will continue to do all these steps.
> +	 */
> +	if (core_optimize_checkout && !needs_working_tree_merge(opts, old_branch_info, new_branch_info))
> +		return 0;
> +
>  	hold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);
>  	if (read_cache_preload(NULL) < 0)
>  		return error(_("index file corrupt"));

Thanks.