Re: [PATCH] Teach git-mergetool about Apple's opendiff/FileMerge

From: Junio C Hamano <junkio@cox.net>
Date: Mar 23, 2007, 04:45 UTC
Message-ID: <7vbqiksh4a.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <20070322213728.GD3854@regex.yaph.org>

arjen@yaph.org (Arjen Laarhoven) writes:

> Signed-off-by: Arjen Laarhoven <arjen@yaph.org>

I cannot comment on the calling interface of opendiff, as I do not have access to an Apple. Here are my first impressions.

Show 15 quoted lines

> diff --git a/git-mergetool.sh b/git-mergetool.sh
> index 7942fd0..58ae201 100755
> --- a/git-mergetool.sh
> +++ b/git-mergetool.sh
> @@ -248,6 +248,30 @@ merge_file () {
>  		mv -- "$BACKUP" "$path.orig"
>  	    fi
>  	    ;;
> +	opendiff)
> +	    touch "$BACKUP"
> +	    if base_present; then
> +		opendiff $LOCAL $REMOTE -ancestor $BASE -merge $path | cat
> +            else
> +                opendiff $LOCAL $REMOTE -merge $path | cat
> +            fi

I sense inconsistent tabbing here.

More seriously, all of the above $variable references must be dq'ed; see other case arms for good examples.

What's the purpose of this cat anyway? It looks like an expensive no-op to me.

Show 18 quoted lines

> +	    if test "$path" -nt "$BACKUP" ; then
> +		status=0;
> +	    else
> +		while true; do
> +		    echo "$path seems unchanged."
> +		    echo -n "Was the merge successful? [y/n] "
> +		    read answer < /dev/tty
> +		    case "$answer" in
> +			y*|Y*) status=0; break ;;
> +			n*|N*) status=1; break ;;
> +		    esac
> +		done
> +	    fi
> +	    if test "$status" -eq 0; then
> +		mv -- "$BACKUP" "$path.orig"
> +	    fi
> +	    ;;
>      esac

This part is duplicated across meld|vimdiff and xxdiff arms; you probably would want to have a patch that makes a shell function to factor this out, and then another patch to add this opendiff support.