threads / patch / 49030

patch, 7 partsimprove range-diffs coloring and [RFC] move detection

Subject: [PATCH 0/7] improve range-diffs coloring and [RFC] move detection

## tl;dr

13 messages between Aug 4, 2018 and Aug 6, 2018. Diffs are folded; open one to read it.

replies: 12people: 2as markdown or json

Stefan Beller· Aug 4, 2018, 01:53 UTC · lore
This builds on top of sb/range-diff-colors, which builds on js/range-diff.
As Thomas seemed happy with the range-diff and said
"and fwiw I didn't think there's anything major that needs to be addressed."
I found it reasonable to build more on top of those series.
[1] https://public-inbox.org/git/20180729215053.GE9955@hank.intra.tgummerer.com/
It can also be found via
  git fetch https://github.com/stefanbeller/git moved-in-patches
This series provides for 2 goals, which share a common base refactoring:

The refactoring part is to allow the diff machinery to prefix old/new/context lines with prefixes as chosen by the user.

The first feature is to slightly adapt coloring of range-diff to have the file markers

  --- a/diff.c
  +++ b/diff.c
  
be shown as context color. This is done by using custom prefixes for
old/new/context lines in the inner diffs and indenting all other lines
instead by a white space indicating it to be context.

The second feature is more RFC-ish in nature and just exposes the mechanism to the user in a meaning-ful way. All of the diff family (including format-patch) as well as apply/am can use */~ instead of +/- for moved lines of code.

Thanks, Stefan

Stefan Beller (7):
  diff.c: emit_line_0 to take string instead of first sign
  diff.c: add --output-indicator-{new, old, context}
  range-diff: make use of different output indicators
  range-diff: indent special lines as context
  diff.c: rename color_moved to markup_moved
  diff.c: factor determine_line_color out of
    emit_diff_symbol_from_struct
  diff/am: enhance diff format to use */~ for moved lines
 apply.c               |  12 +++
 diff.c                | 180 ++++++++++++++++++++++++++----------------
 diff.h                |  10 ++-
 range-diff.c          |  17 +++-
 t/t3206-range-diff.sh |  12 +--
 5 files changed, 153 insertions(+), 78 deletions(-)
-- 
2.18.0.597.ga71716f1ad-goog
Stefan Beller· Aug 4, 2018, 01:53 UTC · re: Stefan Beller · lore

[PATCH 1/7] diff.c: emit_line_0 to take string instead of first sign

By providing a string as the first part of the emission we can extend it later more easily.

While at it, document emit_line_0.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 diff.c | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)
Show changes to diff.c +17 −11
diff --git a/diff.c b/diff.c
index 2bd4d3d6839..57a8a38755e 100644
--- a/diff.c
+++ b/diff.c
@@ -575,9 +575,15 @@ static void check_blank_at_eof(mmfile_t *mf1, mmfile_t *mf2,
 	ecbdata->blank_at_eof_in_postimage = (at - l2) + 1;
 }
 
+/*
+ * Emits
+ * <set_sign> <first> <reset> <set> <second> <reset> LF
+ * if they are present. 'first' is a NULL terminated string,
+ * 'second' is a buffer of length 'len'.
+ */
 static void emit_line_0(struct diff_options *o,
 			const char *set_sign, const char *set, const char *reset,
-			int first, const char *line, int len)
+			const char *first, const char *second, int len)
 {
 	int has_trailing_newline, has_trailing_carriage_return;
 	int reverse = !!set && !!set_sign;
@@ -587,11 +593,11 @@ static void emit_line_0(struct diff_options *o,
 
 	fputs(diff_line_prefix(o), file);
 
-	has_trailing_newline = (len > 0 && line[len-1] == '\n');
+	has_trailing_newline = (len > 0 && second[len-1] == '\n');
 	if (has_trailing_newline)
 		len--;
 
-	has_trailing_carriage_return = (len > 0 && line[len-1] == '\r');
+	has_trailing_carriage_return = (len > 0 && second[len-1] == '\r');
 	if (has_trailing_carriage_return)
 		len--;
 
@@ -609,7 +615,7 @@ static void emit_line_0(struct diff_options *o,
 	}
 
 	if (first)
-		fputc(first, file);
+		fputs(first, file);
 
 	if (!len)
 		goto end_of_line;
@@ -620,7 +626,7 @@ static void emit_line_0(struct diff_options *o,
 		fputs(set, file);
 		needs_reset = 1;
 	}
-	fwrite(line, len, 1, file);
+	fwrite(second, len, 1, file);
 	needs_reset |= len > 0;
 
 end_of_line:
@@ -635,7 +641,7 @@ static void emit_line_0(struct diff_options *o,
 static void emit_line(struct diff_options *o, const char *set, const char *reset,
 		      const char *line, int len)
 {
-	emit_line_0(o, set, NULL, reset, 0, line, len);
+	emit_line_0(o, set, NULL, reset, NULL, line, len);
 }
 
 enum diff_symbol {
@@ -996,7 +1002,7 @@ static void dim_moved_lines(struct diff_options *o)
 static void emit_line_ws_markup(struct diff_options *o,
 				const char *set_sign, const char *set,
 				const char *reset,
-				char sign, const char *line, int len,
+				const char *sign, const char *line, int len,
 				unsigned ws_rule, int blank_at_eof)
 {
 	const char *ws = NULL;
@@ -1039,7 +1045,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,
 		context = diff_get_color_opt(o, DIFF_CONTEXT);
 		reset = diff_get_color_opt(o, DIFF_RESET);
 		putc('\n', o->file);
-		emit_line_0(o, context, NULL, reset, '\\',
+		emit_line_0(o, context, NULL, reset, "\\",
 			    nneof, strlen(nneof));
 		break;
 	case DIFF_SYMBOL_SUBMODULE_HEADER:
@@ -1077,7 +1083,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,
 			else if (c == '-')
 				set = diff_get_color_opt(o, DIFF_FILE_OLD);
 		}
-		emit_line_ws_markup(o, set_sign, set, reset, ' ', line, len,
+		emit_line_ws_markup(o, set_sign, set, reset, " ", line, len,
 				    flags & (DIFF_SYMBOL_CONTENT_WS_MASK), 0);
 		break;
 	case DIFF_SYMBOL_PLUS:
@@ -1120,7 +1126,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,
 				set = diff_get_color_opt(o, DIFF_CONTEXT_BOLD);
 			flags |= WS_IGNORE_FIRST_SPACE;
 		}
-		emit_line_ws_markup(o, set_sign, set, reset, '+', line, len,
+		emit_line_ws_markup(o, set_sign, set, reset, "+", line, len,
 				    flags & DIFF_SYMBOL_CONTENT_WS_MASK,
 				    flags & DIFF_SYMBOL_CONTENT_BLANK_LINE_EOF);
 		break;
@@ -1163,7 +1169,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,
 			else
 				set = diff_get_color_opt(o, DIFF_CONTEXT_DIM);
 		}
-		emit_line_ws_markup(o, set_sign, set, reset, '-', line, len,
+		emit_line_ws_markup(o, set_sign, set, reset, "-", line, len,
 				    flags & DIFF_SYMBOL_CONTENT_WS_MASK, 0);
 		break;
 	case DIFF_SYMBOL_WORDS_PORCELAIN:

-- 
2.18.0.597.ga71716f1ad-goog
Stefan Beller· Aug 4, 2018, 01:53 UTC · re: Stefan Beller · lore

[PATCH 2/7] diff.c: add --output-indicator-{new, old, context}

This will prove useful in range-diff in a later patch as we will be able to differentiate between adding a new file (that line is starting with +++ and then the file name) and regular new lines.

It could also be useful for experimentation in new patch formats, i.e. we could teach git to emit moved lines with lines other than +/-.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 diff.c | 21 +++++++++++++++++----
 diff.h |  5 +++++
 2 files changed, 22 insertions(+), 4 deletions(-)
Show changes to 2 files +22 −4

diff.c, diff.h

diff --git a/diff.c b/diff.c
index 57a8a38755e..2e711809700 100644
--- a/diff.c
+++ b/diff.c
@@ -1032,7 +1032,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,
 					 struct emitted_diff_symbol *eds)
 {
 	static const char *nneof = " No newline at end of file\n";
-	const char *context, *reset, *set, *set_sign, *meta, *fraginfo;
+	const char *context, *reset, *set, *set_sign, *meta, *fraginfo, *first;
 	struct strbuf sb = STRBUF_INIT;
 
 	enum diff_symbol s = eds->s;
@@ -1083,7 +1083,9 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,
 			else if (c == '-')
 				set = diff_get_color_opt(o, DIFF_FILE_OLD);
 		}
-		emit_line_ws_markup(o, set_sign, set, reset, " ", line, len,
+		first = o->output_indicators[OI_CONTEXT] ?
+			o->output_indicators[OI_CONTEXT] : " ";
+		emit_line_ws_markup(o, set_sign, set, reset, first, line, len,
 				    flags & (DIFF_SYMBOL_CONTENT_WS_MASK), 0);
 		break;
 	case DIFF_SYMBOL_PLUS:
@@ -1126,7 +1128,10 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,
 				set = diff_get_color_opt(o, DIFF_CONTEXT_BOLD);
 			flags |= WS_IGNORE_FIRST_SPACE;
 		}
-		emit_line_ws_markup(o, set_sign, set, reset, "+", line, len,
+
+		first = o->output_indicators[OI_NEW] ?
+			o->output_indicators[OI_NEW] : "+";
+		emit_line_ws_markup(o, set_sign, set, reset, first, line, len,
 				    flags & DIFF_SYMBOL_CONTENT_WS_MASK,
 				    flags & DIFF_SYMBOL_CONTENT_BLANK_LINE_EOF);
 		break;
@@ -1169,7 +1174,9 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,
 			else
 				set = diff_get_color_opt(o, DIFF_CONTEXT_DIM);
 		}
-		emit_line_ws_markup(o, set_sign, set, reset, "-", line, len,
+		first = o->output_indicators[OI_OLD] ?
+			o->output_indicators[OI_OLD] : "-";
+		emit_line_ws_markup(o, set_sign, set, reset, first, line, len,
 				    flags & DIFF_SYMBOL_CONTENT_WS_MASK, 0);
 		break;
 	case DIFF_SYMBOL_WORDS_PORCELAIN:
@@ -4670,6 +4677,12 @@ int diff_opt_parse(struct diff_options *options,
 		 options->output_format |= DIFF_FORMAT_DIFFSTAT;
 	} else if (!strcmp(arg, "--no-compact-summary"))
 		 options->flags.stat_with_summary = 0;
+	else if (skip_prefix(arg, "--output-indicator-new=", &arg))
+		options->output_indicators[OI_NEW] = arg;
+	else if (skip_prefix(arg, "--output-indicator-old=", &arg))
+		options->output_indicators[OI_OLD] = arg;
+	else if (skip_prefix(arg, "--output-indicator-context=", &arg))
+		options->output_indicators[OI_CONTEXT] = arg;
 
 	/* renames options */
 	else if (starts_with(arg, "-B") ||
diff --git a/diff.h b/diff.h
index a08a3b2a293..b8bbe7baeb8 100644
--- a/diff.h
+++ b/diff.h
@@ -194,6 +194,11 @@ struct diff_options {
 	FILE *file;
 	int close_file;
 
+#define OI_NEW 0
+#define OI_OLD 1
+#define OI_CONTEXT 2
+	const char *output_indicators[3];
+
 	struct pathspec pathspec;
 	pathchange_fn_t pathchange;
 	change_fn_t change;

-- 
2.18.0.597.ga71716f1ad-goog
Stefan Beller· Aug 4, 2018, 01:53 UTC · re: Stefan Beller · lore

[PATCH 3/7] range-diff: make use of different output indicators

This change itself only changes the internal communication and should have no visible effect to the user. We instruct the diff code that produces the inner diffs to use X, Y, Z instead of the usual markers for new, old and context lines

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 range-diff.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)
Show changes to range-diff.c +14 −1
diff --git a/range-diff.c b/range-diff.c
index 347b4a79f25..a4ff945427e 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -38,6 +38,9 @@ static int read_patches(const char *range, struct string_list *list)
 
 	argv_array_pushl(&cp.args, "log", "--no-color", "-p", "--no-merges",
 			"--reverse", "--date-order", "--decorate=no",
+			"--output-indicator-new=X",
+			"--output-indicator-old=Y",
+			"--output-indicator-context=Z",
 			"--no-abbrev-commit", range,
 			NULL);
 	cp.out = -1;
@@ -108,8 +111,18 @@ static int read_patches(const char *range, struct string_list *list)
 			 * we are not interested.
 			 */
 			continue;
-		else
+		else if (line.buf[0] == 'X') {
+			strbuf_addch(&buf, '+');
+			strbuf_add(&buf, line.buf + 1, line.len - 1);
+		} else if (line.buf[0] == 'Y') {
+			strbuf_addch(&buf, '-');
+			strbuf_add(&buf, line.buf + 1, line.len - 1);
+		} else if (line.buf[0] == 'Z') {
+			strbuf_addch(&buf, ' ');
+			strbuf_add(&buf, line.buf + 1, line.len - 1);
+		} else {
 			strbuf_addbuf(&buf, &line);
+		}
 
 		strbuf_addch(&buf, '\n');
 		util->diffsize++;

-- 
2.18.0.597.ga71716f1ad-goog
Stefan Beller· Aug 4, 2018, 01:53 UTC · re: Stefan Beller · lore

[PATCH 4/7] range-diff: indent special lines as context

The range-diff coloring is a bit fuzzy when it comes to special lines of a diff, such as indicating new and old files with +++ and ---, as it would pickup the first character and interpret it for its coloring, which seems annoying as in regular diffs, these lines are colored bold via DIFF_METAINFO.

By indenting these lines by a white space, they will be treated as context which is much more useful, an example [1] on the range diff series itself:

[...]
    + diff --git a/Documentation/git-range-diff.txt b/Documentation/git-range-diff.txt
    + new file mode 100644
    + --- /dev/null
    + +++ b/Documentation/git-range-diff.txt
    +@@
    ++git-range-diff(1)
[...]
    +
      diff --git a/Makefile b/Makefile
      --- a/Makefile
      +++ b/Makefile
[...]

The first lines that introduce the new file for the man page will have the '+' sign colored and the rest of the line will be bold.

The later lines that indicate a change to the Makefile will be treated as context both in the outer and inner diff, such that those lines stay regular color.

[1] ./git-range-diff pr-1/dscho/branch-diff-v3...pr-1/dscho/branch-diff-v4
    These tags are found at https://github.com/gitgitgadget/git
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 range-diff.c          |  2 ++
 t/t3206-range-diff.sh | 12 ++++++------
 2 files changed, 8 insertions(+), 6 deletions(-)
Show changes to 2 files +8 −6

range-diff.c, t/t3206-range-diff.sh

diff --git a/range-diff.c b/range-diff.c
index a4ff945427e..91d5f12180d 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -85,6 +85,7 @@ static int read_patches(const char *range, struct string_list *list)
 			strbuf_addch(&buf, '\n');
 			if (!util->diff_offset)
 				util->diff_offset = buf.len;
+			strbuf_addch(&buf, ' ');
 			strbuf_addbuf(&buf, &line);
 		} else if (in_header) {
 			if (starts_with(line.buf, "Author: ")) {
@@ -121,6 +122,7 @@ static int read_patches(const char *range, struct string_list *list)
 			strbuf_addch(&buf, ' ');
 			strbuf_add(&buf, line.buf + 1, line.len - 1);
 		} else {
+			strbuf_addch(&buf, ' ');
 			strbuf_addbuf(&buf, &line);
 		}
 
diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index e3b0764b433..0cd23cbff41 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -133,9 +133,9 @@ test_expect_success 'changed message' '
 	    Z
 	    +    Also a silly comment here!
 	    +
-	    Zdiff --git a/file b/file
-	    Z--- a/file
-	    Z+++ b/file
+	    Z diff --git a/file b/file
+	    Z --- a/file
+	    Z +++ b/file
 	3:  147e64e = 3:  b9cb956 s/11/B/
 	4:  a63e992 = 4:  8add5f1 s/12/B/
 	EOF
@@ -152,9 +152,9 @@ test_expect_success 'dual-coloring' '
 	:     <RESET>
 	:    <REVERSE><GREEN>+<RESET> <BOLD>   Also a silly comment here!<RESET>
 	:    <REVERSE><GREEN>+<RESET>
-	:     diff --git a/file b/file<RESET>
-	:    <RED> --- a/file<RESET>
-	:    <GREEN> +++ b/file<RESET>
+	:      diff --git a/file b/file<RESET>
+	:      --- a/file<RESET>
+	:      +++ b/file<RESET>
 	:<RED>3:  0559556 <RESET><YELLOW>!<RESET><GREEN> 3:  b9cb956<RESET><YELLOW> s/11/B/<RESET>
 	:    <REVERSE><CYAN>@@ -10,7 +10,7 @@<RESET>
 	:      9<RESET>

-- 
2.18.0.597.ga71716f1ad-goog
Stefan Beller· Aug 4, 2018, 01:53 UTC · re: Stefan Beller · lore

[PATCH 5/7] diff.c: rename color_moved to markup_moved

This just renames a variable to make the next patch easier to review.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 diff.c | 28 ++++++++++++++--------------
 diff.h |  2 +-
 2 files changed, 15 insertions(+), 15 deletions(-)
Show changes to 2 files +15 −15

diff.c, diff.h

diff --git a/diff.c b/diff.c
index 2e711809700..d3829c7d086 100644
--- a/diff.c
+++ b/diff.c
@@ -821,7 +821,7 @@ static int shrink_potential_moved_blocks(struct moved_entry **pmb,
 }
 
 /*
- * If o->color_moved is COLOR_MOVED_PLAIN, this function does nothing.
+ * If o->markup_moved is COLOR_MOVED_PLAIN, this function does nothing.
  *
  * Otherwise, if the last block has fewer alphanumeric characters than
  * COLOR_MOVED_MIN_ALNUM_COUNT, unset DIFF_SYMBOL_MOVED_LINE on all lines in
@@ -836,7 +836,7 @@ static int shrink_potential_moved_blocks(struct moved_entry **pmb,
 static void adjust_last_block(struct diff_options *o, int n, int block_length)
 {
 	int i, alnum_count = 0;
-	if (o->color_moved == COLOR_MOVED_PLAIN)
+	if (o->markup_moved == COLOR_MOVED_PLAIN)
 		return;
 	for (i = 1; i < block_length + 1; i++) {
 		const char *c = o->emitted_symbols->buf[n - i].line;
@@ -895,7 +895,7 @@ static void mark_color_as_moved(struct diff_options *o,
 
 		l->flags |= DIFF_SYMBOL_MOVED_LINE;
 
-		if (o->color_moved == COLOR_MOVED_PLAIN)
+		if (o->markup_moved == COLOR_MOVED_PLAIN)
 			continue;
 
 		/* Check any potential block runs, advance each or nullify */
@@ -4220,7 +4220,7 @@ void diff_setup(struct diff_options *options)
 		options->b_prefix = "b/";
 	}
 
-	options->color_moved = diff_color_moved_default;
+	options->markup_moved = diff_color_moved_default;
 }
 
 void diff_setup_done(struct diff_options *options)
@@ -4333,7 +4333,7 @@ void diff_setup_done(struct diff_options *options)
 		die(_("--follow requires exactly one pathspec"));
 
 	if (!options->use_color || external_diff())
-		options->color_moved = 0;
+		options->markup_moved = 0;
 }
 
 static int opt_arg(const char *arg, int arg_short, const char *arg_long, int *val)
@@ -4796,16 +4796,16 @@ int diff_opt_parse(struct diff_options *options,
 		options->use_color = 0;
 	else if (!strcmp(arg, "--color-moved")) {
 		if (diff_color_moved_default)
-			options->color_moved = diff_color_moved_default;
-		if (options->color_moved == COLOR_MOVED_NO)
-			options->color_moved = COLOR_MOVED_DEFAULT;
+			options->markup_moved = diff_color_moved_default;
+		if (options->markup_moved == COLOR_MOVED_NO)
+			options->markup_moved = COLOR_MOVED_DEFAULT;
 	} else if (!strcmp(arg, "--no-color-moved"))
-		options->color_moved = COLOR_MOVED_NO;
+		options->markup_moved = COLOR_MOVED_NO;
 	else if (skip_prefix(arg, "--color-moved=", &arg)) {
 		int cm = parse_color_moved(arg);
 		if (cm < 0)
 			die("bad --color-moved argument: %s", arg);
-		options->color_moved = cm;
+		options->markup_moved = cm;
 	} else if (skip_to_optional_arg_default(arg, "--color-words", &options->word_regex, NULL)) {
 		options->use_color = 1;
 		options->word_diff = DIFF_WORDS_COLOR;
@@ -5623,7 +5623,7 @@ static void diff_flush_patch_all_file_pairs(struct diff_options *o)
 	if (WSEH_NEW & WS_RULE_MASK)
 		BUG("WS rules bit mask overlaps with diff symbol flags");
 
-	if (o->color_moved)
+	if (o->markup_moved)
 		o->emitted_symbols = &esm;
 
 	for (i = 0; i < q->nr; i++) {
@@ -5633,7 +5633,7 @@ static void diff_flush_patch_all_file_pairs(struct diff_options *o)
 	}
 
 	if (o->emitted_symbols) {
-		if (o->color_moved) {
+		if (o->markup_moved) {
 			struct hashmap add_lines, del_lines;
 
 			hashmap_init(&del_lines,
@@ -5643,7 +5643,7 @@ static void diff_flush_patch_all_file_pairs(struct diff_options *o)
 
 			add_lines_to_move_detection(o, &add_lines, &del_lines);
 			mark_color_as_moved(o, &add_lines, &del_lines);
-			if (o->color_moved == COLOR_MOVED_ZEBRA_DIM)
+			if (o->markup_moved == COLOR_MOVED_ZEBRA_DIM)
 				dim_moved_lines(o);
 
 			hashmap_free(&add_lines, 0);
@@ -5731,7 +5731,7 @@ void diff_flush(struct diff_options *options)
 			fclose(options->file);
 		options->file = xfopen("/dev/null", "w");
 		options->close_file = 1;
-		options->color_moved = 0;
+		options->markup_moved = 0;
 		for (i = 0; i < q->nr; i++) {
 			struct diff_filepair *p = q->queue[i];
 			if (check_pair_status(p))
diff --git a/diff.h b/diff.h
index b8bbe7baeb8..0dd1651dda4 100644
--- a/diff.h
+++ b/diff.h
@@ -217,7 +217,7 @@ struct diff_options {
 		COLOR_MOVED_PLAIN = 1,
 		COLOR_MOVED_ZEBRA = 2,
 		COLOR_MOVED_ZEBRA_DIM = 3,
-	} color_moved;
+	} markup_moved;
 	#define COLOR_MOVED_DEFAULT COLOR_MOVED_ZEBRA
 	#define COLOR_MOVED_MIN_ALNUM_COUNT 20
 };

-- 
2.18.0.597.ga71716f1ad-goog
Stefan Beller· Aug 4, 2018, 01:53 UTC · re: Stefan Beller · lore

[PATCH 6/7] diff.c: factor determine_line_color out of emit_diff_symbol_from_struct

Factoring out code that determines color for both the new and old code even saves some lines, though minuscule. However we introduce an offset computation in the color array.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 diff.c | 88 +++++++++++++++++++++++++++++-----------------------------
 1 file changed, 44 insertions(+), 44 deletions(-)
Show changes to diff.c +44 −44
diff --git a/diff.c b/diff.c
index d3829c7d086..56bab011df7 100644
--- a/diff.c
+++ b/diff.c
@@ -1028,6 +1028,48 @@ static void emit_line_ws_markup(struct diff_options *o,
 	}
 }
 
+static const char *determine_line_color(struct diff_options *o,
+					struct emitted_diff_symbol *eds)
+{
+	const char *set;
+	unsigned flags = eds->flags;
+
+	/*
+	 * To have these offsets work, we need to keep
+	 * DIFF_FILE_OLD_MOVED{_ALT, _ALT_DIM, DIM, _}
+	 * in the same order as their _NEW_ equivalents; we do not need
+	 * to care about DIFF_FILE_{NEW, OLD} and their relations to others.
+	 */
+	const int off = (eds->s == DIFF_SYMBOL_PLUS) ?
+		DIFF_FILE_NEW_MOVED - DIFF_FILE_OLD_MOVED : 0;
+
+	switch (flags & (DIFF_SYMBOL_MOVED_LINE |
+			 DIFF_SYMBOL_MOVED_LINE_ALT |
+			 DIFF_SYMBOL_MOVED_LINE_UNINTERESTING)) {
+	case DIFF_SYMBOL_MOVED_LINE |
+	     DIFF_SYMBOL_MOVED_LINE_ALT |
+	     DIFF_SYMBOL_MOVED_LINE_UNINTERESTING:
+		set = diff_get_color_opt(o, DIFF_FILE_OLD_MOVED_ALT_DIM + off);
+		break;
+	case DIFF_SYMBOL_MOVED_LINE |
+	     DIFF_SYMBOL_MOVED_LINE_ALT:
+		set = diff_get_color_opt(o, DIFF_FILE_OLD_MOVED_ALT + off);
+		break;
+	case DIFF_SYMBOL_MOVED_LINE |
+	     DIFF_SYMBOL_MOVED_LINE_UNINTERESTING:
+		set = diff_get_color_opt(o, DIFF_FILE_OLD_MOVED_DIM + off);
+		break;
+	case DIFF_SYMBOL_MOVED_LINE:
+		set = diff_get_color_opt(o, DIFF_FILE_OLD_MOVED + off);
+		break;
+	default:
+		set = (eds->s == DIFF_SYMBOL_PLUS) ?
+			diff_get_color_opt(o, DIFF_FILE_NEW):
+			diff_get_color_opt(o, DIFF_FILE_OLD);
+	}
+	return set;
+}
+
 static void emit_diff_symbol_from_struct(struct diff_options *o,
 					 struct emitted_diff_symbol *eds)
 {
@@ -1089,28 +1131,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,
 				    flags & (DIFF_SYMBOL_CONTENT_WS_MASK), 0);
 		break;
 	case DIFF_SYMBOL_PLUS:
-		switch (flags & (DIFF_SYMBOL_MOVED_LINE |
-				 DIFF_SYMBOL_MOVED_LINE_ALT |
-				 DIFF_SYMBOL_MOVED_LINE_UNINTERESTING)) {
-		case DIFF_SYMBOL_MOVED_LINE |
-		     DIFF_SYMBOL_MOVED_LINE_ALT |
-		     DIFF_SYMBOL_MOVED_LINE_UNINTERESTING:
-			set = diff_get_color_opt(o, DIFF_FILE_NEW_MOVED_ALT_DIM);
-			break;
-		case DIFF_SYMBOL_MOVED_LINE |
-		     DIFF_SYMBOL_MOVED_LINE_ALT:
-			set = diff_get_color_opt(o, DIFF_FILE_NEW_MOVED_ALT);
-			break;
-		case DIFF_SYMBOL_MOVED_LINE |
-		     DIFF_SYMBOL_MOVED_LINE_UNINTERESTING:
-			set = diff_get_color_opt(o, DIFF_FILE_NEW_MOVED_DIM);
-			break;
-		case DIFF_SYMBOL_MOVED_LINE:
-			set = diff_get_color_opt(o, DIFF_FILE_NEW_MOVED);
-			break;
-		default:
-			set = diff_get_color_opt(o, DIFF_FILE_NEW);
-		}
+		set = determine_line_color(o, eds);
 		reset = diff_get_color_opt(o, DIFF_RESET);
 		if (!o->flags.dual_color_diffed_diffs)
 			set_sign = NULL;
@@ -1136,28 +1157,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,
 				    flags & DIFF_SYMBOL_CONTENT_BLANK_LINE_EOF);
 		break;
 	case DIFF_SYMBOL_MINUS:
-		switch (flags & (DIFF_SYMBOL_MOVED_LINE |
-				 DIFF_SYMBOL_MOVED_LINE_ALT |
-				 DIFF_SYMBOL_MOVED_LINE_UNINTERESTING)) {
-		case DIFF_SYMBOL_MOVED_LINE |
-		     DIFF_SYMBOL_MOVED_LINE_ALT |
-		     DIFF_SYMBOL_MOVED_LINE_UNINTERESTING:
-			set = diff_get_color_opt(o, DIFF_FILE_OLD_MOVED_ALT_DIM);
-			break;
-		case DIFF_SYMBOL_MOVED_LINE |
-		     DIFF_SYMBOL_MOVED_LINE_ALT:
-			set = diff_get_color_opt(o, DIFF_FILE_OLD_MOVED_ALT);
-			break;
-		case DIFF_SYMBOL_MOVED_LINE |
-		     DIFF_SYMBOL_MOVED_LINE_UNINTERESTING:
-			set = diff_get_color_opt(o, DIFF_FILE_OLD_MOVED_DIM);
-			break;
-		case DIFF_SYMBOL_MOVED_LINE:
-			set = diff_get_color_opt(o, DIFF_FILE_OLD_MOVED);
-			break;
-		default:
-			set = diff_get_color_opt(o, DIFF_FILE_OLD);
-		}
+		set = determine_line_color(o, eds);
 		reset = diff_get_color_opt(o, DIFF_RESET);
 		if (!o->flags.dual_color_diffed_diffs)
 			set_sign = NULL;

-- 
2.18.0.597.ga71716f1ad-goog
Stefan Beller· Aug 4, 2018, 01:53 UTC · re: Stefan Beller · lore

[RFC PATCH 7/7] diff/am: enhance diff format to use */~ for moved lines

Try it out via
    ./git-format-patch --mark-moved 15ef69314d^..15ef69314d
to see if you like it.

This separates the coloring decision from the detection of moved lines. When giving --mark-moved, move detection is still performed and the output markers are adjusted to */~ for new and old code.

git-apply and git-am will also accept these patches by rewriting those signs back to +/-.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 apply.c | 12 ++++++++++++
 diff.c  | 21 +++++++++++++++++++++
 diff.h  |  5 ++++-
 3 files changed, 37 insertions(+), 1 deletion(-)
Show changes to 3 files +37 −1

apply.c, diff.c, diff.h

diff --git a/apply.c b/apply.c
index 23a0f25ded8..cc42a4fa02a 100644
--- a/apply.c
+++ b/apply.c
@@ -2900,6 +2900,12 @@ static int apply_one_fragment(struct apply_state *state,
 			    ws_blank_line(patch + 1, plen, ws_rule))
 				is_blank_context = 1;
 			/* fallthrough */
+		case '~':
+			/*
+			 * For now ignore moved line indicators and apply
+			 * as a regular old line
+			 */
+			/* fallthrough */
 		case '-':
 			memcpy(old, patch + 1, plen);
 			add_line_info(&preimage, old, plen,
@@ -2908,6 +2914,12 @@ static int apply_one_fragment(struct apply_state *state,
 			if (first == '-')
 				break;
 			/* fallthrough */
+		case '*':
+			/*
+			 * For now ignore moved line indicators and apply
+			 * as a regular new line
+			 */
+			/* fallthrough */
 		case '+':
 			/* --no-add does not add new lines */
 			if (first == '+' && state->no_add)
diff --git a/diff.c b/diff.c
index 56bab011df7..8e39e77229f 100644
--- a/diff.c
+++ b/diff.c
@@ -1043,6 +1043,9 @@ static const char *determine_line_color(struct diff_options *o,
 	const int off = (eds->s == DIFF_SYMBOL_PLUS) ?
 		DIFF_FILE_NEW_MOVED - DIFF_FILE_OLD_MOVED : 0;
 
+	if (!o->color_moved)
+		goto default_color;
+
 	switch (flags & (DIFF_SYMBOL_MOVED_LINE |
 			 DIFF_SYMBOL_MOVED_LINE_ALT |
 			 DIFF_SYMBOL_MOVED_LINE_UNINTERESTING)) {
@@ -1063,6 +1066,7 @@ static const char *determine_line_color(struct diff_options *o,
 		set = diff_get_color_opt(o, DIFF_FILE_OLD_MOVED + off);
 		break;
 	default:
+default_color:
 		set = (eds->s == DIFF_SYMBOL_PLUS) ?
 			diff_get_color_opt(o, DIFF_FILE_NEW):
 			diff_get_color_opt(o, DIFF_FILE_OLD);
@@ -1152,6 +1156,9 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,
 
 		first = o->output_indicators[OI_NEW] ?
 			o->output_indicators[OI_NEW] : "+";
+		if (o->output_indicators[OI_MOVED_NEW] &&
+		   (flags & DIFF_SYMBOL_MOVED_LINE))
+			first = "*";
 		emit_line_ws_markup(o, set_sign, set, reset, first, line, len,
 				    flags & DIFF_SYMBOL_CONTENT_WS_MASK,
 				    flags & DIFF_SYMBOL_CONTENT_BLANK_LINE_EOF);
@@ -1176,6 +1183,9 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,
 		}
 		first = o->output_indicators[OI_OLD] ?
 			o->output_indicators[OI_OLD] : "-";
+		if (o->output_indicators[OI_MOVED_NEW] &&
+		   (flags & DIFF_SYMBOL_MOVED_LINE))
+			first = "~";
 		emit_line_ws_markup(o, set_sign, set, reset, first, line, len,
 				    flags & DIFF_SYMBOL_CONTENT_WS_MASK, 0);
 		break;
@@ -4795,6 +4805,7 @@ int diff_opt_parse(struct diff_options *options,
 	else if (!strcmp(arg, "--no-color"))
 		options->use_color = 0;
 	else if (!strcmp(arg, "--color-moved")) {
+		options->color_moved = 1;
 		if (diff_color_moved_default)
 			options->markup_moved = diff_color_moved_default;
 		if (options->markup_moved == COLOR_MOVED_NO)
@@ -4806,6 +4817,16 @@ int diff_opt_parse(struct diff_options *options,
 		if (cm < 0)
 			die("bad --color-moved argument: %s", arg);
 		options->markup_moved = cm;
+		options->color_moved = 1;
+	} else if (skip_prefix(arg, "--mark-moved", &arg)) {
+		/*
+		 * NEEDSWORK:
+		 * Once merged with 51da15eb230 (diff.c: add a blocks mode for
+		 * moved code detection, 2018-07-16), make it COLOR_MOVED_BLOCKS
+		 */
+		options->markup_moved = COLOR_MOVED_PLAIN;
+		options->output_indicators[OI_MOVED_NEW] = "*";
+		options->output_indicators[OI_MOVED_OLD] = "~";
 	} else if (skip_to_optional_arg_default(arg, "--color-words", &options->word_regex, NULL)) {
 		options->use_color = 1;
 		options->word_diff = DIFF_WORDS_COLOR;
diff --git a/diff.h b/diff.h
index 0dd1651dda4..0058602c849 100644
--- a/diff.h
+++ b/diff.h
@@ -197,7 +197,9 @@ struct diff_options {
 #define OI_NEW 0
 #define OI_OLD 1
 #define OI_CONTEXT 2
-	const char *output_indicators[3];
+#define OI_MOVED_NEW 3
+#define OI_MOVED_OLD 4
+	const char *output_indicators[5];
 
 	struct pathspec pathspec;
 	pathchange_fn_t pathchange;
@@ -211,6 +213,7 @@ struct diff_options {
 
 	int diff_path_counter;
 
+	unsigned color_moved : 1;
 	struct emitted_diff_symbols *emitted_symbols;
 	enum {
 		COLOR_MOVED_NO = 0,

-- 
2.18.0.597.ga71716f1ad-goog
Junio C Hamano· Aug 4, 2018, 17:15 UTC · re: Stefan Beller · lore

Re: [RFC PATCH 7/7] diff/am: enhance diff format to use */~ for moved lines

Stefan Beller <sbeller@google.com> writes:
Show 13 quoted lines
> Try it out via
>     ./git-format-patch --mark-moved 15ef69314d^..15ef69314d
> to see if you like it.
>
> This separates the coloring decision from the detection of moved lines.
> When giving --mark-moved, move detection is still performed and the output
> markers are adjusted to */~ for new and old code.
>
> git-apply and git-am will also accept these patches by rewriting those
> signs back to +/-.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---

This does not have anything to do with the range-diff topic, but would stand on its own merit. 

I have a mixed feeling about this.

If you need to convince "GNU patch" maintainers to accept these two signs, then probably it is not worth the battle of incompatiblity. If it is truly a worthy innovation, they would follow suit, which is how they learned to take our renaming diffs without us prodding them. I just do not get the gut feeling that it would happen for this particular thing, and I am not convinced myself enough to sell this to "patch" maintainers and expect to be taken seriously.

When reviewing anything complex that would be helped by moved code highlighting, I do not think a normal person would choose to review such a change only inside MUA. I certainly won't. I'd rather apply the patch and view it within a larger context than the piece of e-mail that was originally sent offers, with better tools like -W and --color-moved applied locally. So in that sense, I do not think I'd appreciate lines that begin with '~'/'*' as different kind of '-'/'+', as helpful hints; at least until my eyes get used to them, they would only appear as distraction.

In other words, I have this nagging suspicion that people who suggested to you that this would help in e-mail workflow are misguided and they do not understand e-mail workflow in the first place, but perhaps it is just me.

Thanks.
Stefan Beller· Aug 6, 2018, 06:07 UTC · re: Junio C Hamano · lore

Re: [RFC PATCH 7/7] diff/am: enhance diff format to use */~ for moved lines

On Sat, Aug 4, 2018 at 10:15 AM Junio C Hamano <gitster@pobox.com> wrote:
Show 19 quoted lines
>
> Stefan Beller <sbeller@google.com> writes:
>
> > Try it out via
> >     ./git-format-patch --mark-moved 15ef69314d^..15ef69314d
> > to see if you like it.
> >
> > This separates the coloring decision from the detection of moved lines.
> > When giving --mark-moved, move detection is still performed and the output
> > markers are adjusted to */~ for new and old code.
> >
> > git-apply and git-am will also accept these patches by rewriting those
> > signs back to +/-.
> >
> > Signed-off-by: Stefan Beller <sbeller@google.com>
> > ---
>
> This does not have anything to do with the range-diff topic, but
> would stand on its own merit.

Yes. I should have emphasized this more in the cover letter. This is more a "while at it" thing, that is easy to do due to the refactoring in previous patches.

> I have a mixed feeling about this.
Me, too.
Show 7 quoted lines
> If you need to convince "GNU patch" maintainers to accept these two
> signs, then probably it is not worth the battle of incompatiblity.
> If it is truly a worthy innovation, they would follow suit, which is
> how they learned to take our renaming diffs without us prodding
> them.  I just do not get the gut feeling that it would happen for
> this particular thing, and I am not convinced myself enough to sell
> this to "patch" maintainers and expect to be taken seriously.
ok.
Show 9 quoted lines
> When reviewing anything complex that would be helped by moved code
> highlighting, I do not think a normal person would choose to review
> such a change only inside MUA.  I certainly won't.  I'd rather apply
> the patch and view it within a larger context than the piece of
> e-mail that was originally sent offers, with better tools like -W
> and --color-moved applied locally.  So in that sense, I do not think
> I'd appreciate lines that begin with '~'/'*' as different kind of
> '-'/'+', as helpful hints; at least until my eyes get used to them,
> they would only appear as distraction.

My use case would be patches that are *not* complex, but still shuffling lots of code around, e.g. reordering functions/paragraphs in a file.

> In other words, I have this nagging suspicion that people who
> suggested to you that this would help in e-mail workflow are
> misguided and they do not understand e-mail workflow in the first
> place, but perhaps it is just me.

There are no other people that suggested this. It was really just a quick shot "while at it" as we had the refactoring in place that enables this, and I think for trivial patches (non-complex, but lots of changes) it *may* be beneficial. But it is more for corner cases, I guess.

Stefan
Junio C Hamano· Aug 4, 2018, 16:57 UTC · re: Stefan Beller · lore

Re: [PATCH 0/7] improve range-diffs coloring and [RFC] move detection

Stefan Beller <sbeller@google.com> writes:
> This builds on top of sb/range-diff-colors, which builds on js/range-diff.
As another round of js/range-diff is expected, according to
<nycvar.QRO.7.76.6.1808011800570.71@tvgsbejvaqbjf.bet>

I will refrain from queuing this right now. Possible conflict resolution that won't be reusable when the base one is rerolled and this and another topic that depend on the current round of js/range-diff are rebased on top is not something I can spend my time on this week.

Stefan Beller· Aug 6, 2018, 06:01 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/7] improve range-diffs coloring and [RFC] move detection

On Sat, Aug 4, 2018 at 9:57 AM Junio C Hamano <gitster@pobox.com> wrote:
Show 8 quoted lines
>
> Stefan Beller <sbeller@google.com> writes:
>
> > This builds on top of sb/range-diff-colors, which builds on js/range-diff.
>
> As another round of js/range-diff is expected, according to
>
> <nycvar.QRO.7.76.6.1808011800570.71@tvgsbejvaqbjf.bet>

Oh right. I forgot to mention that in this cover letter, but Johannes has had work issues last week and people threw lots of stuff at him, so I concluded the resend might take a while. Hence I just put it out there in case we're happy with the status quo of that series.

Show 5 quoted lines
> I will refrain from queuing this right now.  Possible conflict
> resolution that won't be reusable when the base one is rerolled and
> this and another topic that depend on the current round of
> js/range-diff are rebased on top is not something I can spend my
> time on this week.

Thanks! I'll resend when appropriate. Stefan

Junio C Hamano· Aug 6, 2018, 20:18 UTC · re: Stefan Beller · lore

Re: [PATCH 0/7] improve range-diffs coloring and [RFC] move detection

Stefan Beller <sbeller@google.com> writes:
Show 17 quoted lines
> On Sat, Aug 4, 2018 at 9:57 AM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Stefan Beller <sbeller@google.com> writes:
>>
>> > This builds on top of sb/range-diff-colors, which builds on js/range-diff.
>>
>> As another round of js/range-diff is expected, according to
>>
>> <nycvar.QRO.7.76.6.1808011800570.71@tvgsbejvaqbjf.bet>
>
> Oh right. I forgot to mention that in this cover letter, but Johannes
> has had work issues last week and people threw lots of stuff at him,
> so I concluded the resend might take a while. Hence I just put it out there
> in case we're happy with the status quo of that series.
> ...
>
> Thanks! I'll resend when appropriate.
Thanks; that way I have one less thing I have to remember ;-)

← back to recent threads