Re: [PATCH] checkout: eliminate unnecessary merge for trivial checkout
- From
Jeff King <peff@peff.net>
- Date
- Sep 8, 2016, 21:37 UTC
- Message-ID
- <20160908213738.zgwgfy3nybkam3hk@sigill.intra.peff.net>
- In-Reply-To
- <xmqqh99qf5o7.fsf@gitster.mtv.corp.google.com>
On Thu, Sep 08, 2016 at 02:22:16PM -0700, Junio C Hamano wrote:
Show 34 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?I was just writing another reply, but I think our complaints may have dovetailed.
My issue is that the condition above is an unreadable mass. It would be really nice to pull it out into a helper function, and then all of the items could be split out and commented independently, like:
static int needs_working_tree_merge(const struct checkout_opts *opts,
const struct branch_info *old,
const struct branch_info *new)
{
/*
* We must do the merge if we are actually moving to a new
* commit.
*/
if (!old->commit || !new->commit ||
oidcmp(&old.commit->object.oid, &new->commit->object.oid))
return 1;/* Option "foo" is not compatible because of... */ if (opts->foo) return 1;
... etc ... }
That still leaves your "what if opts->something is not listed" question open, but at least it makes it easier to comment on it in the code.
-Peff
PS I didn't think hard on whether the conditions above make _sense_. My first goal would be to get more communication about them individually, and then we can evaluate them.