threads / patch / 17407

patchmergetool merge/skip/abort at prompt

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

## tl;dr

6 messages between Jan 28, 2009 and Jan 28, 2009. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

Caleb Cushing· Jan 28, 2009, 06:56 UTC · lore

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(-)
Show changes to git-mergetool.sh +18 −2
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
David Aguilar· Jan 28, 2009, 08:04 UTC · re: Caleb Cushing · lore

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

Hi Caleb
On Tue, Jan 27, 2009 at 10:56 PM, Caleb Cushing <xenoterracide@gmail.com> wrote:
Show 21 quoted lines
> 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?

Show 26 quoted lines
> +           "$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
Caleb Cushing· Jan 28, 2009, 09:50 UTC · re: David Aguilar · lore

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

Show 6 quoted lines
>  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
Charles Bailey· Jan 28, 2009, 08:47 UTC · re: Caleb Cushing · lore

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

On Wed, Jan 28, 2009 at 01:56:47AM -0500, Caleb Cushing wrote:
Show 28 quoted lines
> 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/
Caleb Cushing· Jan 28, 2009, 09:53 UTC · re: Charles Bailey · lore

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

>  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

← back to recent threads