Merge branch 'jk/external-diff-use-argv-array'

Code clean-up (and a bugfix which has been merged for 2.0).

* jk/external-diff-use-argv-array:
  run_external_diff: refactor cmdline setup logic
  run_external_diff: hoist common bits out of conditional
  run_external_diff: drop fflush(NULL)
  run_external_diff: clean up error handling
  run_external_diff: use an argv_array for the environment

diff.c

@@ -2880,6 +2880,16 @@ static struct diff_tempfile *prepare_temp_file(const char *name,
 	return temp;
 }
 
+static void add_external_diff_name(struct argv_array *argv,
+				   const char *name,
+				   struct diff_filespec *df)
+{
+	struct diff_tempfile *temp = prepare_temp_file(name, df);
+	argv_array_push(argv, temp->name);
+	argv_array_push(argv, temp->hex);
+	argv_array_push(argv, temp->mode);
+}
+
 /* An external diff command takes:
  *
  * diff-cmd name infile1 infile1-sha1 infile1-mode \
@@ -2896,48 +2906,32 @@ static void run_external_diff(const char *pgm,
 			      struct diff_options *o)
 {
 	struct argv_array argv = ARGV_ARRAY_INIT;
-	int retval;
+	struct argv_array env = ARGV_ARRAY_INIT;
 	struct diff_queue_struct *q = &diff_queued_diff;
-	const char *env[3] = { NULL };
-	char env_counter[50];
-	char env_total[50];
+
+	argv_array_push(&argv, pgm);
+	argv_array_push(&argv, name);
 
 	if (one && two) {
-		struct diff_tempfile *temp_one, *temp_two;
-		const char *othername = (other ? other : name);
-		temp_one = prepare_temp_file(name, one);
-		temp_two = prepare_temp_file(othername, two);
-		argv_array_push(&argv, pgm);
-		argv_array_push(&argv, name);
-		argv_array_push(&argv, temp_one->name);
-		argv_array_push(&argv, temp_one->hex);
-		argv_array_push(&argv, temp_one->mode);
-		argv_array_push(&argv, temp_two->name);
-		argv_array_push(&argv, temp_two->hex);
-		argv_array_push(&argv, temp_two->mode);
-		if (other) {
+		add_external_diff_name(&argv, name, one);
+		if (!other)
+			add_external_diff_name(&argv, name, two);
+		else {
+			add_external_diff_name(&argv, other, two);
 			argv_array_push(&argv, other);
 			argv_array_push(&argv, xfrm_msg);
 		}
-	} else {
-		argv_array_push(&argv, pgm);
-		argv_array_push(&argv, name);
 	}
-	fflush(NULL);
 
-	env[0] = env_counter;
-	snprintf(env_counter, sizeof(env_counter), "GIT_DIFF_PATH_COUNTER=%d",
-		 ++o->diff_path_counter);
-	env[1] = env_total;
-	snprintf(env_total, sizeof(env_total), "GIT_DIFF_PATH_TOTAL=%d", q->nr);
+	argv_array_pushf(&env, "GIT_DIFF_PATH_COUNTER=%d", ++o->diff_path_counter);
+	argv_array_pushf(&env, "GIT_DIFF_PATH_TOTAL=%d", q->nr);
+
+	if (run_command_v_opt_cd_env(argv.argv, RUN_USING_SHELL, NULL, env.argv))
+		die(_("external diff died, stopping at %s"), name);
 
-	retval = run_command_v_opt_cd_env(argv.argv, RUN_USING_SHELL, NULL, env);
 	remove_tempfile();
 	argv_array_clear(&argv);
-	if (retval) {
-		fprintf(stderr, "external diff died, stopping at %s.\n", name);
-		exit(1);
-	}
+	argv_array_clear(&env);
 }
 
 static int similarity_index(struct diff_filepair *p)

t/t7800-difftool.sh

@@ -58,7 +58,7 @@ test_expect_success PERL 'custom tool commands override built-ins' '
 
 test_expect_success PERL 'difftool ignores bad --tool values' '
 	: >expect &&
-	test_expect_code 1 \
+	test_must_fail \
 		git difftool --no-prompt --tool=bad-tool branch >actual &&
 	test_cmp expect actual
 '