Re: filtering out mode-change-only changes
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:53:11
Subsystem:
kernel build + files below scripts/ (unless maintained elsewhere), the rest · Maintainers:
Nathan Chancellor, Nicolas Schier, Linus Torvalds
Possibly related (same subject, not in this thread)
- 2016-06-15 · Re: filtering out mode-change-only changes · Pete Harlan <hidden>
- 2016-06-15 · Re: filtering out mode-change-only changes · Neal Kreitzinger <hidden>
- 2016-06-15 · Re: filtering out mode-change-only changes · Junio C Hamano <hidden>
- 2016-06-15 · filtering out mode-change-only changes · Neal Kreitzinger <hidden>
- 2016-06-15 · Re: filtering out mode-change-only changes · Junio C Hamano <hidden>
Junio C Hamano [off-list ref] 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.
A patch to do so may look like this (untested, of course). A few things to note on the changes: - 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)