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

> 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.
>