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

[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 -n/--no-prompt option to mergetool

[Charles Bailey]

This option lets git mergetool invoke the conflict resolution program
without waiting for a user response 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 29369d0..b4e4ee4 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..6d6bfe0 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>] [-n|--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.
 
+-n 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..8bc5366 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] [-n|--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
 	    ;;
+	-n|--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 b4e4ee4..789c88a 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 6d6bfe0..15466f3 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>] [-n|--no-prompt|--prompt] [<file>]...
+'git mergetool' [--tool=<tool>] [-n|--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 8bc5366..c1e2de9 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -8,7 +8,8 @@
 # at the discretion of Junio C Hamano.
 #
 
-USAGE='[--tool=tool] [-n|--no-prompt|--prompt] [file to merge] ...'
+USAGE='[--tool=tool] [-n|--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 Tue, Oct 21, 2008 at 11:13:19AM +0100, 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.

All 3 patches look good to me, and match what I expected from our
earlier discussion. But I am not too familiar with mergetool, so take my
approval with a grain of salt. :)

-Peff

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

[Andreas Ericsson]

Charles Bailey wrote:
> This option lets git mergetool invoke the conflict resolution program
> without waiting for a user response 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 29369d0..b4e4ee4 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..6d6bfe0 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>] [-n|--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.
>  
> +-n or --no-prompt::
> +	Don't prompt before each invocation of the merge resolution
> +	program.
> +

There is discussion already about "-n should be for dry-run!" and git's
inconsistencies in such matters. Wouldn't -y ("assume yes on prompt")
be better?

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

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

[Charles Bailey]

On Tue, Oct 21, 2008 at 01:49:30PM +0200, Andreas Ericsson wrote:
>
> There is discussion already about "-n should be for dry-run!" and git's
> inconsistencies in such matters. Wouldn't -y ("assume yes on prompt")
> be better?
>

I must have missed this discussion. I've just had a very quick look at
a handful of basic modifying git commands (merge, pull, commit,
checkout, reset, revert) and only found 'add' that used -n as
--dry-run.

That said, I've no real objections to -y if that makes for a better
consensus.

-- 
Charles Bailey
http://ccgi.hashpling.plus.com/blog/

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

[Charles Bailey]

On Tue, Oct 21, 2008 at 01:26:55PM +0100, Charles Bailey wrote:
> On Tue, Oct 21, 2008 at 01:49:30PM +0200, Andreas Ericsson wrote:
> >
> > There is discussion already about "-n should be for dry-run!" and git's
> > inconsistencies in such matters. Wouldn't -y ("assume yes on prompt")
> > be better?
> >
> 
> I must have missed this discussion. I've just had a very quick look at
> a handful of basic modifying git commands (merge, pull, commit,
> checkout, reset, revert) and only found 'add' that used -n as
> --dry-run.
> 
> That said, I've no real objections to -y if that makes for a better
> consensus.
> 

I'm pretty keen on this patch, but have no strong opinions on which
short option is used, so are there any votes against -y?

If I re-roll with the favoured short option is there anything else
that would prevent its adoption into next or master?

-- 
Charles Bailey
http://ccgi.hashpling.plus.com/blog/

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

[Junio C Hamano]

Charles Bailey <charles@hashpling.org> writes:

> I'm pretty keen on this patch, but have no strong opinions on which
> short option is used, so are there any votes against -y?

Between 'n' and 'y', I am in favour of the latter, but at the same time I
have to wonder if there are other commands that would want "Assume yes"
option.  It could be that this single command that prompts for "Is this
Ok" is an oddball and giving it an "interactive" option to trigger the
current behaviour might make things more consistent.  I dunno.

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

[Charles Bailey]

On Wed, Oct 22, 2008 at 04:21:21PM -0700, Junio C Hamano wrote:
> Charles Bailey <charles@hashpling.org> writes:
> 
> > I'm pretty keen on this patch, but have no strong opinions on which
> > short option is used, so are there any votes against -y?
> 
> Between 'n' and 'y', I am in favour of the latter, but at the same time I
> have to wonder if there are other commands that would want "Assume yes"
> option.  It could be that this single command that prompts for "Is this
> Ok" is an oddball and giving it an "interactive" option to trigger the
> current behaviour might make things more consistent.  I dunno.
> 

I think that git mergetool probably counts as at least 'unusual', and
I think that there is some merit in the current default behaviour. It
gives you a prompt at which you can C-c if it's about to run something
that you don't want it to do the first few times that you try
mergetool.

After more thoughts, I'm somewhat in favour of dropping the short
switch altogether. As it just saves a single keypress per merge I
imagine that most mergetool users, once they discover this new
feature and decide that they want to use it, will prefer to use a user
config option to switch it on. The command line option then becomes
something that you would only need to use to override your normal
default and something on which to hang the option description in the
man page. 

-- 
Charles Bailey
http://ccgi.hashpling.plus.com/blog/

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

[William Pursell]

Charles Bailey wrote:
> 
> After more thoughts, I'm somewhat in favour of dropping the short
> switch altogether. As it just saves a single keypress per merge I
> imagine that most mergetool users, once they discover this new
> feature and decide that they want to use it, will prefer to use a user
> config option to switch it on. The command line option then becomes
> something that you would only need to use to override your normal
> default and something on which to hang the option description in the
> man page. 

If the short option is dropped, the config option should
probably associated with mergetool.<tool>.interactive rather
than mergetool.interactive.  (s/interactive/whatever)


-- 
William Pursell

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

[Charles Bailey]

On Fri, Oct 24, 2008 at 11:32:17PM +0100, William Pursell wrote:
>
> If the short option is dropped, the config option should
> probably associated with mergetool.<tool>.interactive rather
> than mergetool.interactive.  (s/interactive/whatever)

I'm not sure I understand your reasoning. The no-prompt/interactive
option affects the behaviour of the mergetool script independent of
which particular merge tool is being used. Why should the presence or
absence of a short option affect whether the config option is global
or per tool?

-- 
Charles Bailey
http://ccgi.hashpling.plus.com/blog/

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

[William Pursell]

Charles Bailey wrote:
> On Fri, Oct 24, 2008 at 11:32:17PM +0100, William Pursell wrote:
>> If the short option is dropped, the config option should
>> probably associated with mergetool.<tool>.interactive rather
>> than mergetool.interactive.  (s/interactive/whatever)
> 
> I'm not sure I understand your reasoning. The no-prompt/interactive
> option affects the behaviour of the mergetool script independent of
> which particular merge tool is being used. Why should the presence or
> absence of a short option affect whether the config option is global
> or per tool?
> 

My thinking is that when using an interactive tool
like vimdiff, the user is probably not going to
care as much about being prompted, or may prefer
to have the prompt in that situation.  However,
if they've written a script to do the merge
non-interactively, then the prompt is undesirable.

So a person might want to be prompted with
git mergetool -t vimdiff, and prefer no prompt
with git mergetool -t my-script.  Being able
to configure the behavior on a per-tool
basis would allow that.

-- 
William Pursell

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

[Charles Bailey]

On Sat, Oct 25, 2008 at 11:11:14AM +0100, William Pursell wrote:
>
> My thinking is that when using an interactive tool
> like vimdiff, the user is probably not going to
> care as much about being prompted, or may prefer
> to have the prompt in that situation.  However,
> if they've written a script to do the merge
> non-interactively, then the prompt is undesirable.
>
> So a person might want to be prompted with
> git mergetool -t vimdiff, and prefer no prompt
> with git mergetool -t my-script.  Being able
> to configure the behavior on a per-tool
> basis would allow that.

I can see your point, although personally I feel that if you've gone
to the trouble of writing a special script, the extra typing of an
extra long option (if you prefer prompting normally) is not that big
compared with the hassle of maintaining per-tool config settings if
you like to regularly swap between merge tools.

Both scenarios are probably not too common so I wouldn't object
strongly to either. (But per tool configs is more dev work!)

-- 
Charles Bailey
http://ccgi.hashpling.plus.com/blog/

[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