All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH RFC 1/2] strbuf: move strbuf_add_tabexpand into strbuf.c
@ 2017-03-28 12:22 Jacob Keller
  2017-03-28 12:22 ` [PATCH RFC 2/2] diff: teach diff to expand tabs in output Jacob Keller
  0 siblings, 1 reply; 6+ messages in thread
From: Jacob Keller @ 2017-03-28 12:22 UTC (permalink / raw)
  To: git; +Cc: Jacob Keller

From: Jacob Keller <jacob.keller@gmail.com>

In commit 7cc13c717b52 ("pretty: expand tabs in indented logs to make
things line up properly", 2016-03-16) a new function was added to insert
a line into a strbuf while expanding the tabs into spaces. This
functionality was used to help show the log message correctly when it
has been indented, so as to properly display the expected output.

This functionality will be useful in a future patch that adds similar
functionality into git diff, so lets move it into strbuf.c and make it
a public function.

While we're doing this, rename a few of the variables to fix the
surrounding strbuf code.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
 pretty.c | 50 --------------------------------------------------
 strbuf.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 strbuf.h |  6 ++++++
 3 files changed, 56 insertions(+), 50 deletions(-)

diff --git a/pretty.c b/pretty.c
index d0f86f5d85ca..70368509ffea 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1658,56 +1658,6 @@ void pp_title_line(struct pretty_print_context *pp,
 	strbuf_release(&title);
 }
 
-static int pp_utf8_width(const char *start, const char *end)
-{
-	int width = 0;
-	size_t remain = end - start;
-
-	while (remain) {
-		int n = utf8_width(&start, &remain);
-		if (n < 0 || !start)
-			return -1;
-		width += n;
-	}
-	return width;
-}
-
-static void strbuf_add_tabexpand(struct strbuf *sb, int tabwidth,
-				 const char *line, int linelen)
-{
-	const char *tab;
-
-	while ((tab = memchr(line, '\t', linelen)) != NULL) {
-		int width = pp_utf8_width(line, tab);
-
-		/*
-		 * If it wasn't well-formed utf8, or it
-		 * had characters with badly defined
-		 * width (control characters etc), just
-		 * give up on trying to align things.
-		 */
-		if (width < 0)
-			break;
-
-		/* Output the data .. */
-		strbuf_add(sb, line, tab - line);
-
-		/* .. and the de-tabified tab */
-		strbuf_addchars(sb, ' ', tabwidth - (width % tabwidth));
-
-		/* Skip over the printed part .. */
-		linelen -= tab + 1 - line;
-		line = tab + 1;
-	}
-
-	/*
-	 * Print out everything after the last tab without
-	 * worrying about width - there's nothing more to
-	 * align.
-	 */
-	strbuf_add(sb, line, linelen);
-}
-
 /*
  * pp_handle_indent() prints out the intendation, and
  * the whole line (without the final newline), after
diff --git a/strbuf.c b/strbuf.c
index 00457940cfc1..6cecfcadb05b 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -275,6 +275,56 @@ void strbuf_commented_addf(struct strbuf *sb, const char *fmt, ...)
 	strbuf_release(&buf);
 }
 
+static int find_utf8_width(const char *start, const char *end)
+{
+	int width = 0;
+	size_t remain = end - start;
+
+	while (remain) {
+		int n = utf8_width(&start, &remain);
+		if (n < 0 || !start)
+			return -1;
+		width += n;
+	}
+	return width;
+}
+
+void strbuf_add_tabexpand(struct strbuf *sb, int tabwidth,
+			  const char *buf, size_t size)
+{
+	const char *tab;
+
+	while ((tab = memchr(buf, '\t', size)) != NULL) {
+		int width = find_utf8_width(buf, tab);
+
+		/*
+		 * If it wasn't well-formed utf8, or it
+		 * had characters with badly defined
+		 * width (control characters etc), just
+		 * give up on trying to align things.
+		 */
+		if (width < 0)
+			break;
+
+		/* Output the data .. */
+		strbuf_add(sb, buf, tab - buf);
+
+		/* .. and the de-tabified tab */
+		strbuf_addchars(sb, ' ', tabwidth - (width % tabwidth));
+
+		/* Skip over the printed part .. */
+		size -= tab + 1 - buf;
+		buf = tab + 1;
+	}
+
+	/*
+	 * Print out everything after the last tab without
+	 * worrying about width - there's nothing more to
+	 * align.
+	 */
+	strbuf_add(sb, buf, size);
+}
+
 void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap)
 {
 	int len;
diff --git a/strbuf.h b/strbuf.h
index 80047b1bb7b8..17e04911833e 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -238,6 +238,12 @@ extern void strbuf_splice(struct strbuf *, size_t pos, size_t len,
  */
 extern void strbuf_add_commented_lines(struct strbuf *out, const char *buf, size_t size);
 
+/**
+ * Add a NUL-terminated string to the buffer. Tabs will be expanded using the
+ * provided tabwidth.
+ */
+extern void strbuf_add_tabexpand(struct strbuf *sb, int tabwidth,
+				 const char *buf, size_t size);
 
 /**
  * Add data of given length to the buffer.
-- 
2.12.2.650.ga248b8c51283


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH RFC 2/2] diff: teach diff to expand tabs in output
  2017-03-28 12:22 [PATCH RFC 1/2] strbuf: move strbuf_add_tabexpand into strbuf.c Jacob Keller
@ 2017-03-28 12:22 ` Jacob Keller
  2017-03-28 19:03   ` Junio C Hamano
  0 siblings, 1 reply; 6+ messages in thread
From: Jacob Keller @ 2017-03-28 12:22 UTC (permalink / raw)
  To: git; +Cc: Jacob Keller

From: Jacob Keller <jacob.keller@gmail.com>

When creating a diff for contents, we prepend a single character to
represent the state of that line. This character can offset how the tabs
in the real content are displayed, which may result in weird alignment
issues when viewing the diffs. Teach the diff core a new option to
expand these tabs, similar to how we already expand log contents.

This new option can be used to display the lines so that the reader can
see the expected results without the confusion of the offset tabstops
caused by the extra character prepended to each line.

Because some of the printing location is tied up into the whitespace
checking code, we also need to teach this code that it may need to
expand tabs as well. We will expand the output at the last moment so
that the whitespace checks see the contents before it is expanded.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
I'm really not a fan of how the ws code ended up. It seems pretty ugly
and weird to hack in the expand_tabs stuff here. However, I'm really not
sure how else I could handle this. Additionally, I'm not 100% sure
whether this interacts with format-patch or other machinery which may
well want some way to be excluded. Thoughts? Does anyone have a better
implementation?

I think there also may be some wonky bits when performing the tab
expansion during whitespace checks, due to the way we expand, because I
don't think that the tabexpand function takes into account the "current"
location when adding a string, so it very well may not be correct.... I
am unsure if there is a good way to fix this.

 Documentation/diff-options.txt |  6 ++++
 cache.h                        |  2 +-
 diff.c                         | 23 +++++++++++++--
 diff.h                         |  6 ++++
 t/t4063-diff-expand-tabs.sh    | 65 ++++++++++++++++++++++++++++++++++++++++++
 ws.c                           | 42 ++++++++++++++++++---------
 6 files changed, 126 insertions(+), 18 deletions(-)
 create mode 100755 t/t4063-diff-expand-tabs.sh

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 89cc0f48deef..82e314b20b3d 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -329,6 +329,12 @@ endif::git-format-patch[]
 	the diff-patch output format.  Non default number of
 	digits can be specified with `--abbrev=<n>`.
 
+--diff-expand-tabs[=<n>]::
+	When showing diff output, expand any tabs into spaces first before
+	printing. The size of the tabs is determined by <n> which defaults to
+	8 if not provided. --no-diff-expand-tabs or a size of 0 will disable
+	expansion.
+
 -B[<n>][/<m>]::
 --break-rewrites[=[<n>][/<m>]]::
 	Break complete rewrite changes into pairs of delete and
diff --git a/cache.h b/cache.h
index 5c8078291c47..2e221174edd4 100644
--- a/cache.h
+++ b/cache.h
@@ -2155,7 +2155,7 @@ extern unsigned whitespace_rule_cfg;
 extern unsigned whitespace_rule(const char *);
 extern unsigned parse_whitespace_rule(const char *);
 extern unsigned ws_check(const char *line, int len, unsigned ws_rule);
-extern void ws_check_emit(const char *line, int len, unsigned ws_rule, FILE *stream, const char *set, const char *reset, const char *ws);
+extern void ws_check_emit(const char *line, int len, unsigned ws_rule, FILE *stream, const char *set, const char *reset, const char *ws, int expand_tabs);
 extern char *whitespace_error_string(unsigned ws);
 extern void ws_fix_copy(struct strbuf *, const char *, int, unsigned, int *);
 extern int ws_blank_line(const char *line, int len, unsigned ws_rule);
diff --git a/diff.c b/diff.c
index 58cb72d7e72a..488019335df7 100644
--- a/diff.c
+++ b/diff.c
@@ -544,7 +544,14 @@ static void emit_line_0(struct diff_options *o, const char *set, const char *res
 		fputs(set, file);
 		if (!nofirst)
 			fputc(first, file);
-		fwrite(line, len, 1, file);
+		if (o->expand_tabs) {
+			struct strbuf sb = STRBUF_INIT;
+			strbuf_add_tabexpand(&sb, o->expand_tabs, line, len);
+			fwrite(sb.buf, sb.len, 1, file);
+			strbuf_release(&sb);
+		} else {
+			fwrite(line, len, 1, file);
+		}
 		fputs(reset, file);
 	}
 	if (has_trailing_carriage_return)
@@ -595,7 +602,8 @@ static void emit_line_checked(const char *reset,
 		/* Emit just the prefix, then the rest. */
 		emit_line_0(ecbdata->opt, set, reset, sign, "", 0);
 		ws_check_emit(line, len, ecbdata->ws_rule,
-			      ecbdata->opt->file, set, reset, ws);
+			      ecbdata->opt->file, set, reset, ws,
+			      ecbdata->opt->expand_tabs);
 	}
 }
 
@@ -2190,7 +2198,7 @@ static void checkdiff_consume(void *priv, char *line, unsigned long len)
 		free(err);
 		emit_line(data->o, set, reset, line, 1);
 		ws_check_emit(line + 1, len - 1, data->ws_rule,
-			      data->o->file, set, reset, ws);
+			      data->o->file, set, reset, ws, 0);
 	} else if (line[0] == ' ') {
 		data->lineno++;
 	} else if (line[0] == '@') {
@@ -4044,6 +4052,15 @@ int diff_opt_parse(struct diff_options *options,
 		else if (40 < options->abbrev)
 			options->abbrev = 40;
 	}
+	else if (!strcmp(arg, "--no-diff-expand-tabs"))
+		options->expand_tabs = 0;
+	else if (!strcmp(arg, "--diff-expand-tabs"))
+		options->expand_tabs = DEFAULT_EXPAND_TAB_WIDTH;
+	else if (skip_prefix(arg, "--diff-expand-tabs=", &arg)) {
+		options->expand_tabs = strtoul(arg, NULL, 10);
+		if (options->expand_tabs < 0)
+			options->expand_tabs = 0;
+	}
 	else if ((argcount = parse_long_opt("src-prefix", av, &optarg))) {
 		options->a_prefix = optarg;
 		return argcount;
diff --git a/diff.h b/diff.h
index e9ccb38c26c7..3de7fe68e6b1 100644
--- a/diff.h
+++ b/diff.h
@@ -147,6 +147,8 @@ struct diff_options {
 	int setup;
 	int abbrev;
 	int ita_invisible_in_index;
+#define DEFAULT_EXPAND_TAB_WIDTH 8
+	int expand_tabs;
 /* white-space error highlighting */
 #define WSEH_NEW 1
 #define WSEH_CONTEXT 2
@@ -308,6 +310,10 @@ extern void diffcore_fix_diff_index(struct diff_options *);
 "  --name-status show names and status of changed files.\n" \
 "  --full-index  show full object name on index lines.\n" \
 "  --abbrev=<n>  abbreviate object names in diff-tree header and diff-raw.\n" \
+"  --diff-expand-tabs=<n>\n" \
+"                expand tabs in the output to spaces, to preserve the\n" \
+"                alignment of tabs as compared to the input. Tabs will expand\n" \
+"                to <n> spaces. If <n> is 0, expansion is disabled.\n" \
 "  -R            swap input file pairs.\n" \
 "  -B            detect complete rewrites.\n" \
 "  -M            detect renames.\n" \
diff --git a/t/t4063-diff-expand-tabs.sh b/t/t4063-diff-expand-tabs.sh
new file mode 100755
index 000000000000..929a6103f6aa
--- /dev/null
+++ b/t/t4063-diff-expand-tabs.sh
@@ -0,0 +1,65 @@
+#!/bin/sh
+#
+# Copyright (c) 2017 Jacob Keller
+#
+
+test_description='Test diff tab expansion
+
+'
+. ./test-lib.sh
+. "$TEST_DIRECTORY"/diff-lib.sh
+
+test_expect_success '--diff-expand-tabs setup' '
+	git reset --hard &&
+	{
+		echo "0. no tabs"
+	} >x &&
+	git update-index --add x &&
+	git commit -a --allow-empty -m preimage &&
+	{
+		echo "0.	some	tabs"
+	} >x &&
+
+	cat >expect.default <<-\EOF &&
+	diff --git a/x b/x
+	index e4d15da..410478b 100644
+	--- a/x
+	+++ b/x
+	@@ -1 +1 @@
+	-0. no tabs
+	+0.      some    tabs
+	EOF
+
+	cat >expect.size-12 <<-\EOF &&
+	diff --git a/x b/x
+	index e4d15da..410478b 100644
+	--- a/x
+	+++ b/x
+	@@ -1 +1 @@
+	-0. no tabs
+	+0.          some        tabs
+	EOF
+
+	cat >expect.size-0 <<-\EOF
+	diff --git a/x b/x
+	index e4d15da..410478b 100644
+	--- a/x
+	+++ b/x
+	@@ -1 +1 @@
+	-0. no tabs
+	+0.	some	tabs
+	EOF
+'
+
+test_expect_success 'test --diff-expand-tabs option' '
+	git diff --diff-expand-tabs >actual &&
+	test_cmp expect.default actual &&
+
+	git diff --diff-expand-tabs=12 >actual &&
+	test_cmp expect.size-12 actual &&
+
+	git diff --diff-expand-tabs=0 >actual &&
+	test_cmp expect.size-0 actual
+'
+
+test_done
diff --git a/ws.c b/ws.c
index a07caedd5a56..316081c7c2ee 100644
--- a/ws.c
+++ b/ws.c
@@ -139,10 +139,24 @@ char *whitespace_error_string(unsigned ws)
 	return strbuf_detach(&err, NULL);
 }
 
+/* Emit the line to the stream, expanding tabs if necessary */
+static void ws_emit(int expand_tabs, const char *line, int len, FILE *stream)
+{
+	if (expand_tabs) {
+		struct strbuf sb = STRBUF_INIT;
+		strbuf_add_tabexpand(&sb, expand_tabs, line, len);
+		fwrite(sb.buf, sb.len, 1, stream);
+		strbuf_release(&sb);
+	} else {
+		fwrite(line, len, 1, stream);
+	}
+}
+
 /* If stream is non-NULL, emits the line after checking. */
 static unsigned ws_check_emit_1(const char *line, int len, unsigned ws_rule,
 				FILE *stream, const char *set,
-				const char *reset, const char *ws)
+				const char *reset, const char *ws,
+				int expand_tabs)
 {
 	unsigned result = 0;
 	int written = 0;
@@ -187,20 +201,20 @@ static unsigned ws_check_emit_1(const char *line, int len, unsigned ws_rule,
 			result |= WS_SPACE_BEFORE_TAB;
 			if (stream) {
 				fputs(ws, stream);
-				fwrite(line + written, i - written, 1, stream);
+				ws_emit(expand_tabs, line + written, i - written, stream);
 				fputs(reset, stream);
-				fwrite(line + i, 1, 1, stream);
+				ws_emit(expand_tabs, line + i, 1, stream);
 			}
 		} else if (ws_rule & WS_TAB_IN_INDENT) {
 			result |= WS_TAB_IN_INDENT;
 			if (stream) {
-				fwrite(line + written, i - written, 1, stream);
+				ws_emit(expand_tabs, line + written, i - written, stream);
 				fputs(ws, stream);
-				fwrite(line + i, 1, 1, stream);
+				ws_emit(expand_tabs, line + i, 1, stream);
 				fputs(reset, stream);
 			}
 		} else if (stream) {
-			fwrite(line + written, i - written + 1, 1, stream);
+			ws_emit(expand_tabs, line + written, i - written + 1, stream);
 		}
 		written = i + 1;
 	}
@@ -210,7 +224,7 @@ static unsigned ws_check_emit_1(const char *line, int len, unsigned ws_rule,
 		result |= WS_INDENT_WITH_NON_TAB;
 		if (stream) {
 			fputs(ws, stream);
-			fwrite(line + written, i - written, 1, stream);
+			ws_emit(expand_tabs, line + written, i - written, stream);
 			fputs(reset, stream);
 		}
 		written = i;
@@ -225,16 +239,16 @@ static unsigned ws_check_emit_1(const char *line, int len, unsigned ws_rule,
 		/* Emit non-highlighted (middle) segment. */
 		if (trailing_whitespace - written > 0) {
 			fputs(set, stream);
-			fwrite(line + written,
-			    trailing_whitespace - written, 1, stream);
+			ws_emit(expand_tabs, line + written,
+			    trailing_whitespace - written, stream);
 			fputs(reset, stream);
 		}
 
 		/* Highlight errors in trailing whitespace. */
 		if (trailing_whitespace != len) {
 			fputs(ws, stream);
-			fwrite(line + trailing_whitespace,
-			    len - trailing_whitespace, 1, stream);
+			ws_emit(expand_tabs, line + trailing_whitespace,
+			    len - trailing_whitespace, stream);
 			fputs(reset, stream);
 		}
 		if (trailing_carriage_return)
@@ -247,14 +261,14 @@ static unsigned ws_check_emit_1(const char *line, int len, unsigned ws_rule,
 
 void ws_check_emit(const char *line, int len, unsigned ws_rule,
 		   FILE *stream, const char *set,
-		   const char *reset, const char *ws)
+		   const char *reset, const char *ws, int expand_tabs)
 {
-	(void)ws_check_emit_1(line, len, ws_rule, stream, set, reset, ws);
+	(void)ws_check_emit_1(line, len, ws_rule, stream, set, reset, ws, expand_tabs);
 }
 
 unsigned ws_check(const char *line, int len, unsigned ws_rule)
 {
-	return ws_check_emit_1(line, len, ws_rule, NULL, NULL, NULL, NULL);
+	return ws_check_emit_1(line, len, ws_rule, NULL, NULL, NULL, NULL, 0);
 }
 
 int ws_blank_line(const char *line, int len, unsigned ws_rule)
-- 
2.12.2.650.ga248b8c51283


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH RFC 2/2] diff: teach diff to expand tabs in output
  2017-03-28 12:22 ` [PATCH RFC 2/2] diff: teach diff to expand tabs in output Jacob Keller
@ 2017-03-28 19:03   ` Junio C Hamano
  2017-03-28 20:05     ` Jacob Keller
  0 siblings, 1 reply; 6+ messages in thread
From: Junio C Hamano @ 2017-03-28 19:03 UTC (permalink / raw)
  To: Jacob Keller; +Cc: git, Jacob Keller

Jacob Keller <jacob.e.keller@intel.com> writes:

> I'm really not a fan of how the ws code ended up. It seems pretty ugly
> and weird to hack in the expand_tabs stuff here. However, I'm really not
> sure how else I could handle this. Additionally, I'm not 100% sure
> whether this interacts with format-patch or other machinery which may
> well want some way to be excluded. Thoughts?

As long as you do the same as "do we color the output?  no, no, we
are format-patch and must not color" logic to refrain from expanding
the tabs, you should be OK.

> I think there also may be some wonky bits when performing the tab
> expansion during whitespace checks, due to the way we expand, because I
> don't think that the tabexpand function takes into account the "current"
> location when adding a string, so it very well may not be correct.... I
> am unsure if there is a good way to fix this.

This "feature" is limited to the diff output, so one way may be to
leave the code as-is and pipe the output to a filter that is similar
to /usr/bin/expand but knows that the first column is special (this
is the part that "this is limited to diff" kicks in).  You may even
be able to implement it as a new option to "expand(1)" and then
people who aren't Git users would also benefit.


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH RFC 2/2] diff: teach diff to expand tabs in output
  2017-03-28 19:03   ` Junio C Hamano
@ 2017-03-28 20:05     ` Jacob Keller
  2017-03-28 20:38       ` Jeff King
  0 siblings, 1 reply; 6+ messages in thread
From: Jacob Keller @ 2017-03-28 20:05 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Jacob Keller, Git mailing list

On Tue, Mar 28, 2017 at 12:03 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jacob Keller <jacob.e.keller@intel.com> writes:
>
>> I'm really not a fan of how the ws code ended up. It seems pretty ugly
>> and weird to hack in the expand_tabs stuff here. However, I'm really not
>> sure how else I could handle this. Additionally, I'm not 100% sure
>> whether this interacts with format-patch or other machinery which may
>> well want some way to be excluded. Thoughts?
>
> As long as you do the same as "do we color the output?  no, no, we
> are format-patch and must not color" logic to refrain from expanding
> the tabs, you should be OK.
>
>> I think there also may be some wonky bits when performing the tab
>> expansion during whitespace checks, due to the way we expand, because I
>> don't think that the tabexpand function takes into account the "current"
>> location when adding a string, so it very well may not be correct.... I
>> am unsure if there is a good way to fix this.
>
> This "feature" is limited to the diff output, so one way may be to
> leave the code as-is and pipe the output to a filter that is similar
> to /usr/bin/expand but knows that the first column is special (this
> is the part that "this is limited to diff" kicks in).  You may even
> be able to implement it as a new option to "expand(1)" and then
> people who aren't Git users would also benefit.
>

That makes more sense. I'll take a look at that. It might even be
possible to modify the pager setup so that it does that as part of its
process.

Thanks,
Jake

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH RFC 2/2] diff: teach diff to expand tabs in output
  2017-03-28 20:05     ` Jacob Keller
@ 2017-03-28 20:38       ` Jeff King
  2017-03-28 20:46         ` Junio C Hamano
  0 siblings, 1 reply; 6+ messages in thread
From: Jeff King @ 2017-03-28 20:38 UTC (permalink / raw)
  To: Jacob Keller; +Cc: Junio C Hamano, Jacob Keller, Git mailing list

On Tue, Mar 28, 2017 at 01:05:40PM -0700, Jacob Keller wrote:

> On Tue, Mar 28, 2017 at 12:03 PM, Junio C Hamano <gitster@pobox.com> wrote:
> > Jacob Keller <jacob.e.keller@intel.com> writes:
> >
> >> I'm really not a fan of how the ws code ended up. It seems pretty ugly
> >> and weird to hack in the expand_tabs stuff here. However, I'm really not
> >> sure how else I could handle this. Additionally, I'm not 100% sure
> >> whether this interacts with format-patch or other machinery which may
> >> well want some way to be excluded. Thoughts?
> >
> > As long as you do the same as "do we color the output?  no, no, we
> > are format-patch and must not color" logic to refrain from expanding
> > the tabs, you should be OK.
> >
> >> I think there also may be some wonky bits when performing the tab
> >> expansion during whitespace checks, due to the way we expand, because I
> >> don't think that the tabexpand function takes into account the "current"
> >> location when adding a string, so it very well may not be correct.... I
> >> am unsure if there is a good way to fix this.
> >
> > This "feature" is limited to the diff output, so one way may be to
> > leave the code as-is and pipe the output to a filter that is similar
> > to /usr/bin/expand but knows that the first column is special (this
> > is the part that "this is limited to diff" kicks in).  You may even
> > be able to implement it as a new option to "expand(1)" and then
> > people who aren't Git users would also benefit.
> >
> 
> That makes more sense. I'll take a look at that. It might even be
> possible to modify the pager setup so that it does that as part of its
> process.

This is similar to how I use diff-highlight, which is basically:

  [pager]
  log = diff-highlight | less
  show = diff-highlight | less
  diff = diff-highlight | less

I've wanted to move diff-highlight inside git, but ran into ugly
conflicts with the whitespace-marking code as well. Something like:

  git show b16a991c1be5681b4b673d4343dfcc0c2f5ad498 |
  perl -pe 's/^(.)(\t+)/$1 . (" " x (length($2) * 8))/e'

But sticking it in your pager pipeline is tough, because you actually
need to skip over the color bytes when they are present. You can see in
diff-highlight how this is handled.

That also means an option to something like "expand" is tough, because
"first character" really means "first non-ANSI-code character".

-Peff

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH RFC 2/2] diff: teach diff to expand tabs in output
  2017-03-28 20:38       ` Jeff King
@ 2017-03-28 20:46         ` Junio C Hamano
  0 siblings, 0 replies; 6+ messages in thread
From: Junio C Hamano @ 2017-03-28 20:46 UTC (permalink / raw)
  To: Jeff King; +Cc: Jacob Keller, Jacob Keller, Git mailing list

Jeff King <peff@peff.net> writes:

> That also means an option to something like "expand" is tough, because
> "first character" really means "first non-ANSI-code character".

That is true, but once you commit to the mindset that you are
extending "expand", then that becomes a mere part of what must be
done, i.e. if you want your "expand" to be able to handle coloured
input, you'd need to know how wide each segment of the input is.
For that matter, you would also want your "expand" to pay attention
to the differences between display- vs byte-widths of a string,
perhaps reusing utf8_strwidth() or something.

Also "the first character is special" does not have to be a "diff
specific hack".  Your extended "expand" can take a list of
tab-widths and the special case for highlighting diff output happens
to take 9,8 as the "list of tab-widths" parameter (whose semantics
is that each number in the list tells how wide the tab is, and the
last number repeats forever, so 9,8 really means 9,8,8,8,8,...).

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2017-03-28 20:46 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-28 12:22 [PATCH RFC 1/2] strbuf: move strbuf_add_tabexpand into strbuf.c Jacob Keller
2017-03-28 12:22 ` [PATCH RFC 2/2] diff: teach diff to expand tabs in output Jacob Keller
2017-03-28 19:03   ` Junio C Hamano
2017-03-28 20:05     ` Jacob Keller
2017-03-28 20:38       ` Jeff King
2017-03-28 20:46         ` Junio C Hamano

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.