Merge branch 'rs/diff-exit-code-with-external-diff'

"git diff --exit-code --ext-diff" learned to take the exit status
of the external diff driver into account when deciding the exit
status of the overall "git diff" invocation when configured to do
so.

* rs/diff-exit-code-with-external-diff:
  diff: let external diffs report that changes are uninteresting
  userdiff: add and use struct external_diff
  t4020: test exit code with external diffs

Documentation/config/diff.txt

@@ -79,6 +79,15 @@ diff.external::
 	you want to use an external diff program only on a subset of
 	your files, you might want to use linkgit:gitattributes[5] instead.
 
+diff.trustExitCode::
+	If this boolean value is set to true then the
+	`diff.external` command is expected to return exit code
+	0 if it considers the input files to be equal or 1 if it
+	considers them to be different, like `diff(1)`.
+	If it is set to false, which is the default, then the command
+	is expected to return exit code 0 regardless of equality.
+	Any other exit code causes Git to report a fatal error.
+
 diff.ignoreSubmodules::
 	Sets the default value of --ignore-submodules. Note that this
 	affects only 'git diff' Porcelain, and not lower level 'diff'
@@ -164,6 +173,15 @@ diff.<driver>.command::
 	The custom diff driver command.  See linkgit:gitattributes[5]
 	for details.
 
+diff.<driver>.trustExitCode::
+	If this boolean value is set to true then the
+	`diff.<driver>.command` command is expected to return exit code
+	0 if it considers the input files to be equal or 1 if it
+	considers them to be different, like `diff(1)`.
+	If it is set to false, which is the default, then the command
+	is expected to return exit code 0 regardless of equality.
+	Any other exit code causes Git to report a fatal error.
+
 diff.<driver>.xfuncname::
 	The regular expression that the diff driver should use to
 	recognize the hunk header.  A built-in pattern may also be used.

Documentation/diff-options.txt

@@ -820,6 +820,11 @@ ifndef::git-log[]
 
 --quiet::
 	Disable all output of the program. Implies `--exit-code`.
+	Disables execution of external diff helpers whose exit code
+	is not trusted, i.e. their respective configuration option
+	`diff.trustExitCode` or `diff.<driver>.trustExitCode` or
+	environment variable `GIT_EXTERNAL_DIFF_TRUST_EXIT_CODE` is
+	false.
 endif::git-log[]
 endif::git-format-patch[]
 

Documentation/git.txt

@@ -644,6 +644,16 @@ parameter, <path>.
 For each path `GIT_EXTERNAL_DIFF` is called, two environment variables,
 `GIT_DIFF_PATH_COUNTER` and `GIT_DIFF_PATH_TOTAL` are set.
 
+`GIT_EXTERNAL_DIFF_TRUST_EXIT_CODE`::
+	If this Boolean environment variable is set to true then the
+	`GIT_EXTERNAL_DIFF` command is expected to return exit code
+	0 if it considers the input files to be equal or 1 if it
+	considers them to be different, like `diff(1)`.
+	If it is set to false, which is the default, then the command
+	is expected to return exit code 0 regardless of equality.
+	Any other exit code causes Git to report a fatal error.
+
+
 `GIT_DIFF_PATH_COUNTER`::
 	A 1-based counter incremented by one for every path.
 

Documentation/gitattributes.txt

@@ -776,6 +776,11 @@ with the above configuration, i.e. `j-c-diff`, with 7
 parameters, just like `GIT_EXTERNAL_DIFF` program is called.
 See linkgit:git[1] for details.
 
+If the program is able to ignore certain changes (similar to
+`git diff --ignore-space-change`), then also set the option
+`trustExitCode` to true.  It is then expected to return exit code 1 if
+it finds significant changes and 0 if it doesn't.
+
 Setting the internal diff algorithm
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 

diff.c

@@ -57,7 +57,7 @@ static int diff_color_moved_ws_default;
 static int diff_context_default = 3;
 static int diff_interhunk_context_default;
 static char *diff_word_regex_cfg;
-static char *external_diff_cmd_cfg;
+static struct external_diff external_diff_cfg;
 static char *diff_order_file_cfg;
 int diff_auto_refresh_index = 1;
 static int diff_mnemonic_prefix;
@@ -431,7 +431,11 @@ int git_diff_ui_config(const char *var, const char *value,
 		return 0;
 	}
 	if (!strcmp(var, "diff.external"))
-		return git_config_string(&external_diff_cmd_cfg, var, value);
+		return git_config_string(&external_diff_cfg.cmd, var, value);
+	if (!strcmp(var, "diff.trustexitcode")) {
+		external_diff_cfg.trust_exit_code = git_config_bool(var, value);
+		return 0;
+	}
 	if (!strcmp(var, "diff.wordregex"))
 		return git_config_string(&diff_word_regex_cfg, var, value);
 	if (!strcmp(var, "diff.orderfile"))
@@ -548,18 +552,22 @@ static char *quote_two(const char *one, const char *two)
 	return strbuf_detach(&res, NULL);
 }
 
-static const char *external_diff(void)
+static const struct external_diff *external_diff(void)
 {
-	static const char *external_diff_cmd = NULL;
+	static struct external_diff external_diff_env, *external_diff_ptr;
 	static int done_preparing = 0;
 
 	if (done_preparing)
-		return external_diff_cmd;
+		return external_diff_ptr;
-	external_diff_cmd = xstrdup_or_null(getenv("GIT_EXTERNAL_DIFF"));
+	external_diff_env.cmd = xstrdup_or_null(getenv("GIT_EXTERNAL_DIFF"));
-	if (!external_diff_cmd)
+	if (git_env_bool("GIT_EXTERNAL_DIFF_TRUST_EXIT_CODE", 0))
-		external_diff_cmd = external_diff_cmd_cfg;
+		external_diff_env.trust_exit_code = 1;
+	if (external_diff_env.cmd)
+		external_diff_ptr = &external_diff_env;
+	else if (external_diff_cfg.cmd)
+		external_diff_ptr = &external_diff_cfg;
 	done_preparing = 1;
-	return external_diff_cmd;
+	return external_diff_ptr;
 }
 
 /*
@@ -4375,7 +4383,7 @@ static void add_external_diff_name(struct repository *r,
  *               infile2 infile2-sha1 infile2-mode [ rename-to ]
  *
  */
-static void run_external_diff(const char *pgm,
+static void run_external_diff(const struct external_diff *pgm,
 			      const char *name,
 			      const char *other,
 			      struct diff_filespec *one,
@@ -4385,8 +4393,21 @@ static void run_external_diff(const char *pgm,
 {
 	struct child_process cmd = CHILD_PROCESS_INIT;
 	struct diff_queue_struct *q = &diff_queued_diff;
+	int quiet = !(o->output_format & DIFF_FORMAT_PATCH);
+	int rc;
 
-	strvec_push(&cmd.args, pgm);
+	/*
+	 * Trivial equality is handled by diff_unmodified_pair() before
+	 * we get here.  If we don't need to show the diff and the
+	 * external diff program lacks the ability to tell us whether
+	 * it's empty then we consider it non-empty without even asking.
+	 */
+	if (!pgm->trust_exit_code && quiet) {
+		o->found_changes = 1;
+		return;
+	}
+
+	strvec_push(&cmd.args, pgm->cmd);
 	strvec_push(&cmd.args, name);
 
 	if (one && two) {
@@ -4406,7 +4427,15 @@ static void run_external_diff(const char *pgm,
 	diff_free_filespec_data(one);
 	diff_free_filespec_data(two);
 	cmd.use_shell = 1;
-	if (run_command(&cmd))
+	cmd.no_stdout = quiet;
+	rc = run_command(&cmd);
+	if (!pgm->trust_exit_code && rc == 0)
+		o->found_changes = 1;
+	else if (pgm->trust_exit_code && rc == 0)
+		; /* nothing */
+	else if (pgm->trust_exit_code && rc == 1)
+		o->found_changes = 1;
+	else
 		die(_("external diff died, stopping at %s"), name);
 
 	remove_tempfile();
@@ -4512,7 +4541,7 @@ static void fill_metainfo(struct strbuf *msg,
 	}
 }
 
-static void run_diff_cmd(const char *pgm,
+static void run_diff_cmd(const struct external_diff *pgm,
 			 const char *name,
 			 const char *other,
 			 const char *attr_path,
@@ -4530,8 +4559,8 @@ static void run_diff_cmd(const char *pgm,
 	if (o->flags.allow_external || !o->ignore_driver_algorithm)
 		drv = userdiff_find_by_path(o->repo->index, attr_path);
 
-	if (o->flags.allow_external && drv && drv->external)
+	if (o->flags.allow_external && drv && drv->external.cmd)
-		pgm = drv->external;
+		pgm = &drv->external;
 
 	if (msg) {
 		/*
@@ -4597,7 +4626,7 @@ static void strip_prefix(int prefix_length, const char **namep, const char **oth
 
 static void run_diff(struct diff_filepair *p, struct diff_options *o)
 {
-	const char *pgm = external_diff();
+	const struct external_diff *pgm = external_diff();
 	struct strbuf msg;
 	struct diff_filespec *one = p->one;
 	struct diff_filespec *two = p->two;
@@ -4924,6 +4953,13 @@ void diff_setup_done(struct diff_options *options)
 		options->flags.exit_with_status = 1;
 	}
 
+	/*
+	 * External diffs could declare non-identical contents equal
+	 * (think diff --ignore-space-change).
+	 */
+	if (options->flags.allow_external && options->flags.exit_with_status)
+		options->flags.diff_from_contents = 1;
+
 	options->diff_path_counter = 0;
 
 	if (options->flags.follow_renames)

t/t4020-diff-external.sh

@@ -172,6 +172,72 @@ test_expect_success 'no diff with -diff' '
 	grep Binary out
 '
 
+check_external_diff () {
+	expect_code=$1
+	expect_out=$2
+	expect_err=$3
+	command_code=$4
+	trust_exit_code=$5
+	shift 5
+	options="$@"
+
+	command="echo output; exit $command_code;"
+	desc="external diff '$command' with trustExitCode=$trust_exit_code"
+	with_options="${options:+ with }$options"
+
+	test_expect_success "$desc via attribute$with_options" "
+		test_config diff.foo.command \"$command\" &&
+		test_config diff.foo.trustExitCode $trust_exit_code &&
+		echo \"file diff=foo\" >.gitattributes &&
+		test_expect_code $expect_code git diff $options >out 2>err &&
+		test_cmp $expect_out out &&
+		test_cmp $expect_err err
+	"
+
+	test_expect_success "$desc via diff.external$with_options" "
+		test_config diff.external \"$command\" &&
+		test_config diff.trustExitCode $trust_exit_code &&
+		>.gitattributes &&
+		test_expect_code $expect_code git diff $options >out 2>err &&
+		test_cmp $expect_out out &&
+		test_cmp $expect_err err
+	"
+
+	test_expect_success "$desc via GIT_EXTERNAL_DIFF$with_options" "
+		>.gitattributes &&
+		test_expect_code $expect_code env \
+			GIT_EXTERNAL_DIFF=\"$command\" \
+			GIT_EXTERNAL_DIFF_TRUST_EXIT_CODE=$trust_exit_code \
+			git diff $options >out 2>err &&
+		test_cmp $expect_out out &&
+		test_cmp $expect_err err
+	"
+}
+
+test_expect_success 'setup output files' '
+	: >empty &&
+	echo output >output &&
+	echo "fatal: external diff died, stopping at file" >error
+'
+
+check_external_diff   0 output empty 0 off
+check_external_diff 128 output error 1 off
+check_external_diff   0 output empty 0 on
+check_external_diff   0 output empty 1 on
+check_external_diff 128 output error 2 on
+
+check_external_diff   1 output empty 0 off --exit-code
+check_external_diff 128 output error 1 off --exit-code
+check_external_diff   0 output empty 0 on  --exit-code
+check_external_diff   1 output empty 1 on  --exit-code
+check_external_diff 128 output error 2 on  --exit-code
+
+check_external_diff   1 empty  empty 0 off --quiet
+check_external_diff   1 empty  empty 1 off --quiet # we don't even call the program
+check_external_diff   0 empty  empty 0 on  --quiet
+check_external_diff   1 empty  empty 1 on  --quiet
+check_external_diff 128 empty  error 2 on  --quiet
+
 echo NULZbetweenZwords | perl -pe 'y/Z/\000/' > file
 
 test_expect_success 'force diff with "diff"' '

userdiff.c

@@ -333,7 +333,7 @@ PATTERNS("scheme",
 	 "|([^][)(}{[ \t])+"),
 PATTERNS("tex", "^(\\\\((sub)*section|chapter|part)\\*{0,1}\\{.*)$",
 	 "\\\\[a-zA-Z@]+|\\\\.|([a-zA-Z0-9]|[^\x01-\x7f])+"),
-{ "default", NULL, NULL, -1, { NULL, 0 } },
+{ .name = "default", .binary = -1 },
 };
 #undef PATTERNS
 #undef IPATTERN
@@ -445,7 +445,11 @@ int userdiff_config(const char *k, const char *v)
 	if (!strcmp(type, "binary"))
 		return parse_tristate(&drv->binary, k, v);
 	if (!strcmp(type, "command"))
-		return git_config_string((char **) &drv->external, k, v);
+		return git_config_string((char **) &drv->external.cmd, k, v);
+	if (!strcmp(type, "trustexitcode")) {
+		drv->external.trust_exit_code = git_config_bool(k, v);
+		return 0;
+	}
 	if (!strcmp(type, "textconv"))
 		return git_config_string((char **) &drv->textconv, k, v);
 	if (!strcmp(type, "cachetextconv"))

userdiff.h

@@ -11,9 +11,14 @@ struct userdiff_funcname {
 	int cflags;
 };
 
+struct external_diff {
+	char *cmd;
+	unsigned trust_exit_code:1;
+};
+
 struct userdiff_driver {
 	const char *name;
-	const char *external;
+	struct external_diff external;
 	const char *algorithm;
 	int binary;
 	struct userdiff_funcname funcname;