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/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
