> -----Original Message-----
> From: Jeff King [mailto:[email protected]]
> Sent: Thursday, September 8, 2016 5:38 PM
> To: Junio C Hamano <[email protected]>
> Cc: Ben Peart <[email protected]>; [email protected];
> [email protected]; [email protected]; Ben Peart
> <[email protected]>
> Subject: Re: [PATCH] checkout: eliminate unnecessary merge for trivial
> checkout
>
> On Thu, Sep 08, 2016 at 02:22:16PM -0700, Junio C Hamano wrote:
>
> > > + /*
> > > + * 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?
Because this induces a behavior change (the optimized path will no
longer do a "soft reset" and regenerate the index for example) I was
attempting to make it as restrictive as possible but still enable the
fast path in the most common case. If everyone is OK with the behavior
change, I can make the optimization more inclusive by removing those
tests that are not absolutely required (like opts->prefix).
To help ensure the optimization is updated when new checkout options are
added I could add a comment into the checkout_opts structure and/or put
a pseudo version check into the code so if the size of the structure
changes, the fast path fails. That feels a little hacky and I haven't
seen that in other areas so I'd rather stick with splitting it out into
a helper function and add comments.
>
> 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 is a great suggestion. Splitting this out into a helper function
with comments will definitely make this more readable/maintainable and
provide more information on why each test is there. I'll do that and
reroll the patch.
>
> 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.
