Thomas Rast <trast@xxxxxxxxxxxxxxx> writes: > The REUC extension stores the stage 1/2/3 data of entries which were > marked resolved by the user, to enable 'git checkout -m <name>' to > restore the conflicted state later. Yes. > When a file was deleted on one side of the merge and unmodified on the > other, merge-recursive uses remove_file_from_cache() to remove it from > the result index. This uses remove_index_entry_at(), which calls > record_resolve_undo(). What is missing from here which confused me during my initial read of this patch is "But such a removal is the natural resolution for the three-way merge. It is not a resolution by the end user, and should not be unresolved with 'checkout -m'. Recording REUC entry for such a path is wrong." Perhaps you thought it was so obvious that it can be left unsaid, but combined with the "in fact _even_ useless" below, I had to re-read it to find something that says why it was _wrong_, did not find any, and had to scratch my head. > Such REUC entries are in fact even useless: neither 'git checkout -m' > nor 'git update-index --unresolve' can resurrect the file (the former > because there is no corresponding index entry, the latter because the > file is missing one side). I do not think that "they are not used the current implementation of the commands that are supposed to use the information" automatically qualifies as a good reason to remove them. If the conflict were "we modified while they removed", the resolution may either be "keep some stuff we added" or "delete the path", we may want to be able to resurrect the conflicted state with "checkout -m" for them, and we may want to fix "checkout -m" and "update-index --unresolve" to deal with such a case if they don't already, which is an independent topic. For the "one side untouched, the other side removed" case, removing is the natural resolution, so we do not want to have REUC entry to begin with, so there is nothing to fix in "checkout -m" for that case. > Solve this by taking a more specific approach to record_resolve_undo(): Just a sanity check. Are there cases we would want to have _any_ REUC entries in the index, after running any mergy operation, not just merge-recursive, but cherry-pick and friends that share the same machinery? > * git-rm and 'git update-index --remove' go through > remove_file_from_cache(), just like merge-recursive. Make them use > a new _extended version that optionally records REUC. I wonder if it is better have two functions, one records REUC (and does nothing else) and the other that removes a path from the in-core index (and does nothing else), instead of two functions that both remove a path from the in-core index (one with REUC and the other without). Would it be less error-prone for the callers and make the resulting code easier to follow? if (path is conflicted and we are resolving as removal) record_REUC(&the_index, path); remove_file_from_cache(&the_index, path); Not a suggestion "I think it is better", but just a question. > * git-add and 'git update-index conflicted_file' go through the > add_index_entry() call chain. git-apply and git-read-tree use > add_index_entry() too. However, they insert stage-0 entries, which > already means resolving. So even if these cases were not caught > earlier, saving the unresolved state would be correct. > So we can unconditionally record REUC deeper in the call chain. Are there cases where an automerge use add_index_entry() to insert a stage#0 entry to the index (which already means resolving) to record a clean automerge? Doesn't the same "a natural three-way resolution should not be unresolved with 'checkout -m'" reasoning as the original motivation of this patch apply to it? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html