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

3 messages, 3 authors, 2016-06-15 · open the first message on its own page

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

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:43:01

arjen@yaph.org (Arjen Laarhoven) writes:
Signed-off-by: Arjen Laarhoven <redacted>
I cannot comment on the calling interface of opendiff, as I do
not have access to an Apple.  Here are my first impressions.
quoted hunk
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.
+	    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.

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

From: Steven Grimm <hidden>
Date: 2016-06-15 22:43:01

Junio C Hamano wrote:
quoted
+                opendiff $LOCAL $REMOTE -merge $path | cat
What's the purpose of this cat anyway?  It looks like an
expensive no-op to me.
  
If stdout is a tty, opendiff appears to background itself automatically. 
I assume he wanted to prevent that without losing any output from the 
command.

-Steve

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

From: Arjen Laarhoven <hidden>
Date: 2016-06-15 22:43:01

Hi,
I cannot comment on the calling interface of opendiff, as I do
not have access to an Apple.  Here are my first impressions.
quoted
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.
quoted
+	    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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help