[PATCH] mergetool merge/skip/abort at prompt

[PATCH] mergetool merge/skip/abort at prompt

[Caleb Cushing]

previously git mergetool when run with prompt only allowed the user to continue
merging. This changes git mergetool to allow the option of skipping a file or
aborting, and includes an addtional key to explicitly select merge.

Signed-off-by: Caleb Cushing <xenoterracide@gmail.com>
---
 git-mergetool.sh |   20 ++++++++++++++++++--
 1 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/git-mergetool.sh b/git-mergetool.sh
index 00e1337..575fbb2 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -177,8 +177,24 @@ merge_file () {
     describe_file "$local_mode" "local" "$LOCAL"
     describe_file "$remote_mode" "remote" "$REMOTE"
     if "$prompt" = true; then
-	printf "Hit return to start merge resolution tool (%s): " "$merge_tool"
-	read ans
+	while true; do
+	    printf "Use (m)erge file or (s)kip file, or (a)bort? (%s): " \
+	    "$merge_tool"
+	    read ans
+	    case "$ans" in
+		[mM]*|"")
+		    break
+		    ;;
+		[sS]*)
+		    cleanup_temp_files
+		    return 0
+		    ;;
+		[aA]*)
+		    cleanup_temp_files
+		    exit 0
+		    ;;
+	    esac
+	done
     fi

     case "$merge_tool" in
-- 
1.6.1.1

Re: [PATCH] mergetool merge/skip/abort at prompt

[Caleb Cushing]

better? I think I got the formatting fixed... got the imap working...
added a more descriptive commit message. hoping that everything looks
good.

Re: [PATCH] mergetool merge/skip/abort at prompt

[David Aguilar]

Hi Caleb

On Tue, Jan 27, 2009 at 10:56 PM, Caleb Cushing <xenoterracide@gmail.com> wrote:
> previously git mergetool when run with prompt only allowed the user to continue
> merging. This changes git mergetool to allow the option of skipping a file or
> aborting, and includes an addtional key to explicitly select merge.
>
> Signed-off-by: Caleb Cushing <xenoterracide@gmail.com>
> ---
>  git-mergetool.sh |   20 ++++++++++++++++++--
>  1 files changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/git-mergetool.sh b/git-mergetool.sh
> index 00e1337..575fbb2 100755
> --- a/git-mergetool.sh
> +++ b/git-mergetool.sh
> @@ -177,8 +177,24 @@ merge_file () {
>     describe_file "$local_mode" "local" "$LOCAL"
>     describe_file "$remote_mode" "remote" "$REMOTE"
>     if "$prompt" = true; then
> -       printf "Hit return to start merge resolution tool (%s): " "$merge_tool"
> -       read ans
> +       while true; do
> +           printf "Use (m)erge file or (s)kip file, or (a)bort? (%s): " \


I really like the feature you added here.  I'm sorry to bikeshed on
this conversation, but after trying it I have one tiny suggestion.

Right now the prompt looks like this with your patch:

"""
Merging the files: foo bar baz

Normal merge conflict for 'foo':
  {local}: created
  {remote}: created
Use (m)erge file or (s)kip file, or (a)bort? (xxdiff): m
"""

do you think
"Use merge file or skip file, or abort?"
might be better expressed as:

"""
Merging: foo bar baz

Normal merge conflict for 'foo':
  {local}: created
  {remote}: created
(m)erge, (s)kip, or (q)uit? (xxdiff): m
"""?

I realize that your patch only touches the last line of the prompt
(and not the introductory "Merging the files:" line) so if you agree
then maybe I can throw a patch together for the introductory line.

Also, my example has quit instead of abort for two reasons (the first
one is silly)
1. skip rhymes with quit, so it reads very nicely out loud
2. consistency with git add --interactive
3. less typos (q and s are diagonal on qwerty, s and a are adjacent)
(okay, that last one is silly too)

Some might also mis-associate 'abort' with meaning "abort the merge."

slightly off-topic:
If we're looking at cleaning up mergetool a bit would you all mind a
separate patch to convert it to using hard tabs throughout, just like
git-rebase.sh?


> +           "$merge_tool"
> +           read ans
> +           case "$ans" in
> +               [mM]*|"")
> +                   break
> +                   ;;
> +               [sS]*)
> +                   cleanup_temp_files
> +                   return 0
> +                   ;;
> +               [aA]*)
> +                   cleanup_temp_files
> +                   exit 0
> +                   ;;
> +           esac
> +       done
>     fi
>
>     case "$merge_tool" in
> --
> 1.6.1.1
> --
> 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] mergetool merge/skip/abort at prompt

[Charles Bailey]

On Wed, Jan 28, 2009 at 01:56:47AM -0500, Caleb Cushing wrote:
> diff --git a/git-mergetool.sh b/git-mergetool.sh
> index 00e1337..575fbb2 100755
> --- a/git-mergetool.sh
> +++ b/git-mergetool.sh
> @@ -177,8 +177,24 @@ merge_file () {
>      describe_file "$local_mode" "local" "$LOCAL"
>      describe_file "$remote_mode" "remote" "$REMOTE"
>      if "$prompt" = true; then
> -	printf "Hit return to start merge resolution tool (%s): " "$merge_tool"
> -	read ans
> +	while true; do
> +	    printf "Use (m)erge file or (s)kip file, or (a)bort? (%s): " \
> +	    "$merge_tool"
> +	    read ans
> +	    case "$ans" in
> +		[mM]*|"")
> +		    break
> +		    ;;
> +		[sS]*)
> +		    cleanup_temp_files
> +		    return 0
> +		    ;;
> +		[aA]*)
> +		    cleanup_temp_files
> +		    exit 0
> +		    ;;
> +	    esac
> +	done

This patch does now apply for me, so I've given it a longer look. It
does roughly what I expect, but I can't help feeling that the change
isn't in the best place.

Currently, whatever the prompt, the merge tool will always be run so
it makes sense (or at least there is no negative) in creating the
temporary files before running the merge tool.

With this change, it would seem to be more logical to ask whether the
merge tool is to be run before creating the temporary files, removing
the need for them to be cleaned up if the answer is no. I think that
this would be cleaner overall.

At the same time, however, it might be worth refactoring the
merge_file function as the same criticism could probably levelled at
the code paths that perform symlink and deleted file merges and these
paths would probably now share much more of the logic and behaviour of
a normal file merge.

Trying out this refactoring and adding the option to choose local or
remote file versions without running the merge tool has been on my
todo list for a while, but I might actually have a go at it this
weekend if nobody beats me to it.

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

Re: [PATCH] mergetool merge/skip/abort at prompt

[Caleb Cushing]

>  Also, my example has quit instead of abort for two reasons (the first
>  one is silly)
>  1. skip rhymes with quit, so it reads very nicely out loud
>  2. consistency with git add --interactive
>  3. less typos (q and s are diagonal on qwerty, s and a are adjacent)
>  (okay, that last one is silly too)

I chose abort because it's used in other places in mergetool (for same
purpose). I'm not opposed to cleaning up or making it more consistent
with other utilities though. but perhaps that's for another patch...

>  slightly off-topic:
>  If we're looking at cleaning up mergetool a bit would you all mind a
>  separate patch to convert it to using hard tabs throughout, just like
>  git-rebase.sh?

in an earlier thread... I complained loudly about the mixing of tabs
and spaces, it should be banned imho, causes nothing but problems.

-- 
Caleb Cushing

http://xenoterracide.blogspot.com

Re: [PATCH] mergetool merge/skip/abort at prompt

[Caleb Cushing]

>  With this change, it would seem to be more logical to ask whether the
>  merge tool is to be run before creating the temporary files, removing
>  the need for them to be cleaned up if the answer is no. I think that
>  this would be cleaner overall.

I agree, but to be honest, I couldn't get the logic wrapped around my
head, so I did it this way. refactoring does seem to be the best idea,
but I don't understand enough of it yet to do so.

-- 
Caleb Cushing

http://xenoterracide.blogspot.com