threads / patch / 15992

patch, 3 partsFix some tab/space inconsistencies in git-mergetool.sh

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

## tl;dr

13 messages between Oct 21, 2008 and Nov 13, 2008. Diffs are folded; open one to read it.

replies: 12people: 5as markdown or json

Charles Bailey· Oct 21, 2008, 10:13 UTC · lore

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(-)
Show changes to git-mergetool.sh +19 −19
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
Charles Bailey· Oct 21, 2008, 10:13 UTC · re: Charles Bailey · lore

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

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(-)
Show changes to 3 files +26 −4

Documentation/config.txt, Documentation/git-mergetool.txt, git-mergetool.sh

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
Charles Bailey· Oct 21, 2008, 10:13 UTC · re: Charles Bailey · lore

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

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(-)
Show changes to 3 files +51 −11

Documentation/config.txt, Documentation/git-mergetool.txt, git-mergetool.sh

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
Jeff King· Oct 21, 2008, 11:12 UTC · re: Charles Bailey · lore

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

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
Andreas Ericsson· Oct 21, 2008, 11:49 UTC · re: Charles Bailey · lore

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

Charles Bailey wrote:
Show 48 quoted lines
> 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
Charles Bailey· Oct 21, 2008, 12:26 UTC · re: Andreas Ericsson · lore

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

On Tue, Oct 21, 2008 at 01:49:30PM +0200, Andreas Ericsson wrote:
Show 5 quoted lines
>
> 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/
Charles Bailey· Oct 22, 2008, 21:17 UTC · re: Charles Bailey · lore

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

On Tue, Oct 21, 2008 at 01:26:55PM +0100, Charles Bailey wrote:
Show 15 quoted lines
> 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/
Junio C Hamano· Oct 22, 2008, 23:21 UTC · re: Charles Bailey · lore

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

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.

Charles Bailey· Oct 23, 2008, 06:44 UTC · re: Junio C Hamano · lore

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

On Wed, Oct 22, 2008 at 04:21:21PM -0700, Junio C Hamano wrote:
Show 11 quoted lines
> 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/
William Pursell· Oct 24, 2008, 22:32 UTC · re: Charles Bailey · lore

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

Charles Bailey wrote:
Show 9 quoted lines
> 
> 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
Charles Bailey· Oct 24, 2008, 22:55 UTC · re: William Pursell · lore

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

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/
William Pursell· Oct 25, 2008, 10:11 UTC · re: Charles Bailey · lore

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

Charles Bailey wrote:
Show 11 quoted lines
> 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
Charles Bailey· Nov 13, 2008, 12:28 UTC · re: William Pursell · lore

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

On Sat, Oct 25, 2008 at 11:11:14AM +0100, William Pursell wrote:
Show 13 quoted lines
>
> 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/

← back to recent threads