Re: [PATCH 2/2] mergetool: run prompt only if guessed tool

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Apr 22, 2014, 07:56 UTC
Message-ID: <535620a6e8274_3d2216372ec9d@nysa.notmuch>
In-Reply-To: <20140422073008.GA11584@hashpling.org>

Charles Bailey wrote:

Show 17 quoted lines

> On Tue, Apr 22, 2014 at 01:53:46AM -0500, Felipe Contreras wrote:
> > Charles Bailey wrote:
> > > On Tue, Apr 22, 2014 at 01:24:09AM -0500, Felipe Contreras wrote:
> > > > 
> > > > This is what I get when a tool is not working:
> > > > 
> > > >   Documentation/config.txt seems unchanged.
> > > >   Was the merge successful? [y/n]
> > > 
> > > Does this happen now even with merge tools for which we do trust the
> > > exit code? If so, my original concern is addressed.
> > 
> > Which tools are those?
> 
> I didn't remember off hand, but checking the mergetools directory
> suggests that kdiff3 is one (there's no call to check_unchanged). I
> stopped checking after I found one.

So:

	% cat > ~/bin/kdiff3 <<-\EOF
	#!/bin/sh
	false
	EOF
	% chmod +x ~/bin/kdiff3
  % git -c merge.tool=kdiff3 mergetool
  merge of Documentation/config.txt failed
  Continue merging other unresolved paths (y/n) ?

Show 11 quoted lines

> > You don't see anything wrong with asking the user *every single time* he runs
> > `git mergetool`, even though he *already told us* which tool to use?
> >  
> > If so, I'm pretty sure everybody else disagrees with you.
> 
> I think that you may have misunderstood me. As I said, I've no
> particular objections to changing the default (subject a few concerns
> that may or may not still apply).
> 
> Having said that, the fact that the user has configured the merge tool
> doesn't mean that he necessarily doesn't want to see the prompt.

Not necessarily, but in 99% of the cases it does. And for the remaining there's always mergetool.prompt = true.

> In a part of my reply which you snipped, I said that it's sometimes the
> particular file that's due to be resolved that might prompt a user to want to
> skip launching the tool.

That's a possibility, however, in almost all the situations I've wanted to stop a merge, it's *after* I've seen the actual conflicts in the file.

Anyway, I've revisited the code, and it's only now that I've realized that this:

  Hit return to start merge resolution tool (kdiff3):

Is not actually asking me for the tool I want to use; the value in parenthesis is not the default, and I can't type in another tool.

So the purpose of the prompt is very different from what I was thinking, yet I still think the value of such prompt is marginal at best.

> Either way, I think we shouldn't unconditionally override an explicitly
> set mergetool.prompt and if we are (effectively) changing the default we
> should probably update the documentation to say so as well.

An explicitly set mergetool.prompt = true would override the default. See the patch.

I looked, the documentation doesn't mention any default. We could add it, but I don't think it's necesarily part of this patch.

-- 
Felipe Contreras