[PATCH 0/2] Simple default fixes for v2.0

[PATCH 0/2] Simple default fixes for v2.0

[Felipe Contreras]

I don't see why people need to set these to have sensible behavior:

  merge.defaulttoupstream = true
  mergetool.prompt = false

Let's change them to sane defaults so they are not needed.

Felipe Contreras (2):
  merge: enable defaulttoupstream by default
  mergetool: run prompt only if guessed tool

 Documentation/git-merge.txt |  5 ++---
 builtin/merge.c             |  2 +-
 git-mergetool.sh            | 14 +++++++++++---
 3 files changed, 14 insertions(+), 7 deletions(-)

-- 
1.9.2+fc1.1.g5c924db

[PATCH 1/2] merge: enable defaulttoupstream by default

[Felipe Contreras]

There's no point in this:

% git merge
fatal: No commit specified and merge.defaultToUpstream not set.

We know the most likely scenario is that the user wants to merge the
upstream, and if not, he can set merge.defaultToUpstream to false.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Documentation/git-merge.txt | 5 ++---
 builtin/merge.c             | 2 +-
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
index a3c1fa3..cf2c374 100644
--- a/Documentation/git-merge.txt
+++ b/Documentation/git-merge.txt
@@ -101,9 +101,8 @@ commit or stash your changes before running 'git merge'.
 	Specifying more than one commit will create a merge with
 	more than two parents (affectionately called an Octopus merge).
 +
-If no commit is given from the command line, and if `merge.defaultToUpstream`
-configuration variable is set, merge the remote-tracking branches
-that the current branch is configured to use as its upstream.
+If no commit is given from the command line, merge the remote-tracking
+branches that the current branch is configured to use as its upstream.
 See also the configuration section of this manual page.
 
 
diff --git a/builtin/merge.c b/builtin/merge.c
index 66d8843..1fc9319 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -63,7 +63,7 @@ static int verbosity;
 static int allow_rerere_auto;
 static int abort_current_merge;
 static int show_progress = -1;
-static int default_to_upstream;
+static int default_to_upstream = 1;
 static const char *sign_commit;
 
 static struct strategy all_strategy[] = {
-- 
1.9.2+fc1.1.g5c924db

Re: [PATCH 1/2] merge: enable defaulttoupstream by default

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> There's no point in this:
>
> % git merge
> fatal: No commit specified and merge.defaultToUpstream not set.
>
> We know the most likely scenario is that the user wants to merge the
> upstream, and if not, he can set merge.defaultToUpstream to false.

And a new possible failure case is when there is no upstream defined
for the current branch, which gets perfectly good new error message:

    $ git merge
    fatal: No remote for the current branch.

So I think this is good.  We want to protect this with a new test,
no?

Will queue as-is for now.

> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  Documentation/git-merge.txt | 5 ++---
>  builtin/merge.c             | 2 +-
>  2 files changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
> index a3c1fa3..cf2c374 100644
> --- a/Documentation/git-merge.txt
> +++ b/Documentation/git-merge.txt
> @@ -101,9 +101,8 @@ commit or stash your changes before running 'git merge'.
>  	Specifying more than one commit will create a merge with
>  	more than two parents (affectionately called an Octopus merge).
>  +
> -If no commit is given from the command line, and if `merge.defaultToUpstream`
> -configuration variable is set, merge the remote-tracking branches
> -that the current branch is configured to use as its upstream.
> +If no commit is given from the command line, merge the remote-tracking
> +branches that the current branch is configured to use as its upstream.
>  See also the configuration section of this manual page.
>  
>  
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 66d8843..1fc9319 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -63,7 +63,7 @@ static int verbosity;
>  static int allow_rerere_auto;
>  static int abort_current_merge;
>  static int show_progress = -1;
> -static int default_to_upstream;
> +static int default_to_upstream = 1;
>  static const char *sign_commit;
>  
>  static struct strategy all_strategy[] = {

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

[Felipe Contreras]

It's annoying to see the prompt:

  Hit return to start merge resolution tool (foo):

Every time the user does 'git mergetool' even if the user already
configured 'foo' as the wanted tool.

Display this prompt only when the user hasn't explicitly configured a
tool.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 git-mergetool.sh | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/git-mergetool.sh b/git-mergetool.sh
index 332528f..d08dc92 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -277,7 +277,7 @@ merge_file () {
 	echo "Normal merge conflict for '$MERGED':"
 	describe_file "$local_mode" "local" "$LOCAL"
 	describe_file "$remote_mode" "remote" "$REMOTE"
-	if "$prompt" = true
+	if test "$guessed_merge_tool" = true || test "$prompt" = true
 	then
 		printf "Hit return to start merge resolution tool (%s): " "$merge_tool"
 		read ans || return 1
@@ -315,7 +315,8 @@ merge_file () {
 	return 0
 }
 
-prompt=$(git config --bool mergetool.prompt || echo true)
+prompt=$(git config --bool mergetool.prompt)
+guessed_merge_tool=false
 
 while test $# != 0
 do
@@ -373,7 +374,14 @@ prompt_after_failed_merge () {
 
 if test -z "$merge_tool"
 then
-	merge_tool=$(get_merge_tool "$merge_tool") || exit
+	# Check if a merge tool has been configured
+	merge_tool=$(get_configured_merge_tool)
+	# Try to guess an appropriate merge tool if no tool has been set.
+	if test -z "$merge_tool"
+	then
+		merge_tool=$(guess_merge_tool) || exit
+		guessed_merge_tool=true
+	fi
 fi
 merge_keep_backup="$(git config --bool mergetool.keepBackup || echo true)"
 merge_keep_temporaries="$(git config --bool mergetool.keepTemporaries || echo false)"
-- 
1.9.2+fc1.1.g5c924db

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

[David Aguilar]

[Cc:ing Charles in case he has an opinion, this behavior dates back to the original MT]

On Sun, Apr 20, 2014 at 07:17:34PM -0500, Felipe Contreras wrote:
> It's annoying to see the prompt:
> 
>   Hit return to start merge resolution tool (foo):
> 
> Every time the user does 'git mergetool' even if the user already
> configured 'foo' as the wanted tool.
> 
> Display this prompt only when the user hasn't explicitly configured a
> tool.

I agree this is probably helpful.
Most users I've met end up configuring mergetool.prompt=false.

Thanks

> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  git-mergetool.sh | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/git-mergetool.sh b/git-mergetool.sh
> index 332528f..d08dc92 100755
> --- a/git-mergetool.sh
> +++ b/git-mergetool.sh
> @@ -277,7 +277,7 @@ merge_file () {
>  	echo "Normal merge conflict for '$MERGED':"
>  	describe_file "$local_mode" "local" "$LOCAL"
>  	describe_file "$remote_mode" "remote" "$REMOTE"
> -	if "$prompt" = true
> +	if test "$guessed_merge_tool" = true || test "$prompt" = true
>  	then
>  		printf "Hit return to start merge resolution tool (%s): " "$merge_tool"
>  		read ans || return 1
> @@ -315,7 +315,8 @@ merge_file () {
>  	return 0
>  }
>  
> -prompt=$(git config --bool mergetool.prompt || echo true)
> +prompt=$(git config --bool mergetool.prompt)
> +guessed_merge_tool=false
>  
>  while test $# != 0
>  do
> @@ -373,7 +374,14 @@ prompt_after_failed_merge () {
>  
>  if test -z "$merge_tool"
>  then
> -	merge_tool=$(get_merge_tool "$merge_tool") || exit
> +	# Check if a merge tool has been configured
> +	merge_tool=$(get_configured_merge_tool)
> +	# Try to guess an appropriate merge tool if no tool has been set.
> +	if test -z "$merge_tool"
> +	then
> +		merge_tool=$(guess_merge_tool) || exit
> +		guessed_merge_tool=true
> +	fi
>  fi
>  merge_keep_backup="$(git config --bool mergetool.keepBackup || echo true)"
>  merge_keep_temporaries="$(git config --bool mergetool.keepTemporaries || echo false)"
> -- 
> 1.9.2+fc1.1.g5c924db
> 
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
David

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

[Charles Bailey]

On Mon, Apr 21, 2014 at 09:59:52PM -0700, David Aguilar wrote:
> [Cc:ing Charles in case he has an opinion, this behavior dates back to the original MT]
> 
> On Sun, Apr 20, 2014 at 07:17:34PM -0500, Felipe Contreras wrote:
> > It's annoying to see the prompt:
> > 
> >   Hit return to start merge resolution tool (foo):
> > 
> > Every time the user does 'git mergetool' even if the user already
> > configured 'foo' as the wanted tool.
> > 
> > Display this prompt only when the user hasn't explicitly configured a
> > tool.
> 
> I agree this is probably helpful.
> Most users I've met end up configuring mergetool.prompt=false.

>From my memory, the reason that we choose to prompt by default is to
help new users or users who have just changed their choice of mergetool.

Because we never use the exit code of the tool to determine whether a
tool "worked", if we don't prompt and the tool fails (bad custom
command, requires X when no X available, etc.) then we'll repeatedly run
the command for all paths requiring resolution leading to, potentially,
a lot of trace with whatever error the tool might happen to report.

We prompt by default to give users a chance to abort the mergetool
session at the first opportunity that they see that the configured tool
is not working.

Personally, I leave mergetool.prompt unset (default true) because one
extra click in a complex merge resolution is relatively low overhead and
to catch myself when I forget that I'm in a no-X environment.

I glanced at the patch and it seems to subvert this intent for users
who have  configured a merge tool. Is my understanding correct?

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

[Felipe Contreras]

Charles Bailey wrote:
> On Mon, Apr 21, 2014 at 09:59:52PM -0700, David Aguilar wrote:
> > [Cc:ing Charles in case he has an opinion, this behavior dates back to the original MT]
> > 
> > On Sun, Apr 20, 2014 at 07:17:34PM -0500, Felipe Contreras wrote:
> > > It's annoying to see the prompt:
> > > 
> > >   Hit return to start merge resolution tool (foo):
> > > 
> > > Every time the user does 'git mergetool' even if the user already
> > > configured 'foo' as the wanted tool.
> > > 
> > > Display this prompt only when the user hasn't explicitly configured a
> > > tool.
> > 
> > I agree this is probably helpful.
> > Most users I've met end up configuring mergetool.prompt=false.
> 
> From my memory, the reason that we choose to prompt by default is to
> help new users or users who have just changed their choice of mergetool.
> 
> Because we never use the exit code of the tool to determine whether a
> tool "worked", if we don't prompt and the tool fails (bad custom
> command, requires X when no X available, etc.) then we'll repeatedly run
> the command for all paths requiring resolution leading to, potentially,
> a lot of trace with whatever error the tool might happen to report.
> 
> We prompt by default to give users a chance to abort the mergetool
> session at the first opportunity that they see that the configured tool
> is not working.

This is what I get when a tool is not working:

  Documentation/config.txt seems unchanged.
  Was the merge successful? [y/n]

> Personally, I leave mergetool.prompt unset (default true) because one
> extra click in a complex merge resolution is relatively low overhead and
> to catch myself when I forget that I'm in a no-X environment.
> 
> I glanced at the patch and it seems to subvert this intent for users
> who have  configured a merge tool. Is my understanding correct?

Yes, that's correct. If the user has *manually* configured a tool, why would
you ask him again? We are annoying the overwhelming the vast majority of users
who already configured the right tool in order to avoid issues with a minute
minority that might potentially but unlikely have a problem once or twice.

That's not productive.

-- 
Felipe Contreras

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

[Charles Bailey]

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.

> > Personally, I leave mergetool.prompt unset (default true) because one
> > extra click in a complex merge resolution is relatively low overhead and
> > to catch myself when I forget that I'm in a no-X environment.
> > 
> > I glanced at the patch and it seems to subvert this intent for users
> > who have  configured a merge tool. Is my understanding correct?
> 
> Yes, that's correct. If the user has *manually* configured a tool, why would
> you ask him again? We are annoying the overwhelming the vast majority of users
> who already configured the right tool in order to avoid issues with a minute
> minority that might potentially but unlikely have a problem once or twice.
> 
> That's not productive.

We're asking to user to signal that he's happy to launch the mergetool
and implicitly giving him an opportunity to break out of the session.
I don't see anything wrong with having this behaviour.

So long as we don't hit the loop-with-lots-of-error-trace for users who
haven't set mergetool.prompt I've no strong objections to changing the
default so long as an explictly set mergetool.prompt is respected.

Ideally, I think I'd like the prompt to accept a "launch/skip/abort"
range of options but that's a wider scoped thing. Sometimes when I'm
resolving a lot of things and not specifying any paths I come across
something that know I don't want to attempt a resolve with my currently
configured tool and I just want to skip it for now. Currently, I have to
either abort the session or let the mergetool fire up and close it again
neither of which are optimal.

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

[Felipe Contreras]

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?

> > > Personally, I leave mergetool.prompt unset (default true) because one
> > > extra click in a complex merge resolution is relatively low overhead and
> > > to catch myself when I forget that I'm in a no-X environment.
> > > 
> > > I glanced at the patch and it seems to subvert this intent for users
> > > who have  configured a merge tool. Is my understanding correct?
> > 
> > Yes, that's correct. If the user has *manually* configured a tool, why would
> > you ask him again? We are annoying the overwhelming the vast majority of users
> > who already configured the right tool in order to avoid issues with a minute
> > minority that might potentially but unlikely have a problem once or twice.
> > 
> > That's not productive.
> 
> We're asking to user to signal that he's happy to launch the mergetool
> and implicitly giving him an opportunity to break out of the session.
> I don't see anything wrong with having this behaviour.

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.

-- 
Felipe Contreras

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

[Charles Bailey]

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.

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

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. 

(Yes, I didn't introduce the first "no prompt" option patch and yes, I
have since changed my mind about whether I have it set, but that's just
a personal preference.)

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

[Felipe Contreras]

Charles Bailey wrote:
> 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) ?

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

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

[Charles Bailey]

On Tue, Apr 22, 2014 at 02:56:22AM -0500, Felipe Contreras wrote:
> An explicitly set mergetool.prompt = true would override the default. See the
> patch.

I have had a chance to test the patch now and it looks good. I think
when glancing at it before I missed the change that dropped "|| echo
true" from 

prompt=$(git config --bool mergetool.prompt || echo true)

so I wasn't sure where the implicit true / explicit true difference was
handled.

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

The bit of documentation that I was thinking of is in
Documentation/git-mergetool.txt where it states that "--prompt" is the
default which is now only partially true.

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

[Junio C Hamano]

Charles Bailey <charles@hashpling.org> writes:

> The bit of documentation that I was thinking of is in
> Documentation/git-mergetool.txt where it states that "--prompt" is the
> default which is now only partially true.

Thanks for being careful to help tying the loose ends.

Perhaps like this?

I take that your original motivation was to confirm to run a tool on
this particular (as opposed to another) path, but the user can also
take the prompt as to confirm to run this (as opposed to some other)
tool.  The latter of which of course is irritating for those who
told which exact tool to use.

I think the large part of the reason why you and Felipe had to have
a long back-and-forth was because it was unclear that the prompt
served these two purposes from the documentation, so I attempted to
clarify the first motivation by adding a brief half-sentence.


 Documentation/git-mergetool.txt | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt
index 07137f2..ec089ff 100644
--- a/Documentation/git-mergetool.txt
+++ b/Documentation/git-mergetool.txt
@@ -71,11 +71,14 @@ success of the resolution after the custom tool has exited.
 --no-prompt::
 	Don't prompt before each invocation of the merge resolution
 	program.
+	This is the default if the merge resolution program is
+	explicitly specified with the `--tool` option or with the
+	`merge.tool` configuration variable.
 
 --prompt::
-	Prompt before each invocation of the merge resolution program.
-	This is the default behaviour; the option is provided to
-	override any configuration settings.
+	Prompt before each invocation of the merge resolution program
+	to ask if it should be run for the path.
+
 
 TEMPORARY FILES
 ---------------

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Perhaps like this?
>
> I take that your original motivation was to confirm to run a tool on
> this particular (as opposed to another) path, but the user can also
> take the prompt as to confirm to run this (as opposed to some other)
> tool.  The latter of which of course is irritating for those who
> told which exact tool to use.
>
> I think the large part of the reason why you and Felipe had to have
> a long back-and-forth was because it was unclear that the prompt
> served these two purposes from the documentation, so I attempted to
> clarify the first motivation by adding a brief half-sentence.

I'll queue the following as a separate documentation patch.  We may
want to polish the wording, so I'll keep it out of 'next' for now.

I think the main patch is good for 'next' with or without doc update
to be cooked during the remainder of this cycle, and I merged it
there already.

Thanks.

-- >8 --
Subject: [PATCH] mergetool: document the default for --[no-]prompt

The original motivation of using the prompt was to confirm to run a
tool on this particular (as opposed to another) path, but the user
can also take the prompt as to confirm to run this (as opposed to
some other) tool.  The latter of which of course is irritating for
those who told which exact tool to use, which is the reason why we
are flipping the default.

During the review discussion of the patch, many people (including
the maintainer) missed that a user can find the prompt useful way to
skip running the tool on particular paths.  Clarify it by adding a
brief half-sentence to the description.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git-mergetool.txt | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt
index 07137f2..e846c2e 100644
--- a/Documentation/git-mergetool.txt
+++ b/Documentation/git-mergetool.txt
@@ -71,11 +71,13 @@ success of the resolution after the custom tool has exited.
 --no-prompt::
 	Don't prompt before each invocation of the merge resolution
 	program.
+	This is the default if the merge resolution program is
+	explicitly specified with the `--tool` option or with the
+	`merge.tool` configuration variable.
 
 --prompt::
-	Prompt before each invocation of the merge resolution program.
-	This is the default behaviour; the option is provided to
-	override any configuration settings.
+	Prompt before each invocation of the merge resolution program
+	to give the user a chance to skip the path.
 
 TEMPORARY FILES
 ---------------
-- 
2.0.0-rc0-224-g3c1c0b8

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

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

> [Cc:ing Charles in case he has an opinion, this behavior dates back to the original MT]
>
> On Sun, Apr 20, 2014 at 07:17:34PM -0500, Felipe Contreras wrote:
>> It's annoying to see the prompt:
>> 
>>   Hit return to start merge resolution tool (foo):
>> 
>> Every time the user does 'git mergetool' even if the user already
>> configured 'foo' as the wanted tool.
>> 
>> Display this prompt only when the user hasn't explicitly configured a
>> tool.
>
> I agree this is probably helpful.
> Most users I've met end up configuring mergetool.prompt=false.
>
> Thanks

OK, is it an Ack?

Thanks for CC'ing Charles, by the way.  I think his point about
mentioning the change of default somewhere in the documentation
has some merits, and it can be done in a follow-up patch on top.




>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> ---
>>  git-mergetool.sh | 14 +++++++++++---
>>  1 file changed, 11 insertions(+), 3 deletions(-)
>> 
>> diff --git a/git-mergetool.sh b/git-mergetool.sh
>> index 332528f..d08dc92 100755
>> --- a/git-mergetool.sh
>> +++ b/git-mergetool.sh
>> @@ -277,7 +277,7 @@ merge_file () {
>>  	echo "Normal merge conflict for '$MERGED':"
>>  	describe_file "$local_mode" "local" "$LOCAL"
>>  	describe_file "$remote_mode" "remote" "$REMOTE"
>> -	if "$prompt" = true
>> +	if test "$guessed_merge_tool" = true || test "$prompt" = true
>>  	then
>>  		printf "Hit return to start merge resolution tool (%s): " "$merge_tool"
>>  		read ans || return 1
>> @@ -315,7 +315,8 @@ merge_file () {
>>  	return 0
>>  }
>>  
>> -prompt=$(git config --bool mergetool.prompt || echo true)
>> +prompt=$(git config --bool mergetool.prompt)
>> +guessed_merge_tool=false
>>  
>>  while test $# != 0
>>  do
>> @@ -373,7 +374,14 @@ prompt_after_failed_merge () {
>>  
>>  if test -z "$merge_tool"
>>  then
>> -	merge_tool=$(get_merge_tool "$merge_tool") || exit
>> +	# Check if a merge tool has been configured
>> +	merge_tool=$(get_configured_merge_tool)
>> +	# Try to guess an appropriate merge tool if no tool has been set.
>> +	if test -z "$merge_tool"
>> +	then
>> +		merge_tool=$(guess_merge_tool) || exit
>> +		guessed_merge_tool=true
>> +	fi
>>  fi
>>  merge_keep_backup="$(git config --bool mergetool.keepBackup || echo true)"
>>  merge_keep_temporaries="$(git config --bool mergetool.keepTemporaries || echo false)"
>> -- 
>> 1.9.2+fc1.1.g5c924db
>> 
>> --
>> To unsubscribe from this list: send the line "unsubscribe git" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[David Aguilar]

On Tue, Apr 22, 2014 at 10:19 AM, Junio C Hamano <gitster@pobox.com> wrote:
> David Aguilar <davvid@gmail.com> writes:
>
>> [Cc:ing Charles in case he has an opinion, this behavior dates back to the original MT]
>>
>> On Sun, Apr 20, 2014 at 07:17:34PM -0500, Felipe Contreras wrote:
>>> It's annoying to see the prompt:
>>>
>>>   Hit return to start merge resolution tool (foo):
>>>
>>> Every time the user does 'git mergetool' even if the user already
>>> configured 'foo' as the wanted tool.
>>>
>>> Display this prompt only when the user hasn't explicitly configured a
>>> tool.
>>
>> I agree this is probably helpful.
>> Most users I've met end up configuring mergetool.prompt=false.
>>
>> Thanks
>
> OK, is it an Ack?

Sure thing.

Acked-by: David Aguilar <davvid@gmail.com>

> Thanks for CC'ing Charles, by the way.  I think his point about
> mentioning the change of default somewhere in the documentation
> has some merits, and it can be done in a follow-up patch on top.

Another thing that crossed my mind is that we have -y for --no-prompt
because --prompt was the original default. Maybe a -i (?) shortcut for
the interactive --prompt can be added to make the "need to skip some
when resolving" use case easier to activate.
-- 
David

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

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

> On Tue, Apr 22, 2014 at 10:19 AM, Junio C Hamano <gitster@pobox.com> wrote:
> ...
>> Thanks for CC'ing Charles, by the way.  I think his point about
>> mentioning the change of default somewhere in the documentation
>> has some merits, and it can be done in a follow-up patch on top.
>
> Another thing that crossed my mind is that we have -y for --no-prompt
> because --prompt was the original default. Maybe a -i (?) shortcut for
> the interactive --prompt can be added to make the "need to skip some
> when resolving" use case easier to activate.

Hmm, perhaps, but is "do we prompt to give a chance to the user to
say 'no, I am not interested in running the tool to that path'" the
only interactivity in the overall end-user experience in using the
mergetool?  To end-users, both interaction with the mergetool
front-end and interaction with individual back-end taken together
would comprise the whole end-user experience, so "--interactive"
option that is implied by "-i" short-cut may make them expect a
behaviour from the backend that is more interactive than without,
which would not be the case, so....

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

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

> [Cc:ing Charles in case he has an opinion, this behavior dates back to the original MT]
>
> On Sun, Apr 20, 2014 at 07:17:34PM -0500, Felipe Contreras wrote:
>> It's annoying to see the prompt:
>> 
>>   Hit return to start merge resolution tool (foo):
>> 
>> Every time the user does 'git mergetool' even if the user already
>> configured 'foo' as the wanted tool.
>> 
>> Display this prompt only when the user hasn't explicitly configured a
>> tool.
>
> I agree this is probably helpful.
> Most users I've met end up configuring mergetool.prompt=false.
>
> Thanks

Do you have reaction to the other one "[PATCH 1/2] merge: enable
defaulttoupstream by default"?

As I do not think (note: I am not saying "I do not know" here) it is
sensible to do these "flip the default" in 2.0, I am hoping that we
can queue them (if the area maintainer, you, agree they are good
changes) to 'next' and cook them for the remainder of this cycle.
2.0 is a place where we execute the "flip the default" that we have
already disussed, designed and agreed since around 1.8.x timeframe,
and I would like to exclude any user visible change that just got
started being discussed to avoid unnecessary fallouts.

Thanks.

>
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> ---
>>  git-mergetool.sh | 14 +++++++++++---
>>  1 file changed, 11 insertions(+), 3 deletions(-)
>> 
>> diff --git a/git-mergetool.sh b/git-mergetool.sh
>> index 332528f..d08dc92 100755
>> --- a/git-mergetool.sh
>> +++ b/git-mergetool.sh
>> @@ -277,7 +277,7 @@ merge_file () {
>>  	echo "Normal merge conflict for '$MERGED':"
>>  	describe_file "$local_mode" "local" "$LOCAL"
>>  	describe_file "$remote_mode" "remote" "$REMOTE"
>> -	if "$prompt" = true
>> +	if test "$guessed_merge_tool" = true || test "$prompt" = true
>>  	then
>>  		printf "Hit return to start merge resolution tool (%s): " "$merge_tool"
>>  		read ans || return 1
>> @@ -315,7 +315,8 @@ merge_file () {
>>  	return 0
>>  }
>>  
>> -prompt=$(git config --bool mergetool.prompt || echo true)
>> +prompt=$(git config --bool mergetool.prompt)
>> +guessed_merge_tool=false
>>  
>>  while test $# != 0
>>  do
>> @@ -373,7 +374,14 @@ prompt_after_failed_merge () {
>>  
>>  if test -z "$merge_tool"
>>  then
>> -	merge_tool=$(get_merge_tool "$merge_tool") || exit
>> +	# Check if a merge tool has been configured
>> +	merge_tool=$(get_configured_merge_tool)
>> +	# Try to guess an appropriate merge tool if no tool has been set.
>> +	if test -z "$merge_tool"
>> +	then
>> +		merge_tool=$(guess_merge_tool) || exit
>> +		guessed_merge_tool=true
>> +	fi
>>  fi
>>  merge_keep_backup="$(git config --bool mergetool.keepBackup || echo true)"
>>  merge_keep_temporaries="$(git config --bool mergetool.keepTemporaries || echo false)"
>> -- 
>> 1.9.2+fc1.1.g5c924db
>> 
>> --
>> To unsubscribe from this list: send the line "unsubscribe git" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Do you have reaction to the other one "[PATCH 1/2] merge: enable
> defaulttoupstream by default"?

Sorry, that other one is not even about difftool/mergetool.

I just checked that patch myself, and found it sensible for the
longer term.