On Sat, Jun 2, 2018 at 11:58 PM, Elijah Newren <newren@xxxxxxxxx> 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?
