Re: filtering out mode-change-only changes

From

Junio C Hamano <gitster@pobox.com>

Date

Feb 29, 2012, 22:17 UTC

Message-ID

<7v7gz5s2rg.fsf@alter.siamese.dyndns.org>

In-Reply-To

<7vipiqwb2g.fsf@alter.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> writes:

> Having said that, if we _were_ to do this built-in, an obvious logical
> place to do so is to define a new DIFF_OPT_IGNORE_EXECUTABLE_BIT, teach
> "--ignore-executable-bit" command line option to diff_opt_parse(), and
> then teach diff_resolve_rename_copy() to consider this bit when the code
> originally set DIFF_STATUS_MODIFIED.  Instead, the updated code that is
> working under --ignore-executable-bit option would drop such a filepair
> from diff_queued_diff.

 - The "header" strbuf holds the lines starting from "diff --git" and the
   meta-information lines such as mode change, similarity, etc.  It is
   passed to the the interface code fn_out_consume() via the xdiff
   machinery when an actual diff is found and emitted before the first
   hunk of the diff.

 - The must_show_header toggle is set at the strategic places when
   information is added to the "header" strbuf that makes the output for
   this filepair a must, even if it turns out that xdiff machinery does
   not find any content changes.  Before this patch, we flipped this
   toggle when we noticed a mode change, so that the header is shown even
   if there is no content change. The patch has to make make it
   conditional, which is what the first hunk is about.

 - The second hunk is not related but I think it is a worthy bit-rot fix.
   The original code before "must_show_header" was introduced showed the
   header upfront unless we are ignoring any whitespace changes, the
   reasoning behind it being that two different blobs may produce no patch
   under --ignore-space-change and in such a case we do not want to show
   anything for the filepair.  When 296c6bb (diff: fix "git show -C -C"
   output when renaming a binary file, 2010-05-26) introduced
   "must_show_header", it retained that logic.

   But I think it was a mistake.  If xdiff machinery decides there is no
   patch to show, taking the --ignore-space-change into account, our
   fn_out_consume() won't be called so we won't show the header
   unnecessarily.  And when we do see a line of patch, fn_out_consume()
   will show the header.

 diff.c |   31 +++++++++++++++++++++++++++++--
 diff.h |    1 +
 2 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
old mode 100644
new mode 100755
diff --git a/diff.c b/diff.c
index a1c06b5..acf7232 100644
--- a/diff.c
+++ b/diff.c
@@ -2152,7 +2152,8 @@ static void builtin_diff(const char *name_a,
 		if (one->mode != two->mode) {
 			strbuf_addf(&header, "%s%sold mode %06o%s\n", line_prefix, set, one->mode, reset);
 			strbuf_addf(&header, "%s%snew mode %06o%s\n", line_prefix, set, two->mode, reset);
-			must_show_header = 1;
+			if (!DIFF_OPT_TST(o, IGNORE_MODE_CHANGE))
+				must_show_header = 1;
 		}
 		if (xfrm_msg)
 			strbuf_addstr(&header, xfrm_msg);
@@ -2207,7 +2208,7 @@ static void builtin_diff(const char *name_a,
 		struct emit_callback ecbdata;
 		const struct userdiff_funcname *pe;
 
-		if (!DIFF_XDL_TST(o, WHITESPACE_FLAGS) || must_show_header) {
+		if (must_show_header) {
 			fprintf(o->file, "%s", header.buf);
 			strbuf_reset(&header);
 		}
@@ -3446,6 +3447,8 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 	}
 	else if (!strcmp(arg, "--no-renames"))
 		options->detect_rename = 0;
+	else if (!strcmp(arg, "--ignore-mode-change"))
+		DIFF_OPT_SET(options, IGNORE_MODE_CHANGE);
 	else if (!strcmp(arg, "--relative"))
 		DIFF_OPT_SET(options, RELATIVE_NAME);
 	else if (!prefixcmp(arg, "--relative=")) {
@@ -4509,10 +4512,34 @@ void diffcore_fix_diff_index(struct diff_options *options)
 	qsort(q->queue, q->nr, sizeof(q->queue[0]), diffnamecmp);
 }
 
+static void diffcore_ignore_mode_change(struct diff_options *diffopt)
+{
+	int i;
+	struct diff_queue_struct *q = &diff_queued_diff;
+	struct diff_queue_struct outq;
+	DIFF_QUEUE_CLEAR(&outq);
+
+	for (i = 0; i < q->nr; i++) {
+		struct diff_filepair *p = q->queue[i];
+
+		if (DIFF_FILE_VALID(p->one) &&
+		    DIFF_FILE_VALID(p->two) &&
+		    (p->one->sha1_valid && p->two->sha1_valid) &&
+		    !hashcmp(p->one->sha1, p->two->sha1))
+			diff_free_filepair(p); /* skip this */
+		else
+			diff_q(&outq, p);
+	}
+	free(q->queue);
+	*q = outq;
+}
+
 void diffcore_std(struct diff_options *options)
 {
 	if (options->skip_stat_unmatch)
 		diffcore_skip_stat_unmatch(options);
+	if (DIFF_OPT_TST(options, IGNORE_MODE_CHANGE))
+		diffcore_ignore_mode_change(options);
 	if (!options->found_follow) {
 		/* See try_to_follow_renames() in tree-diff.c */
 		if (options->break_opt != -1)
diff --git a/diff.h b/diff.h
index 7af5f1e..fabcc96 100644
--- a/diff.h
+++ b/diff.h
@@ -82,6 +82,7 @@ typedef struct strbuf *(*diff_prefix_fn_t)(struct diff_options *opt, void *data)
 #define DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG (1 << 27)
 #define DIFF_OPT_DIRSTAT_BY_LINE     (1 << 28)
 #define DIFF_OPT_FUNCCONTEXT         (1 << 29)
+#define DIFF_OPT_IGNORE_MODE_CHANGE  (1 << 30)
 
 #define DIFF_OPT_TST(opts, flag)    ((opts)->flags & DIFF_OPT_##flag)
 #define DIFF_OPT_SET(opts, flag)    ((opts)->flags |= DIFF_OPT_##flag)