Re: [RFC PATCH 7/7] merge: fix misleading pre-merge check documentation

[Elijah Newren <newren@gmail.com>]

On Sat, Jun 2, 2018 at 11:58 PM, Elijah Newren <newren@gmail.com> wrote:
> builtin/merge.c contains this important requirement for merge strategies:
>
>     ...the index must be in sync with the head commit.  The strategies are
>     responsible to ensure this.
>
> However, Documentation/git-merge.txt says:
>
>     ...[merge will] abort if there are any changes registered in the index
>     relative to the `HEAD` commit.  (One exception is when the changed
>     index entries are in the state that would result from the merge
>     already.)
>
> Interestingly, prior to commit c0be8aa06b85 ("Documentation/git-merge.txt:
> Partial rewrite of How Merge Works", 2008-07-19),
> Documentation/git-merge.txt said much more:
>
>     ...the index file must match the tree of `HEAD` commit...
>     [NOTE]
>     This is a bit of a lite.  In certain special cases [explained
>     in detail]...
>     Otherwise, merge will refuse to do any harm to your repository
>     (that is...your working tree...and index are left intact).
>
> So, this suggests that the exceptions existed because there were special
> cases where it would case no harm, and potentially be slightly more
> convenient for the user.  While the current text in git-merge.txt does
> list a condition under which it would be safe to proceed despite the index
> not matching HEAD, it does not match what is actually implemented, in
> three different ways:
>
>     * The exception is written to describe what unpack-trees allows.  Not
>       all merge strategies allow such an exception, though, making this
>       description misleading.  'ours' and 'octopus' merges have strictly
>       enforced index==HEAD for a while, and the commit previous to this
>       one made 'recursive' do so as well.
>
>     * If someone did a three-way content merge on a specific file using
>       versions from the relevant commits and staged it prior to running
>       merge, then that path would technically satisfy the exception listed
>       in git-merge.txt.  unpack-trees.c would still error out on the path,
>       though, because it defers the three-way content merge logic to other
>       parts of the code (resolve, octopus, or recursive) and has no way of
>       checking whether the index entry from before the merge will match
>       the end result of the merge.
>
>     * The exception as implemented in unpack-trees actually only checked
>       that the index matched the MERGE_HEAD version of the file and that
>       HEAD matched the merge base.  Assuming no renames, that would indeed
>       provide cases where the index matches the end result we'd get from a
>       merge.  But renames means unpack-trees is checking that it instead
>       matches something other than what the final result will be, risking
>       either erroring out when we shouldn't need to, or not erroring out
>       when we should and overwriting the user's staged changes.
>
> In addition to the wording behind this exception being misleading, it is
> also somewhat surprising to see how many times the code for the special
> cases were wrong or the check to make sure the index matched head was
> forgotten altogether:
>
> * Prior to commit ee6566e8d70d ("[PATCH] Rewrite read-tree", 2005-09-05),
>   there were many cases where an unclean index entry was allowed (look for
>   merged_entry_allow_dirty()); it appears that in those cases, the merge
>   would have simply overwritten staged changes with the result of the
>   merge.  Thus, the merge result would have been correct, but the user's
>   uncommitted changes could be thrown away without warning.
>
> * Prior to commit 160252f81626 ("git-merge-ours: make sure our index
>   matches HEAD", 2005-11-03), the 'ours' merge strategy did not check
>   whether the index matched HEAD.  If it didn't, the resulting merge
>   would include all the staged changes, and thus wasn't really an 'ours'
>   strategy.
>
> * Prior to commit 3ec62ad9ffba ("merge-octopus: abort if index does not
>   match HEAD", 2016-04-09), 'octopus' merges did not check whether the
>   index matched HEAD, also resulting in any staged changes from before
>   the commit silently being folded into the resulting merge.  commit
>   a6ee883b8eb5 ("t6044: new merge testcases for when index doesn't match
>   HEAD", 2016-04-09) was also added at the same time to try to test to
>   make sure all strategies did the necessary checking for the requirement
>   that the index match HEAD.  Sadly, it didn't catch all the cases, as
>   evidenced by the remainder of this list...
>
> * Prior to commit 65170c07d466 ("merge-recursive: avoid incorporating
>   uncommitted changes in a merge", 2017-12-21), merge-recursive simply
>   relied on unpack_trees() to do the necessary check, but in one special
>   case it avoided calling unpack_trees() entirely and accidentally ended
>   up silently including any staged changes from before the merge in the
>   resulting merge commit.
>
> * The commit immediately before this one in this series noted that the
>   exceptions were written in a way that assumed no renames, making it
>   unsafe for merge-recursive to use.  merge-recursive was modified to
>   use its own check to enforce that index==HEAD.
>
> This history makes it very tempting to go into builtin/merge.c and replace
> the comment that strategies must enforce that index matches HEAD with code
> that just enforces it.  At this point, that would only affect the
> 'resolve' strategy; all other strategies have each been modified to
> manually enforce it.

I'm curious if anyone has comments on this last paragraph above.
Would anyone object to strictly enforcing index matches HEAD before
all types of merges?