Re: [RFC/PATCH] mergetool: use resolved conflicts in all the views

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Dec 21, 2020, 01:46 UTC
Message-ID: <5fdffe7f7fbd_89f120860@natae.notmuch>
In-Reply-To: <CAJDDKr5EiH3R284XNToTkVQ1a12Mqp7oCA30xpt_cMjatfHQjg@mail.gmail.com>

David Aguilar wrote:

Show 24 quoted lines

> On Sat, Dec 19, 2020 at 11:53 PM Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
> >
> > David Aguilar wrote:
> > > On Sat, Dec 19, 2020 at 12:59 PM Felipe Contreras
> > > <felipe.contreras@gmail.com> 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

That doesn't mean you are right.

This is an appeal to authority fallacy.

> Poking fun is my answer to such misguided seriousness.

I did not poke fun. I was just pointing out the fact that it wasn't an argument.

Show 5 quoted lines

> > > I don't have to imagine these use cases, they are very real.
> >
> > Show one.
> 
> Heh, I don't have to show you anything.

No you don't, but in a discussion with other people if you don't substantiate your claims, then others have no rational reason to consider them.

Show 5 quoted lines

> 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.

So it's not a real use-case, it's an imaginary one.

Even if we do consider this imaginary use-case, you just swept the problem under the carpet by saying "is able to use the full set of information".

How?

Explain how this tool would use this information.

Show 21 quoted lines

> > > 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

None of those are user preferences.

> We even have mergetool.meld.hasOutput which is completely tool-specific.

We don't have a user preference `mergetool.hasOutput`, or
`mergetool.vimdiff3.hasOutput` for that matter.

> I'm not asking, I'm telling you: the tool was designed around per-tool
> hooks.  This is per-tool behavior.  End of story.

And yet a tool cannot override `mergetool.keepBackup`.

Correct?

Show 7 quoted lines

> > ?
> >
> > 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.

Can it?

I am waiting for somebody to show *how*.

> There is a difference between top-level and per-tool behavior.

> This is a per-tool feature

No, that's your opinion. You haven't explained why.

> The way to think about it is that it's a per-tool feature

Ditto.

> We are not designing just for today; we must keep our eye on
> longer-term goals where the community tools can be improved.

Yes, but an actual future that might actually happen.

> Here is the "right" question to ask: Why shouldn't we have this
> flexibility?

Why shouldn't we have `mergetool.vimdiff3.keepBackup`?

> 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,

Precisely *how* a tool couldn't cope?

> 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?

We can move forward right now.

But no, I am 99.99% certain *nobody* will ever implement should_automerge (), so I won't waste my time with it.

If you think otherwise, you spend your time implementing this on top of my patch. That way it's clear who made the mistake.

Show 21 quoted lines

> 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.

I don't think mergetool.autoMerge should be any different from mergetool.keepBackup; a single global user configuration suffices.

You say the argument against such per-tool configuration is thin, I say the argument in favor is non-existent.

> 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.

Nobody can tell me how I chose to spend my free time that I'm choosing to contribute to this project altruistically.

> 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.

That's not a compromise. That's a difference of opinion stated in the code.

I don't have to convince you. And you don't have to convince me.

> 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.

And I would prefer that we don't waste time with hypothetical use-cases that will never materialize.

We can add such configuration when the need actually arises.

But we can't always have what we prefer.

Cheers.

[1] https://en.wikipedia.org/wiki/Argument_from_authority

-- 
Felipe Contreras