summaryrefslogtreecommitdiff
path: root/t/t4018
diff options
context:
space:
mode:
authorLibravatar Elijah Newren <newren@gmail.com>2018-06-30 18:25:03 -0700
committerLibravatar Junio C Hamano <gitster@pobox.com>2018-07-11 09:38:36 -0700
commit55f39cf7551bd468065306328efdb78933b36b69 (patch)
tree7a7e6863d5447d82cceb84ad9ad860bddbf008a2 /t/t4018
parentmerge-recursive: enforce rule that index matches head before merging (diff)
downloadtgif-55f39cf7551bd468065306328efdb78933b36b69.tar.xz
merge: fix misleading pre-merge check documentation
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 lie. 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. (However, note that index==HEAD is not strictly enforced for fast-forward merges, as those are not considered a merge strategy and they trigger in builtin/merge.c before the section in the code where the relevant comment is found.) But, even if we don't take the step of just fixing these problems by enforcing index==HEAD for all strategies, we at least need to update this misleading documentation in git-merge.txt. For now, just modify the claim in Documentation/git-merge.txt to fix the error. The precise details around combination of merges strategies and special cases probably is not relevant to most users, so simply state that exceptions may exist but are narrow and vary depending upon which merge strategy is in use. Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Diffstat (limited to 't/t4018')
0 files changed, 0 insertions, 0 deletions