Re: [PATCH v1] checkout: optionally speed up "git checkout -b foo"
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jul 24, 2018, 19:21 UTC
- Message-ID
- <xmqqh8koxwwi.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <20180724180122.29212-1-benpeart@microsoft.com>
Ben Peart <Ben.Peart@microsoft.com> writes:
Show 9 quoted lines
> 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.
Show 7 quoted lines
> 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.
Show 28 quoted lines
> 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.
Show 7 quoted lines
> +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"?
Show 74 quoted lines
> + */ > + 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?
Show 15 quoted lines
> @@ -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.