Re: [PATCH] Teach git-mergetool about Apple's opendiff/FileMerge
Show 20 quoted lines
> 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.> 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.
Show 23 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.
Arjen Laarhoven
