[PATCH] Terminate diff-* on non-zero exit from GIT_EXTERNAL_DIFF

From: Junio C Hamano <junkio@cox.net>
Date: May 3, 2005, 19:49 UTC
Message-ID: <7v4qdkw0mu.fsf@assigned-by-dhcp.cox.net>

This patch changes the git-apply-patch-script to exit non-zero when the patch cannot be applied. Previously, the external diff driver deliberately ignored the exit status of GIT_EXTERNAL_DIFF command, which was a design mistake. It now stops the processing when GIT_EXTERNAL_DIFF exits non-zero, so the damages from running git-diff-* with git-apply-patch-script between two wrong trees can be contained.

The "diff" command line built-in driver builds is changed to always exit 0 in order to match this new behaviour. I know Pasky does not use GIT_EXTERNAL_DIFF yet, so this change should not break Cogito, either.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---

diff.c | 19 +++++++++------ git-apply-patch-script | 59 +++++++++++++++++++++++-------------------------- 2 files changed, 39 insertions(+), 39 deletions(-)

# - HEAD: Merge with linus-mirror. # + 2: Use GIT_EXTERNAL_DIFF exit status to terminate diff early.

--- a/diff.c
+++ b/diff.c
@@ -83,7 +83,7 @@ static void builtin_diff(const char *nam
 {
 	int i, next_at;
 	const char *diff_cmd = "diff -L'%s%s' -L'%s%s'";
-	const char *diff_arg  = "'%s' '%s'";
+	const char *diff_arg  = "'%s' '%s'||:"; /* "||:" is to return 0 */
 	const char *input_name_sq[2];
 	const char *path0[2];
 	const char *path1[2];
@@ -261,16 +261,19 @@ void run_external_diff(const char *name,
 			printf("* Unmerged path %s\n", name);
 		exit(0);
 	}
-	if (waitpid(pid, &status, 0) < 0 || !WIFEXITED(status)) {
-		/* We do not check the exit status because typically
+	if (waitpid(pid, &status, 0) < 0 ||
+	    !WIFEXITED(status) || WEXITSTATUS(status)) {
+		/* Earlier we did not check the exit status because
 		 * diff exits non-zero if files are different, and
-		 * we are not interested in knowing that.  We *knew*
-		 * they are different and that's why we ran diff
-		 * in the first place!  However if it dies by a signal,
-		 * we stop processing immediately.
+		 * we are not interested in knowing that.  It was a
+		 * mistake which made it harder to quit a diff-*
+		 * session that uses the git-apply-patch-script as
+		 * the GIT_EXTERNAL_DIFF.  A custom GIT_EXTERNAL_DIFF
+		 * should also exit non-zero only when it wants to
+		 * abort the entire diff-* session.
 		 */
 		remove_tempfile();
-		die("external diff died unexpectedly.\n");
+		die("external diff died, stopping at %s.\n", name);
 	}
 	remove_tempfile();
 }
--- a/git-apply-patch-script
+++ b/git-apply-patch-script
@@ -21,38 +21,35 @@ then
 fi
 # This will say "patching ..." so we do not say anything outselves.
 
-diff -u -L "a/$name" -L "b/$name" "$tmp1" "$tmp2" | patch -p1
-test -f "$name.rej" || {
-    case "$mode1,$mode2" in
-    .,?x)
-	# newly created
-	case "$mode2" in
-	+x)
-	    echo >&2 "created $name with mode +x."
-	    chmod "$mode2" "$name"
-	    ;;
-	-x)
-	    echo >&2 "created $name."
-	    ;;
-	esac
-	git-update-cache --add -- "$name"
+diff -u -L "a/$name" -L "b/$name" "$tmp1" "$tmp2" | patch -p1 || exit
+case "$mode1,$mode2" in
+.,?x)
+    # newly created
+    case "$mode2" in
+    +x)
+	echo >&2 "created $name with mode +x."
+	chmod "$mode2" "$name"
 	;;
-    ?x,.)
-	# deleted
-	echo >&2 "deleted $name."
-	rm -f "$name"
-	git-update-cache --remove -- "$name"
+    -x)
+	echo >&2 "created $name."
 	;;
+    esac
+    git-update-cache --add -- "$name"
+    ;;
+?x,.)
+    # deleted
+    echo >&2 "deleted $name."
+    rm -f "$name"
+    git-update-cache --remove -- "$name"
+    ;;
+*)
+    # changed
+    case "$mode1,$mode2" in
+    "$mode2,$mode1") ;;
     *)
-	# changed
-	case "$mode1,$mode2" in
-	"$mode2,$mode1") ;;
-	*)
-	    echo >&2 "changing mode from $mode1 to $mode2."
-	    chmod "$mode2" "$name"
-	    ;;
-	esac
-	git-update-cache -- "$name"
+	echo >&2 "changing mode from $mode1 to $mode2."
+	chmod "$mode2" "$name"
+	;;
     esac
-}
-exit 0
+    git-update-cache -- "$name"
+esac