From: Arjen Laarhoven <arjen@yaph.org>
Date: Fri, 23 Mar 2007 08:25:01 GMT
Subject: Re: [PATCH] Teach git-mergetool about Apple's opendiff/FileMerge
Message-ID: <20070323082501.GF3854@regex.yaph.org>
In-Reply-To: <7vbqiksh4a.fsf@assigned-by-dhcp.cox.net>

Hi,

> I cannot comment on the calling interface of opendiff, as I do
> not have access to an Apple.  Here are my first impressions.
> 
> > 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.

Somehow I missed this.

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

I don't use shell scripting  much, some reading up on quoting
enlightened me :-)

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

opendiff is a wrapper for the FileMerge.app application.  It launches the
FileMerge binary with the expanded filenames and returns immediately,
which is confusing, as git-mergetool immediately continues.  When the
output of opendiff is piped somewhere, it'll wait until FileMerge is
exited (and the user has had a chance to save the merged file).

I think there is another solution, I'll look into this.

> > +	    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.

Will do.

Arjen