On Sat, Dec 19, 2020 at 11:53 PM Felipe Contreras
<felipe.contreras@xxxxxxxxx> wrote:
>
> David Aguilar wrote:
> > On Sat, Dec 19, 2020 at 12:59 PM Felipe Contreras
> > <felipe.contreras@xxxxxxxxx> wrote:
> > > They don't have to rely on that failure, they can just turn off
> > > mergetool.automerge.
> > >
> > >
> > > But fine. Let's the perfect be the enemy of the good. That seems wise.
> >
> > FWIW I'm in favor of having per-tool configuration precisely for
> > custom mergetools that do things with custom file formats and benefit
> > from having all of LOCAL REMOTE and BASE.
>
> That's a preference, not an argument.
Lol, that's why I said "in favor". But, since you asked, it turns out
that my preferences have a stronger weight than yours.
"git shortlog -n git-difftool* git-mergetool*" shows that my
preferences and opinions are 44 times more important than yours in
this area, whether you like it or not. ;-p
Poking fun is my answer to such misguided seriousness.
> > I don't have to imagine these use cases, they are very real.
>
> Show one.
Heh, I don't have to show you anything. If you *really* want to see
one then I would be super happy to get your resume, but I doubt I can
convince you to go that far!
Barring that, please use your imagination. Imagine a custom file
format for a graph-like data structure (both binary and text-like)
that is able to use the full set of information for the purposes of
resolving conflicts. Private tools exist. It's impossible to prove
that they don't so I won't ask you or anyone else to do so.
> > This design choice is also in alignment with the general
> > mergetool/difftool per-tool configuration paradigm. If we didn't
> > support per-tool, then it would be inconsistent.
>
> Can you do
>
> 1. mergetool.vimdiff3.keepBackup
> 2. mergetool.vimdiff3.keepTemporaries
> 3. mergetool.vimdiff3.writeToTemp
> 4. mergetool.vimdiff3.prompt
can_merge()
diff_cmd()
merge_cmd()
translate_merge_tool_path()
list_tool_variants()
exit_code_trustable()
mergetool.<tool>.cmd
mergetool.<tool>.path
mergetool.<tool>.trustExitCode
We even have mergetool.meld.hasOutput which is completely tool-specific.
I'm not asking, I'm telling you: the tool was designed around per-tool
hooks. This is per-tool behavior. End of story.
> ?
>
> In fact the opposite is the case; not a *single* `mergetool.foo`
> configuration can be done with `mergetool.<tool>.foo`.
That's not the right question. This a behavior that can differ
per-tool and thus this feature must reflect that.
There is a difference between top-level and per-tool behavior. This
is a per-tool feature and we want a way for users to be able to
enable/disable it globally while still enabling/disabling for an
individual tool. We must allow individual tools to expose their
enhanced capabilities, otherwise the community tools have no reason to
improve, and private tools would be harmed.
The way to think about it is that it's a per-tool feature with a
top-level default. The important implementation detail is that tools
have plenty of hooks for per-tool flexibility; the configuration is
just one way that we allow for it. The scriplet for something like
"diffconflicts" could even override should_automerge() to return false
since it might want to handle it itself, for example.
We are not designing just for today; we must keep our eye on
longer-term goals where the community tools can be improved.
I'm in favor of the auto-merge feature in general, and think it should
be made the default behavior (in a future version?). That said, it
entails a change in behavior and having a granular mechanism to both
allow enhanced tools to do their own thing, and to allow the original
behavior to be retained, is the most sensible approach.
Here is the "right" question to ask: Why shouldn't we have this
flexibility? The implementation is pretty darn simple considering
that the toolset is already designed with per-tool affordances in
mind, so why not?
What I am 100% certain about is that someone is going to notice the
change in behavior, and whether or not it's "better" is completely
irrelevant. The heavy hammer of completely disabling the feature
means that they will have crippled all of the other mergetools just
because one of them couldn't cope, or because for whatever reason they
want the old behavior, so we should turn that heavy hammer into a
fine-grained chisel.
Are you willing to compromise on the small concession that we should
allow per-tool overrides so that we can move forward and get this
integrated?
To be extremely clear -- is this discussion arguing between the
following two strategies?
# Per-tool override + Global default
should_automerge () {
git config mergetool.$tool.automerge || git config
mergetool.automerge || echo true
}
# vs. Global default only
should_automerge () {
git config mergetool.automerge || echo true
}
, or are we discussing a different aspect? When spelled out in code,
we're discussing whether or not we should have
"mergetool.$tool.automerge ||" in there, and the argument for why not
is pretty thin considering that use cases that would be harmed by not
doing so exist.
Such a trivial difference is not a hill worth dying on so please route
this energy into a patch so that we can get this integrated.
I suggest this compromise -- if adding "mergetool.$tool.automerge ||"
to the logic is something you don't want to do then submit the patch
without it and I'll submit a follow-up patch that adds the per-tool
override.
I'd personally prefer to just have the patch include this from the
get-go, though, so if we've managed to convince you then please take
the shorter path to success.
--
David
