git mergetool enhancements

git mergetool enhancements

[Charles Bailey]

Reroll of previously sent mergetool enhancements. Now using -y as the short
option for --no-prompt, but otherwise the same as before.

[PATCH 1/3] Fix some tab/space inconsistencies in git-mergetool.sh

[Charles Bailey]

git-mergetool.sh mostly uses 8 space tabs and 4 spaces per indent. This
change corrects this in a part of the file affect by a later commit in
this patch series. diff -w considers this change is to be a null change.

Signed-off-by: Charles Bailey <charles@hashpling.org>
---
 git-mergetool.sh |   38 +++++++++++++++++++-------------------
 1 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/git-mergetool.sh b/git-mergetool.sh
index 94187c3..e2da5fc 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -401,25 +401,25 @@ fi
 
 
 if test $# -eq 0 ; then
-	files=`git ls-files -u | sed -e 's/^[^	]*	//' | sort -u`
-	if test -z "$files" ; then
-		echo "No files need merging"
-		exit 0
-	fi
-	echo Merging the files: "$files"
-	git ls-files -u |
-	sed -e 's/^[^	]*	//' |
-	sort -u |
-	while IFS= read i
-	do
-		printf "\n"
-		merge_file "$i" < /dev/tty > /dev/tty
-	done
+    files=`git ls-files -u | sed -e 's/^[^	]*	//' | sort -u`
+    if test -z "$files" ; then
+	echo "No files need merging"
+	exit 0
+    fi
+    echo Merging the files: "$files"
+    git ls-files -u |
+    sed -e 's/^[^	]*	//' |
+    sort -u |
+    while IFS= read i
+    do
+	printf "\n"
+	merge_file "$i" < /dev/tty > /dev/tty
+    done
 else
-	while test $# -gt 0; do
-		printf "\n"
-		merge_file "$1"
-		shift
-	done
+    while test $# -gt 0; do
+	printf "\n"
+	merge_file "$1"
+	shift
+    done
 fi
 exit 0
-- 
1.6.0.2.534.g5ab59

[PATCH 2/3] Add -y/--no-prompt option to mergetool

[Charles Bailey]

This option lets git mergetool invoke the conflict resolution program
without waiting for a user prompt each time.

Also added a mergetool.prompt (default true) configuration variable
controlling the same behaviour

Signed-off-by: Charles Bailey <charles@hashpling.org>
---
 Documentation/config.txt        |    3 +++
 Documentation/git-mergetool.txt |   11 ++++++++++-
 git-mergetool.sh                |   16 +++++++++++++---
 3 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 965ed74..c5b211a 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -976,6 +976,9 @@ mergetool.keepBackup::
 	is set to `false` then this file is not preserved.  Defaults to
 	`true` (i.e. keep the backup files).
 
+mergetool.prompt::
+	Prompt before each invocation of the merge resolution program.
+
 pack.window::
 	The size of the window used by linkgit:git-pack-objects[1] when no
 	window size is given on the command line. Defaults to 10.
diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt
index e0b2703..176483a 100644
--- a/Documentation/git-mergetool.txt
+++ b/Documentation/git-mergetool.txt
@@ -7,7 +7,7 @@ git-mergetool - Run merge conflict resolution tools to resolve merge conflicts
 
 SYNOPSIS
 --------
-'git mergetool' [--tool=<tool>] [<file>]...
+'git mergetool' [--tool=<tool>] [-y|--no-prompt|--prompt] [<file>]...
 
 DESCRIPTION
 -----------
@@ -60,6 +60,15 @@ variable `mergetool.<tool>.trustExitCode` can be set to `true`.
 Otherwise, 'git-mergetool' will prompt the user to indicate the
 success of the resolution after the custom tool has exited.
 
+-y or --no-prompt::
+	Don't prompt before each invocation of the merge resolution
+	program.
+
+--prompt::
+	Prompt before each invocation of the merge resolution program.
+	This is the default behaviour; the option is provided to
+	override any configuration settings.
+
 Author
 ------
 Written by Theodore Y Ts'o <tytso@mit.edu>
diff --git a/git-mergetool.sh b/git-mergetool.sh
index e2da5fc..507028f 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -8,7 +8,7 @@
 # at the discretion of Junio C Hamano.
 #
 
-USAGE='[--tool=tool] [file to merge] ...'
+USAGE='[--tool=tool] [-y|--no-prompt|--prompt] [file to merge] ...'
 SUBDIRECTORY_OK=Yes
 OPTIONS_SPEC=
 . git-sh-setup
@@ -176,8 +176,10 @@ merge_file () {
     echo "Normal merge conflict for '$MERGED':"
     describe_file "$local_mode" "local" "$LOCAL"
     describe_file "$remote_mode" "remote" "$REMOTE"
-    printf "Hit return to start merge resolution tool (%s): " "$merge_tool"
-    read ans
+    if "$prompt" = true; then
+	printf "Hit return to start merge resolution tool (%s): " "$merge_tool"
+	read ans
+    fi
 
     case "$merge_tool" in
 	kdiff3)
@@ -280,6 +282,8 @@ merge_file () {
     cleanup_temp_files
 }
 
+prompt=$(git config --bool mergetool.prompt || echo true)
+
 while test $# != 0
 do
     case "$1" in
@@ -295,6 +299,12 @@ do
 		    shift ;;
 	    esac
 	    ;;
+	-y|--no-prompt)
+	    prompt=false
+	    ;;
+	--prompt)
+	    prompt=true
+	    ;;
 	--)
 	    break
 	    ;;
-- 
1.6.0.2.534.g5ab59

[PATCH 3/3] Add -k/--keep-going option to mergetool

[Charles Bailey]

This option stops git mergetool from aborting at the first failed merge.
This allows some additional use patterns. Merge conflicts can now be
previewed one at time and merges can also be skipped so that they can be
performed in a later pass.

There is also a mergetool.keepGoing configuration variable covering the
same behaviour.

Signed-off-by: Charles Bailey <charles@hashpling.org>
---
 Documentation/config.txt        |    4 +++
 Documentation/git-mergetool.txt |   12 +++++++++-
 git-mergetool.sh                |   46 ++++++++++++++++++++++++++++++--------
 3 files changed, 51 insertions(+), 11 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index c5b211a..0b0bc99 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -976,6 +976,10 @@ mergetool.keepBackup::
 	is set to `false` then this file is not preserved.  Defaults to
 	`true` (i.e. keep the backup files).
 
+mergetool.keepGoing::
+	Continue to attempt resolution of remaining conflicted files even
+	after a merge has failed or been aborted.
+
 mergetool.prompt::
 	Prompt before each invocation of the merge resolution program.
 
diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt
index 176483a..bd2a5ab 100644
--- a/Documentation/git-mergetool.txt
+++ b/Documentation/git-mergetool.txt
@@ -7,7 +7,8 @@ git-mergetool - Run merge conflict resolution tools to resolve merge conflicts
 
 SYNOPSIS
 --------
-'git mergetool' [--tool=<tool>] [-y|--no-prompt|--prompt] [<file>]...
+'git mergetool' [--tool=<tool>] [-y|--no-prompt|--prompt]
+	[-k|--keep-going|--no-keep-going] [<file>]...
 
 DESCRIPTION
 -----------
@@ -69,6 +70,15 @@ success of the resolution after the custom tool has exited.
 	This is the default behaviour; the option is provided to
 	override any configuration settings.
 
+-k or --keep-going::
+	Continue to attempt resolution of remaining conflicted files
+	even after a merge has failed or been aborted.
+
+--no-keep-going::
+	Abort the conflict resolution attempt if any single conflict
+	resolution fails or is aborted. This is the default behaviour;
+	the option is provided to override any configuration settings.
+
 Author
 ------
 Written by Theodore Y Ts'o <tytso@mit.edu>
diff --git a/git-mergetool.sh b/git-mergetool.sh
index 507028f..d60f493 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -8,7 +8,8 @@
 # at the discretion of Junio C Hamano.
 #
 
-USAGE='[--tool=tool] [-y|--no-prompt|--prompt] [file to merge] ...'
+USAGE='[--tool=tool] [-y|--no-prompt|--prompt]
+[-k|--keep-going|--no-keep-going] [file to merge] ...'
 SUBDIRECTORY_OK=Yes
 OPTIONS_SPEC=
 . git-sh-setup
@@ -70,16 +71,16 @@ resolve_symlink_merge () {
 		git checkout-index -f --stage=2 -- "$MERGED"
 		git add -- "$MERGED"
 		cleanup_temp_files --save-backup
-		return
+		return 0
 		;;
 	    [rR]*)
 		git checkout-index -f --stage=3 -- "$MERGED"
 		git add -- "$MERGED"
 		cleanup_temp_files --save-backup
-		return
+		return 0
 		;;
 	    [aA]*)
-		exit 1
+		return 1
 		;;
 	    esac
 	done
@@ -97,15 +98,15 @@ resolve_deleted_merge () {
 	    [mMcC]*)
 		git add -- "$MERGED"
 		cleanup_temp_files --save-backup
-		return
+		return 0
 		;;
 	    [dD]*)
 		git rm -- "$MERGED" > /dev/null
 		cleanup_temp_files
-		return
+		return 0
 		;;
 	    [aA]*)
-		exit 1
+		return 1
 		;;
 	    esac
 	done
@@ -137,7 +138,7 @@ merge_file () {
 	else
 	    echo "$MERGED: file does not need merging"
 	fi
-	exit 1
+	return 1
     fi
 
     ext="$$$(expr "$MERGED" : '.*\(\.[^/]*\)$')"
@@ -269,7 +270,8 @@ merge_file () {
     if test "$status" -ne 0; then
 	echo "merge of $MERGED failed" 1>&2
 	mv -- "$BACKUP" "$MERGED"
-	exit 1
+	cleanup_temp_files
+	return 1
     fi
 
     if test "$merge_keep_backup" = "true"; then
@@ -280,9 +282,11 @@ merge_file () {
 
     git add -- "$MERGED"
     cleanup_temp_files
+    return 0
 }
 
 prompt=$(git config --bool mergetool.prompt || echo true)
+keep_going=$(git config --bool mergetool.keepGoing || echo false)
 
 while test $# != 0
 do
@@ -305,6 +309,12 @@ do
 	--prompt)
 	    prompt=true
 	    ;;
+	-k|--keep-going)
+	    keep_going=true
+	    ;;
+	--no-keep-going)
+	    keep_going=false
+	    ;;
 	--)
 	    break
 	    ;;
@@ -409,6 +419,7 @@ else
     fi
 fi
 
+rollup_status=0
 
 if test $# -eq 0 ; then
     files=`git ls-files -u | sed -e 's/^[^	]*	//' | sort -u`
@@ -424,12 +435,27 @@ if test $# -eq 0 ; then
     do
 	printf "\n"
 	merge_file "$i" < /dev/tty > /dev/tty
+	if test $? -ne 0; then
+	    if test "$keep_going" = true; then
+		rollup_status=1
+	    else
+		exit 1
+	    fi
+	fi
     done
 else
     while test $# -gt 0; do
 	printf "\n"
 	merge_file "$1"
+	if test $? -ne 0; then
+	    if test "$keep_going" = true; then
+		rollup_status=1
+	    else
+		exit 1
+	    fi
+	fi
 	shift
     done
 fi
-exit 0
+
+exit $rollup_status
-- 
1.6.0.2.534.g5ab59

Re: [PATCH 3/3] Add -k/--keep-going option to mergetool

[Jeff King]

On Thu, Nov 13, 2008 at 12:41:15PM +0000, Charles Bailey wrote:

> This option stops git mergetool from aborting at the first failed merge.
> This allows some additional use patterns. Merge conflicts can now be
> previewed one at time and merges can also be skipped so that they can be
> performed in a later pass.
> 
> There is also a mergetool.keepGoing configuration variable covering the
> same behaviour.

I think this series addresses the questions I remember from the first
posting, and it looks sane overall to me. My only two complaints are:

>  Documentation/config.txt        |    4 +++
>  Documentation/git-mergetool.txt |   12 +++++++++-
>  git-mergetool.sh                |   46 ++++++++++++++++++++++++++++++--------
>  3 files changed, 51 insertions(+), 11 deletions(-)

This patch in particular changes some of the innards of mergetool, and
while the changes look good to me via reading, I would feel even more
comfortable if there were some tests (it looks like your previous t7610
gives at least a basic sanity check, but it might be nice to test to
make sure we detect merge failures, too).

> -		return
> +		return 0
> [...]
> -		return
> +		return 0
> [...]
> -		exit 1
> +		return 1
> [...]
> -	exit 1
> +	return 1
> [...]
> -	exit 1
> +	cleanup_temp_files
> +	return 1

One of these is not like the others. Is there a reason to add a
cleanup_temp_files here (and if so, should it be noted in the commit
message, and/or be in a separate commit?).

-Peff

Re: [PATCH 3/3] Add -k/--keep-going option to mergetool

[Charles Bailey]

Jeff King wrote:

>>  Documentation/config.txt        |    4 +++
>>  Documentation/git-mergetool.txt |   12 +++++++++-
>>  git-mergetool.sh                |   46
++++++++++++++++++++++++++++++--------
>>  3 files changed, 51 insertions(+), 11 deletions(-)
>
> This patch in particular changes some of the innards of mergetool, and
> while the changes look good to me via reading, I would feel even more
> comfortable if there were some tests (it looks like your previous t7610
> gives at least a basic sanity check, but it might be nice to test to
> make sure we detect merge failures, too).

I had a little difficulty thinking of some decently robust tests. git
mergetool is, by its nature, more interactive than many git commands.

With --no-prompt it should be easier to make some tests that don't just
hang when it goes wrong. I'll have a further think on this.

>> [...]
>> -	exit 1
>> +	cleanup_temp_files
>> +	return 1
>
> One of these is not like the others. Is there a reason to add a
> cleanup_temp_files here (and if so, should it be noted in the commit
> message, and/or be in a separate commit?).

Ah yes, it's definitely worthy of at least a comment. This is something
that I changed right at the end of testing and I should really have made
it into a separate commit.

Previously, if you aborted a merge, you were left with the
base/local/remote temporaries for the merge that you aborted.

To be honest, I found this a little irritating. The base, local and
remote temporaries are inputs so are accessible from slots 1,2 and 3 of
the index, and any intermediate output will be in the target file. You
can git clean, but if you have other temporaries you need to keep, you
end up having to manually clean them up in any case.

With --keep-going, the problem is compounded. If you make several passes
through a list of complex merges you end up with fistfuls of these
temporary trios in your working tree. It goes from slightly annoying to
very irritating.

Charles.

Re: [PATCH 3/3] Add -k/--keep-going option to mergetool

[Jeff King]

On Fri, Nov 14, 2008 at 01:25:12PM +0000, Charles Bailey wrote:

> Previously, if you aborted a merge, you were left with the
> base/local/remote temporaries for the merge that you aborted.
> 
> To be honest, I found this a little irritating. The base, local and
> remote temporaries are inputs so are accessible from slots 1,2 and 3 of
> the index, and any intermediate output will be in the target file. You
> can git clean, but if you have other temporaries you need to keep, you
> end up having to manually clean them up in any case.
> 
> With --keep-going, the problem is compounded. If you make several passes
> through a list of complex merges you end up with fistfuls of these
> temporary trios in your working tree. It goes from slightly annoying to
> very irritating.

I agree; I have never found the temporary files left by mergetool from a
failed merge to be useful, and get a little annoyed that I have to "git
clean" them away afterwards. Even without --keep-going, I would often
look at and abort a merge several times before resolving it and end up
with several copies.

But I definitely think that is an issue for a separate patch, and one
that needs input from Ted and from other mergetool users.

-Peff

Re: [PATCH 3/3] Add -k/--keep-going option to mergetool

[Theodore Tso]

On Fri, Nov 14, 2008 at 01:25:12PM +0000, Charles Bailey wrote:
> Previously, if you aborted a merge, you were left with the
> base/local/remote temporaries for the merge that you aborted.
> 
> To be honest, I found this a little irritating. The base, local and
> remote temporaries are inputs so are accessible from slots 1,2 and 3 of
> the index, and any intermediate output will be in the target file. You
> can git clean, but if you have other temporaries you need to keep, you
> end up having to manually clean them up in any case.

I agree.  On occasion it's useful, but more often than not, keeping
the temporaries is more annoying than anything else.  At the time when
I wrote mergetool, it was a pain for me to either do "git ls-files
--stage" and then cut and paste the SHA hash, or to type commands like
"git cat-file blob :1:long/pathname/from/the/top.c" to look one
various staged versions of the file.   

I'd suggest that this is probably worth turning into an option
(-k|--keep-files), and default the answer to deleting the temporaries
before mergetool exits.

						- Ted

Re: [PATCH 3/3] Add -k/--keep-going option to mergetool

[Junio C Hamano]

Charles Bailey <charles@hashpling.org> writes:

> This option stops git mergetool from aborting at the first failed merge.
> This allows some additional use patterns. Merge conflicts can now be
> previewed one at time and merges can also be skipped so that they can be
> performed in a later pass.

Hmm, with this command line:

> -'git mergetool' [--tool=<tool>] [-y|--no-prompt|--prompt] [<file>]...

I wonder why this even needs to be an option.  If you do not want to
resolve all of them, you can limit the amount of work you would do by
giving list of paths to work on, can't you?

Re: [PATCH 3/3] Add -k/--keep-going option to mergetool

[Jeff King]

On Fri, Nov 14, 2008 at 09:35:08PM -0800, Junio C Hamano wrote:

> > -'git mergetool' [--tool=<tool>] [-y|--no-prompt|--prompt] [<file>]...
> 
> I wonder why this even needs to be an option.  If you do not want to
> resolve all of them, you can limit the amount of work you would do by
> giving list of paths to work on, can't you?

I don't know about Charles, but I often don't know that I don't want to
merge a file using the graphical tool until I see it in the graphical
tool. IOW, I start trying to merge it there and realize the conflict is
such that I am better off doing it in my editor with conflict markers.

-Peff

Re: [PATCH 3/3] Add -k/--keep-going option to mergetool

[Charles Bailey]

On Fri, Nov 14, 2008 at 09:35:08PM -0800, Junio C Hamano wrote:
> Charles Bailey <charles@hashpling.org> writes:
> 
> > This option stops git mergetool from aborting at the first failed merge.
> > This allows some additional use patterns. Merge conflicts can now be
> > previewed one at time and merges can also be skipped so that they can be
> > performed in a later pass.
> 
> Hmm, with this command line:
> 
> > -'git mergetool' [--tool=<tool>] [-y|--no-prompt|--prompt] [<file>]...
> 
> I wonder why this even needs to be an option.  If you do not want to
> resolve all of them, you can limit the amount of work you would do by
> giving list of paths to work on, can't you?
> 

I have to say that since having this in my tree I've benefitted hugely
from using a visual diff tool as a merge preview tool. I'm working on
a (far from ideal) project where two branches are diverging fast due
to differing goals, yet there is a need for an end product with the
functionality of both branches in it.

This means that I often end up needing to do frequent large merges.
Often it is not until I have the merge open in front of me that it
becomes obvious that while a particular file needs merging, the best
merge strategy will only be obvious after resolving the conflicts in
other parts of the system.

Often several hours of work will just be four or five invocations of
git mergetool until something which compiles emerges. Stopping to view
what the next unmerged file path is and pasting it into another
mergetool command line seems like an unnecessary distraction.

Charles.

Re: [PATCH 3/3] Add -k/--keep-going option to mergetool

[Theodore Tso]

On Thu, Nov 13, 2008 at 12:41:15PM +0000, Charles Bailey wrote:
> This option stops git mergetool from aborting at the first failed merge.
> This allows some additional use patterns. Merge conflicts can now be
> previewed one at time and merges can also be skipped so that they can be
> performed in a later pass.
> 
> There is also a mergetool.keepGoing configuration variable covering the
> same behaviour.

Instead of making this be a command-line and configuration option,
maybe it would be better to prompt the user after an aborted merge,
and give the user the opportunity to continue or abort?  i.e., instead
of just saying "merge of foo.c failed" and then exiting, to ask the
user instad something like, "Merge of foo.c failed; continue
attempting to merge the rest of the files? <y>"

I suspect this might be more friendly than Yet Another command-line
option and configuration parameter.  What do you think?

       	   		 	     	     	 - Ted

Re: [PATCH 3/3] Add -k/--keep-going option to mergetool

[Charles Bailey]

On Sat, Nov 15, 2008 at 10:56:03AM -0500, Theodore Tso wrote:
> Instead of making this be a command-line and configuration option,
> maybe it would be better to prompt the user after an aborted merge,
> and give the user the opportunity to continue or abort?  i.e., instead
> of just saying "merge of foo.c failed" and then exiting, to ask the
> user instad something like, "Merge of foo.c failed; continue
> attempting to merge the rest of the files? <y>"
> 
> I suspect this might be more friendly than Yet Another command-line
> option and configuration parameter.  What do you think?
> 
>        	   		 	     	     	 - Ted

This sounds like a really good alternative interface option. The next
time I have a moment (could be a while!) I'll try and make a patch
based on this idea.

Charles.

Re: [PATCH 2/3] Add -y/--no-prompt option to mergetool

[Theodore Tso]

On Thu, Nov 13, 2008 at 12:41:14PM +0000, Charles Bailey wrote:
> This option lets git mergetool invoke the conflict resolution program
> without waiting for a user prompt each time.
> 
> Also added a mergetool.prompt (default true) configuration variable
> controlling the same behaviour

My apologies for not commeting earlier on your patches; I've been a
bit overloaded as of late --- way too much conference travel!  :-(

I can see why this should perhaps be the default for gui-based merge
program.  The one place where it should perhaps not be the default is if
the tool could be text based (i.e., if you are using text-based emacs
to do the emerge).   So perhaps what we should do is 

1) Make the question of whether or not a particular back-end merge
program should require the user to hit return first to be configured
on a per-gui tool basis.

2) For all of the gui tools default the answer to be to _not_ prompt
first.

3) For the emacs-based merge, make the default be based on whether or
not $DISPLAY is set.  (But allow an overide based on
mergetool.emerge.prompt.)

I think this much more accurately would model what users want, and if
we get the default right for most users, that's definitely a win.

Regards,

							- Ted