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