Make git diff-generation use a simpler spawn-like interface

Make git diff-generation use a simpler spawn-like interface

[Linus Torvalds]

Instead of depending of fork() and execve() and doing things in between
the two, make the git diff functions do everything up front, and then do
a single "spawn_prog()" invocation to run the actual external diff
program (if any is even needed).

This actually ends up simplifying the code, and should make it much
easier to make it efficient under broken operating systems (read: Windows).

Signed-off-by: Linus Torvalds <torvalds@osdl.org>
---

Now, somebody else might disagree with my contention that this also makes 
things simpler, but I like how we separate out the "run an external 
program" phase from the actual setup and header printing phase.

Of course, it still uses fork/execvp here, but now it should be _trivial_ 
to just have a windows-optimized version of "spawn_prog()" that does that 
Windows thang.

		Linus


diff --git a/diff.c b/diff.c
index 804c08c..32da5d7 100644
--- a/diff.c
+++ b/diff.c
@@ -178,11 +178,12 @@ static void emit_rewrite_diff(const char
 		copy_file('+', temp[1].name);
 }
 
-static void builtin_diff(const char *name_a,
+static const char *builtin_diff(const char *name_a,
 			 const char *name_b,
 			 struct diff_tempfile *temp,
 			 const char *xfrm_msg,
-			 int complete_rewrite)
+			 int complete_rewrite,
+			 const char **args)
 {
 	int i, next_at, cmd_size;
 	const char *const diff_cmd = "diff -L%s -L%s";
@@ -242,19 +243,24 @@ static void builtin_diff(const char *nam
 		}
 		if (xfrm_msg && xfrm_msg[0])
 			puts(xfrm_msg);
+		/*
+		 * we do not run diff between different kind
+		 * of objects.
+		 */
 		if (strncmp(temp[0].mode, temp[1].mode, 3))
-			/* we do not run diff between different kind
-			 * of objects.
-			 */
-			exit(0);
+			return NULL;
 		if (complete_rewrite) {
-			fflush(NULL);
 			emit_rewrite_diff(name_a, name_b, temp);
-			exit(0);
+			return NULL;
 		}
 	}
-	fflush(NULL);
-	execlp("/bin/sh","sh", "-c", cmd, NULL);
+
+	/* This is disgusting */
+	*args++ = "sh";
+	*args++ = "-c";
+	*args++ = cmd;
+	*args = NULL;
+	return "/bin/sh";
 }
 
 struct diff_filespec *alloc_filespec(const char *path)
@@ -559,6 +565,40 @@ static void remove_tempfile_on_signal(in
 	raise(signo);
 }
 
+static int spawn_prog(const char *pgm, const char **arg)
+{
+	pid_t pid;
+	int status;
+
+	fflush(NULL);
+	pid = fork();
+	if (pid < 0)
+		die("unable to fork");
+	if (!pid) {
+		execvp(pgm, (char *const*) arg);
+		exit(255);
+	}
+
+	while (waitpid(pid, &status, 0) < 0) {
+		if (errno == EINTR)
+			continue;
+		return -1;
+	}
+
+	/* 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.  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.
+	 */
+	if (WIFEXITED(status) && !WEXITSTATUS(status))
+		return 0;
+	return -1;
+}
+
 /* An external diff command takes:
  *
  * diff-cmd name infile1 infile1-sha1 infile1-mode \
@@ -573,9 +613,9 @@ static void run_external_diff(const char
 			      const char *xfrm_msg,
 			      int complete_rewrite)
 {
+	const char *spawn_arg[10];
 	struct diff_tempfile *temp = diff_temp;
-	pid_t pid;
-	int status;
+	int retval;
 	static int atexit_asked = 0;
 	const char *othername;
 
@@ -592,59 +632,41 @@ static void run_external_diff(const char
 		signal(SIGINT, remove_tempfile_on_signal);
 	}
 
-	fflush(NULL);
-	pid = fork();
-	if (pid < 0)
-		die("unable to fork");
-	if (!pid) {
-		if (pgm) {
-			if (one && two) {
-				const char *exec_arg[10];
-				const char **arg = &exec_arg[0];
-				*arg++ = pgm;
-				*arg++ = name;
-				*arg++ = temp[0].name;
-				*arg++ = temp[0].hex;
-				*arg++ = temp[0].mode;
-				*arg++ = temp[1].name;
-				*arg++ = temp[1].hex;
-				*arg++ = temp[1].mode;
-				if (other) {
-					*arg++ = other;
-					*arg++ = xfrm_msg;
-				}
-				*arg = NULL;
-				execvp(pgm, (char *const*) exec_arg);
+	if (pgm) {
+		const char **arg = &spawn_arg[0];
+		if (one && two) {
+			*arg++ = pgm;
+			*arg++ = name;
+			*arg++ = temp[0].name;
+			*arg++ = temp[0].hex;
+			*arg++ = temp[0].mode;
+			*arg++ = temp[1].name;
+			*arg++ = temp[1].hex;
+			*arg++ = temp[1].mode;
+			if (other) {
+				*arg++ = other;
+				*arg++ = xfrm_msg;
 			}
-			else
-				execlp(pgm, pgm, name, NULL);
+		} else {
+			*arg++ = pgm;
+			*arg++ = name;
 		}
-		/*
-		 * otherwise we use the built-in one.
-		 */
-		if (one && two)
-			builtin_diff(name, othername, temp, xfrm_msg,
-				     complete_rewrite);
-		else
+		*arg = NULL;
+	} else {
+		if (one && two) {
+			pgm = builtin_diff(name, othername, temp, xfrm_msg, complete_rewrite, spawn_arg);
+		} else
 			printf("* Unmerged path %s\n", name);
-		exit(0);
 	}
-	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.  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();
+
+	retval = 0;
+	if (pgm)	
+		retval = spawn_prog(pgm, spawn_arg);
+	remove_tempfile();
+	if (retval) {
 		fprintf(stderr, "external diff died, stopping at %s.\n", name);
 		exit(1);
 	}
-	remove_tempfile();
 }
 
 static void diff_fill_sha1_info(struct diff_filespec *one)

Re: Make git diff-generation use a simpler spawn-like interface

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

> This actually ends up simplifying the code, and should make it much
> easier to make it efficient under broken operating systems (read: Windows).
>
> Signed-off-by: Linus Torvalds <torvalds@osdl.org>
> ---
>
> Now, somebody else might disagree with my contention that this also makes 
> things simpler, but I like how we separate out the "run an external 
> program" phase from the actual setup and header printing phase.

I agree this is going in the right direction.

A note that is offtopic to this particular patch but is related
to Andrew's point.  I had a wrong version of pre-applypatch hook
in the sample shipped as "templates", and after I rebuilt my
private development repository I've been running with the
version without a fix I personally had.  I've pushed out the fix
in the master branch.

I'll look at your patch to git-apply next.