Thread (3 messages) flat view 3 messages, 2 authors, 2016-06-15

Re: [RFH] Mention of 1.7.0 transition plans in Release Notes to 1.6.6

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:47:47
Subsystem: the rest · Maintainer: Linus Torvalds

Junio C Hamano [off-list ref] writes:
 - I do not think of a sane way to cover "diff -b/-w" changes, as this is
   a "bugfix --- but there may be some scripts that have been relying on
   the bug", and a configuration option that retains the buggy behaviour
   does not make much sense.  But I may be mistaken and somebody can come
   up with an easy patch to allow both behaviour, in which case we should
   add similar anti-procrastination measure to this change.
The patch to enable/disable this feature would look like the attached,
which doesn't look too bad.  This apply to a merge between two topics:

    jc/1.7.0-diff-whitespace-only-status (97bf2a0)
    gb/1.7.0-diff-whitespace-only-output (3e97c7c)

An anti-procrastination measure must be done in at least two, but probably
in three steps on top of this patch:

 #1. Flip the default in this patch back to 1, as the traditional
     behaviour is to treat ignore-whitespace options as purely affecting
     output of patch text, not affecting status nor the header;

 #2. Add extra logic to detect if a particular invocation may trigger a
     different behaviour once the default is changed to 0, and issue an
     warning if there is no configuration explicitly set;

 #3. Flip the default to 0, perhaps still keeping the warning when there
     is no configuration.

Make a major release after the second step is done, and wait for at least
one but preferrably two releases, and then ship the result of the third
step in the "flag day" release.  In a much later version we would remove
the "no configuration warning".

Do we need an anti-procrastination measure for this?  If so, I think we
probably need to postpone the "diff" semantics changes after 1.7.0, as I
am not very confident that we can solidly finish the second step before
the 1.6.6 final.

Also, I am not very keen on having this configuration, as its only purpose
is to ask for a broken semantics from "diff", even though it has been the
traditional behaviour for the past four years.

Comments?

---
 diff.c |   15 +++++++++++----
 1 files changed, 11 insertions(+), 4 deletions(-)
diff --git a/diff.c b/diff.c
index da90e6e..6e7c47c 100644
--- a/diff.c
+++ b/diff.c
@@ -29,6 +29,7 @@ static const char *diff_word_regex_cfg;
 static const char *external_diff_cmd_cfg;
 int diff_auto_refresh_index = 1;
 static int diff_mnemonic_prefix;
+static int diff_b_w_output_only;
 
 static char diff_colors[][COLOR_MAXLEN] = {
 	GIT_COLOR_RESET,
@@ -133,6 +134,11 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
+	if (!strcmp(var, "diff.bwoutputonly")) {
+		diff_b_w_output_only = git_config_bool(var, value);
+		return 0;
+	}
+
 	return git_color_default_config(var, value, cb);
 }
 
@@ -1668,7 +1674,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)) {
+		if (!DIFF_XDL_TST(o, WHITESPACE_FLAGS) || diff_b_w_output_only) {
 			fprintf(o->file, "%s", header.buf);
 			strbuf_reset(&header);
 		}
@@ -2537,9 +2543,10 @@ int diff_setup_done(struct diff_options *options)
 	 * inside contents.
 	 */
 
-	if (DIFF_XDL_TST(options, IGNORE_WHITESPACE) ||
-	    DIFF_XDL_TST(options, IGNORE_WHITESPACE_CHANGE) ||
-	    DIFF_XDL_TST(options, IGNORE_WHITESPACE_AT_EOL))
+	if (!diff_b_w_output_only &&
+	    (DIFF_XDL_TST(options, IGNORE_WHITESPACE) ||
+	     DIFF_XDL_TST(options, IGNORE_WHITESPACE_CHANGE) ||
+	     DIFF_XDL_TST(options, IGNORE_WHITESPACE_AT_EOL)))
 		DIFF_OPT_SET(options, DIFF_FROM_CONTENTS);
 	else
 		DIFF_OPT_CLR(options, DIFF_FROM_CONTENTS);
-- 
1.6.6.rc0.61.g41d5b.dirty
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help