Seth House <seth@xxxxxxxxx> writes:
> Would you mind switching the autoMerge option to be per-tool rather than
> under the main mergetool config section?
>
> You're right that it will likely not cause any downstream errors; it's
> just a difference in preference. The tools that perform their own
> conflict resolution will likely want it off and most of the other tools
> will likely want it on. I could envision wanting to configure multiple
> mergetools -- some with and some without.
Thanks for a concise summary.
Many people may stick to just a single tool and may find a single
mergetool.autoMerge knob convenient (and it is OK for them if the
new behaviour broke a tool they do not use), but for those who use
more than one, being able to configure them differently would be
valuable. So I agree that making it a per-tool option would be a
good thing to do. Extra bonus for "this covers all" fallback default
perhaps like so:
if test "$(git config --get --bool "mergetool.$merge_tool.automerge" ||
git config --get --bool "mergetool.autoMerge" ||
echo true)" = true
then
... do the new "reduce conflicts" thing ...
else
... do the "grab the original" thing ...
fi
There is another thing that the final version may want to consider.
Would there be a reason for a tool to actively want to refuse the
end-user request to use the new autoMerge behaviour? A tool that
can merge binary files may want to make sure it gets the original
three blobs that got involved in the conflict, and the above
would allow users to break such a tool. We could say "don't do
that then" to users, or we can use the same mechanism to set up
diff_cmd() etc. per tool [*1*]. A tool that always wants to use
the autoMerge without letting end-user to opt out can be handled
with the same mechanism, but I think that is less likely.
Also, just for completeness (this is a comment made after seeing v3),
unlike the previous rounds, we do not have to worry about conflict
marker length attribute user may have and rely on the default
marker-size hardcoded at the xdl_merge() layer. If we wanted to
make it even safer, "git merge-file --diff3" invocation can also
use a hardcoded --marker-size=7 option to protect from changes in
the underlying xdl_merge()'s default.
Unfortunately, it however also means that we can be confused when we
are seeing a conflicted outer merge of a recursive merge. In such a
case, the virtual ancestor version in stage #1 can have conflict
markers from the inner merge that also conflicted, which uses marker
size a bit longer than what the user gave via the attribute
machinery. If that "a bit longer" length happens to be the same as
the hardcoded assumption of 7 the sed script in v3 makes, the sed
script would not be able to tell between the existing markers in the
virtual ancestor version and the markers "git merge-file --diff3"
creates.
It is unlikely that the end-user sets conflict marker length to
anything shorter than the default 7, so I think it is OK to punt
this potential problem, and assume we are OK. The limitation may
need to be documented, though.
> After work today I can go back through the list of mergetools reviewed
> in my post and grab screenshots of each with this option enabled so that
> everyone in this thread can quickly compare the before/after results.
Thanks.
[Footnote]
*1* If we were to go that extra mile for completeness, the result
would roughly look like the following outline:
1. git-mergetool--lib.sh::setup_tool() would define a new helper
function
automerge_enabled () {
true
}
to be used as the fallback
2. a tool that wants to use the original without allowing the user
to opt into automerge would add
automerge_enabled () {
false
}
in mergetools/$toolname (where toolname is like 'tkdiff',
'vimdiff', etc.)
3. then the "consult mergetool.<tool>.automerge and then
mergetool.automerge in this order" if statement we saw above
would first ask the new helper function, i.e.
if automerge_enabled &&
test "$(git config --get --bool "mergetool.$merge_tool.automerge" ||
git config --get --bool "mergetool.automerge" ||
echo true)" = true
then
...
