Ben Peart <ben.pe...@microsoft.com> writes: > From: Ben Peart <ben.pe...@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.gb13...@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.pe...@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.
