Thomas Rast <tr...@student.ethz.ch> 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
