Re: [PATCH v3] checkout: eliminate unnecessary merge for trivial checkout

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 28, 2016, 17:52 UTC
Message-ID: <xmqqtwczkj3k.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <004d01d219aa$0a941fa0$1fbc5ee0$@gmail.com>

"Ben Peart" <peartben@gmail.com> writes:

Show 10 quoted lines

> The fact that "git checkout -b NEW" updates the index and as a
> result reflects any changes in the sparse-checkout and the issue 
> Junio pointed out earlier about not calling show_local_changes 
> at the end of merge_working_tree are the only difference in behavior
> I am aware of.  Both of these are easily rectified.
>
> That said, given we are skipping huge amounts of work by no longer 
> merging the commit trees, generating a new index, and merging the 
> local modifications in the working tree, it is possible that there are
> other behavior changes I'm just not aware of.

That is OK. It is not ok to leave such bugs at the end of the development before the topic is merged to 'master' to be delivered to the end users, but you do not have to fight alone to produce a perfect piece of code with your first attempt. That's what the reviews and testing period are for.

If you are shooting for the same behaviour, then that is much better than "make 'checkout -b NEW' be equivalent to a sequence of update-ref && symbolic-ref, which is different from others", which was the second explanation you gave earlier. I am much happier with that goal.

But if that is the case, I really do not see any point of singling out "-b NEW" case. The following property MUST be kept:

 (1) "git checkout -b NEW", "git checkout", "git checkout HEAD^0"
     and "git checkout HEAD" (no other parameters to any of them)
     ought to give identical index and working tree.  It is too
     confusing to leave subtly different results that will lead to
     hard to diagnose bugs for only one of them.

That would make the "do we skip unpack_trees() call?" decision a lot simpler to make, I would suspect. We only need to see "are the two trees we would fed unpack_trees() the same as HEAD's tree?" and do not have to look at new_branch and other irrelevant things at all. What happens in the ref namespace is immaterial, as making or skipping an unpack_trees() call would not affect anything other than the resulting index and the working tree. If we want to keep that sparse-checkout wart, we would also need to see if the control file sparse-checkout keeps in $GIT_DIR/ exists, but the result will be much simpler set of rules, and would hopefully help remove the "the optimization kicks in following logic that is an unreviewable-mess" issue.