{"thread":{"id":"45526","subject":"[PATCH RFC 1/2] strbuf: move strbuf_add_tabexpand into strbuf.c","startedAt":"2017-03-28T12:23:25Z","lastAt":"2017-03-28T20:46:46Z","messageCount":6,"participants":["Jacob Keller","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"315563","messageId":"20170328122209.4861-1-jacob.e.keller@intel.com","threadId":"45526","inReplyTo":null,"subject":"[PATCH RFC 1/2] strbuf: move strbuf_add_tabexpand into strbuf.c","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2017-03-28T12:22:08Z","receivedAt":"2017-03-28T12:23:25Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller <jacob.keller@gmail.com>\n\nIn commit 7cc13c717b52 (\"pretty: expand tabs in indented logs to make\nthings line up properly\", 2016-03-16) a new function was added to insert\na line into a strbuf while expanding the tabs into spaces. This\nfunctionality was used to help show the log message correctly when it\nhas been indented, so as to properly display the expected output.\n\nThis functionality will be useful in a future patch that adds similar\nfunctionality into git diff, so lets move it into strbuf.c and make it\na public function.\n\nWhile we're doing this, rename a few of the variables to fix the\nsurrounding strbuf code.\n\nSigned-off-by: Jacob Keller <jacob.keller@gmail.com>\n---\n pretty.c | 50 --------------------------------------------------\n strbuf.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++\n strbuf.h |  6 ++++++\n 3 files changed, 56 insertions(+), 50 deletions(-)\n\ndiff --git a/pretty.c b/pretty.c\nindex d0f86f5d85ca..70368509ffea 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1658,56 +1658,6 @@ void pp_title_line(struct pretty_print_context *pp,\n \tstrbuf_release(&title);\n }\n \n-static int pp_utf8_width(const char *start, const char *end)\n-{\n-\tint width = 0;\n-\tsize_t remain = end - start;\n-\n-\twhile (remain) {\n-\t\tint n = utf8_width(&start, &remain);\n-\t\tif (n < 0 || !start)\n-\t\t\treturn -1;\n-\t\twidth += n;\n-\t}\n-\treturn width;\n-}\n-\n-static void strbuf_add_tabexpand(struct strbuf *sb, int tabwidth,\n-\t\t\t\t const char *line, int linelen)\n-{\n-\tconst char *tab;\n-\n-\twhile ((tab = memchr(line, '\\t', linelen)) != NULL) {\n-\t\tint width = pp_utf8_width(line, tab);\n-\n-\t\t/*\n-\t\t * If it wasn't well-formed utf8, or it\n-\t\t * had characters with badly defined\n-\t\t * width (control characters etc), just\n-\t\t * give up on trying to align things.\n-\t\t */\n-\t\tif (width < 0)\n-\t\t\tbreak;\n-\n-\t\t/* Output the data .. */\n-\t\tstrbuf_add(sb, line, tab - line);\n-\n-\t\t/* .. and the de-tabified tab */\n-\t\tstrbuf_addchars(sb, ' ', tabwidth - (width % tabwidth));\n-\n-\t\t/* Skip over the printed part .. */\n-\t\tlinelen -= tab + 1 - line;\n-\t\tline = tab + 1;\n-\t}\n-\n-\t/*\n-\t * Print out everything after the last tab without\n-\t * worrying about width - there's nothing more to\n-\t * align.\n-\t */\n-\tstrbuf_add(sb, line, linelen);\n-}\n-\n /*\n  * pp_handle_indent() prints out the intendation, and\n  * the whole line (without the final newline), after\ndiff --git a/strbuf.c b/strbuf.c\nindex 00457940cfc1..6cecfcadb05b 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -275,6 +275,56 @@ void strbuf_commented_addf(struct strbuf *sb, const char *fmt, ...)\n \tstrbuf_release(&buf);\n }\n \n+static int find_utf8_width(const char *start, const char *end)\n+{\n+\tint width = 0;\n+\tsize_t remain = end - start;\n+\n+\twhile (remain) {\n+\t\tint n = utf8_width(&start, &remain);\n+\t\tif (n < 0 || !start)\n+\t\t\treturn -1;\n+\t\twidth += n;\n+\t}\n+\treturn width;\n+}\n+\n+void strbuf_add_tabexpand(struct strbuf *sb, int tabwidth,\n+\t\t\t  const char *buf, size_t size)\n+{\n+\tconst char *tab;\n+\n+\twhile ((tab = memchr(buf, '\\t', size)) != NULL) {\n+\t\tint width = find_utf8_width(buf, tab);\n+\n+\t\t/*\n+\t\t * If it wasn't well-formed utf8, or it\n+\t\t * had characters with badly defined\n+\t\t * width (control characters etc), just\n+\t\t * give up on trying to align things.\n+\t\t */\n+\t\tif (width < 0)\n+\t\t\tbreak;\n+\n+\t\t/* Output the data .. */\n+\t\tstrbuf_add(sb, buf, tab - buf);\n+\n+\t\t/* .. and the de-tabified tab */\n+\t\tstrbuf_addchars(sb, ' ', tabwidth - (width % tabwidth));\n+\n+\t\t/* Skip over the printed part .. */\n+\t\tsize -= tab + 1 - buf;\n+\t\tbuf = tab + 1;\n+\t}\n+\n+\t/*\n+\t * Print out everything after the last tab without\n+\t * worrying about width - there's nothing more to\n+\t * align.\n+\t */\n+\tstrbuf_add(sb, buf, size);\n+}\n+\n void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap)\n {\n \tint len;\ndiff --git a/strbuf.h b/strbuf.h\nindex 80047b1bb7b8..17e04911833e 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -238,6 +238,12 @@ extern void strbuf_splice(struct strbuf *, size_t pos, size_t len,\n  */\n extern void strbuf_add_commented_lines(struct strbuf *out, const char *buf, size_t size);\n \n+/**\n+ * Add a NUL-terminated string to the buffer. Tabs will be expanded using the\n+ * provided tabwidth.\n+ */\n+extern void strbuf_add_tabexpand(struct strbuf *sb, int tabwidth,\n+\t\t\t\t const char *buf, size_t size);\n \n /**\n  * Add data of given length to the buffer.\n-- \n2.12.2.650.ga248b8c51283\n\n"},{"id":"315564","messageId":"20170328122209.4861-2-jacob.e.keller@intel.com","threadId":"45526","inReplyTo":"20170328122209.4861-1-jacob.e.keller@intel.com","subject":"[PATCH RFC 2/2] diff: teach diff to expand tabs in output","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2017-03-28T12:22:09Z","receivedAt":"2017-03-28T12:23:48Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller <jacob.keller@gmail.com>\n\nWhen creating a diff for contents, we prepend a single character to\nrepresent the state of that line. This character can offset how the tabs\nin the real content are displayed, which may result in weird alignment\nissues when viewing the diffs. Teach the diff core a new option to\nexpand these tabs, similar to how we already expand log contents.\n\nThis new option can be used to display the lines so that the reader can\nsee the expected results without the confusion of the offset tabstops\ncaused by the extra character prepended to each line.\n\nBecause some of the printing location is tied up into the whitespace\nchecking code, we also need to teach this code that it may need to\nexpand tabs as well. We will expand the output at the last moment so\nthat the whitespace checks see the contents before it is expanded.\n\nSigned-off-by: Jacob Keller <jacob.keller@gmail.com>\n---\nI'm really not a fan of how the ws code ended up. It seems pretty ugly\nand weird to hack in the expand_tabs stuff here. However, I'm really not\nsure how else I could handle this. Additionally, I'm not 100% sure\nwhether this interacts with format-patch or other machinery which may\nwell want some way to be excluded. Thoughts? Does anyone have a better\nimplementation?\n\nI think there also may be some wonky bits when performing the tab\nexpansion during whitespace checks, due to the way we expand, because I\ndon't think that the tabexpand function takes into account the \"current\"\nlocation when adding a string, so it very well may not be correct.... I\nam unsure if there is a good way to fix this.\n\n Documentation/diff-options.txt |  6 ++++\n cache.h                        |  2 +-\n diff.c                         | 23 +++++++++++++--\n diff.h                         |  6 ++++\n t/t4063-diff-expand-tabs.sh    | 65 ++++++++++++++++++++++++++++++++++++++++++\n ws.c                           | 42 ++++++++++++++++++---------\n 6 files changed, 126 insertions(+), 18 deletions(-)\n create mode 100755 t/t4063-diff-expand-tabs.sh\n\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex 89cc0f48deef..82e314b20b3d 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -329,6 +329,12 @@ endif::git-format-patch[]\n \tthe diff-patch output format.  Non default number of\n \tdigits can be specified with `--abbrev=<n>`.\n \n+--diff-expand-tabs[=<n>]::\n+\tWhen showing diff output, expand any tabs into spaces first before\n+\tprinting. The size of the tabs is determined by <n> which defaults to\n+\t8 if not provided. --no-diff-expand-tabs or a size of 0 will disable\n+\texpansion.\n+\n -B[<n>][/<m>]::\n --break-rewrites[=[<n>][/<m>]]::\n \tBreak complete rewrite changes into pairs of delete and\ndiff --git a/cache.h b/cache.h\nindex 5c8078291c47..2e221174edd4 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -2155,7 +2155,7 @@ extern unsigned whitespace_rule_cfg;\n extern unsigned whitespace_rule(const char *);\n extern unsigned parse_whitespace_rule(const char *);\n extern unsigned ws_check(const char *line, int len, unsigned ws_rule);\n-extern void ws_check_emit(const char *line, int len, unsigned ws_rule, FILE *stream, const char *set, const char *reset, const char *ws);\n+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);\n extern char *whitespace_error_string(unsigned ws);\n extern void ws_fix_copy(struct strbuf *, const char *, int, unsigned, int *);\n extern int ws_blank_line(const char *line, int len, unsigned ws_rule);\ndiff --git a/diff.c b/diff.c\nindex 58cb72d7e72a..488019335df7 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -544,7 +544,14 @@ static void emit_line_0(struct diff_options *o, const char *set, const char *res\n \t\tfputs(set, file);\n \t\tif (!nofirst)\n \t\t\tfputc(first, file);\n-\t\tfwrite(line, len, 1, file);\n+\t\tif (o->expand_tabs) {\n+\t\t\tstruct strbuf sb = STRBUF_INIT;\n+\t\t\tstrbuf_add_tabexpand(&sb, o->expand_tabs, line, len);\n+\t\t\tfwrite(sb.buf, sb.len, 1, file);\n+\t\t\tstrbuf_release(&sb);\n+\t\t} else {\n+\t\t\tfwrite(line, len, 1, file);\n+\t\t}\n \t\tfputs(reset, file);\n \t}\n \tif (has_trailing_carriage_return)\n@@ -595,7 +602,8 @@ static void emit_line_checked(const char *reset,\n \t\t/* Emit just the prefix, then the rest. */\n \t\temit_line_0(ecbdata->opt, set, reset, sign, \"\", 0);\n \t\tws_check_emit(line, len, ecbdata->ws_rule,\n-\t\t\t      ecbdata->opt->file, set, reset, ws);\n+\t\t\t      ecbdata->opt->file, set, reset, ws,\n+\t\t\t      ecbdata->opt->expand_tabs);\n \t}\n }\n \n@@ -2190,7 +2198,7 @@ static void checkdiff_consume(void *priv, char *line, unsigned long len)\n \t\tfree(err);\n \t\temit_line(data->o, set, reset, line, 1);\n \t\tws_check_emit(line + 1, len - 1, data->ws_rule,\n-\t\t\t      data->o->file, set, reset, ws);\n+\t\t\t      data->o->file, set, reset, ws, 0);\n \t} else if (line[0] == ' ') {\n \t\tdata->lineno++;\n \t} else if (line[0] == '@') {\n@@ -4044,6 +4052,15 @@ int diff_opt_parse(struct diff_options *options,\n \t\telse if (40 < options->abbrev)\n \t\t\toptions->abbrev = 40;\n \t}\n+\telse if (!strcmp(arg, \"--no-diff-expand-tabs\"))\n+\t\toptions->expand_tabs = 0;\n+\telse if (!strcmp(arg, \"--diff-expand-tabs\"))\n+\t\toptions->expand_tabs = DEFAULT_EXPAND_TAB_WIDTH;\n+\telse if (skip_prefix(arg, \"--diff-expand-tabs=\", &arg)) {\n+\t\toptions->expand_tabs = strtoul(arg, NULL, 10);\n+\t\tif (options->expand_tabs < 0)\n+\t\t\toptions->expand_tabs = 0;\n+\t}\n \telse if ((argcount = parse_long_opt(\"src-prefix\", av, &optarg))) {\n \t\toptions->a_prefix = optarg;\n \t\treturn argcount;\ndiff --git a/diff.h b/diff.h\nindex e9ccb38c26c7..3de7fe68e6b1 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -147,6 +147,8 @@ struct diff_options {\n \tint setup;\n \tint abbrev;\n \tint ita_invisible_in_index;\n+#define DEFAULT_EXPAND_TAB_WIDTH 8\n+\tint expand_tabs;\n /* white-space error highlighting */\n #define WSEH_NEW 1\n #define WSEH_CONTEXT 2\n@@ -308,6 +310,10 @@ extern void diffcore_fix_diff_index(struct diff_options *);\n \"  --name-status show names and status of changed files.\\n\" \\\n \"  --full-index  show full object name on index lines.\\n\" \\\n \"  --abbrev=<n>  abbreviate object names in diff-tree header and diff-raw.\\n\" \\\n+\"  --diff-expand-tabs=<n>\\n\" \\\n+\"                expand tabs in the output to spaces, to preserve the\\n\" \\\n+\"                alignment of tabs as compared to the input. Tabs will expand\\n\" \\\n+\"                to <n> spaces. If <n> is 0, expansion is disabled.\\n\" \\\n \"  -R            swap input file pairs.\\n\" \\\n \"  -B            detect complete rewrites.\\n\" \\\n \"  -M            detect renames.\\n\" \\\ndiff --git a/t/t4063-diff-expand-tabs.sh b/t/t4063-diff-expand-tabs.sh\nnew file mode 100755\nindex 000000000000..929a6103f6aa\n--- /dev/null\n+++ b/t/t4063-diff-expand-tabs.sh\n@@ -0,0 +1,65 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2017 Jacob Keller\n+#\n+\n+test_description='Test diff tab expansion\n+\n+'\n+. ./test-lib.sh\n+. \"$TEST_DIRECTORY\"/diff-lib.sh\n+\n+test_expect_success '--diff-expand-tabs setup' '\n+\tgit reset --hard &&\n+\t{\n+\t\techo \"0. no tabs\"\n+\t} >x &&\n+\tgit update-index --add x &&\n+\tgit commit -a --allow-empty -m preimage &&\n+\t{\n+\t\techo \"0.\tsome\ttabs\"\n+\t} >x &&\n+\n+\tcat >expect.default <<-\\EOF &&\n+\tdiff --git a/x b/x\n+\tindex e4d15da..410478b 100644\n+\t--- a/x\n+\t+++ b/x\n+\t@@ -1 +1 @@\n+\t-0. no tabs\n+\t+0.      some    tabs\n+\tEOF\n+\n+\tcat >expect.size-12 <<-\\EOF &&\n+\tdiff --git a/x b/x\n+\tindex e4d15da..410478b 100644\n+\t--- a/x\n+\t+++ b/x\n+\t@@ -1 +1 @@\n+\t-0. no tabs\n+\t+0.          some        tabs\n+\tEOF\n+\n+\tcat >expect.size-0 <<-\\EOF\n+\tdiff --git a/x b/x\n+\tindex e4d15da..410478b 100644\n+\t--- a/x\n+\t+++ b/x\n+\t@@ -1 +1 @@\n+\t-0. no tabs\n+\t+0.\tsome\ttabs\n+\tEOF\n+'\n+\n+test_expect_success 'test --diff-expand-tabs option' '\n+\tgit diff --diff-expand-tabs >actual &&\n+\ttest_cmp expect.default actual &&\n+\n+\tgit diff --diff-expand-tabs=12 >actual &&\n+\ttest_cmp expect.size-12 actual &&\n+\n+\tgit diff --diff-expand-tabs=0 >actual &&\n+\ttest_cmp expect.size-0 actual\n+'\n+\n+test_done\ndiff --git a/ws.c b/ws.c\nindex a07caedd5a56..316081c7c2ee 100644\n--- a/ws.c\n+++ b/ws.c\n@@ -139,10 +139,24 @@ char *whitespace_error_string(unsigned ws)\n \treturn strbuf_detach(&err, NULL);\n }\n \n+/* Emit the line to the stream, expanding tabs if necessary */\n+static void ws_emit(int expand_tabs, const char *line, int len, FILE *stream)\n+{\n+\tif (expand_tabs) {\n+\t\tstruct strbuf sb = STRBUF_INIT;\n+\t\tstrbuf_add_tabexpand(&sb, expand_tabs, line, len);\n+\t\tfwrite(sb.buf, sb.len, 1, stream);\n+\t\tstrbuf_release(&sb);\n+\t} else {\n+\t\tfwrite(line, len, 1, stream);\n+\t}\n+}\n+\n /* If stream is non-NULL, emits the line after checking. */\n static unsigned ws_check_emit_1(const char *line, int len, unsigned ws_rule,\n \t\t\t\tFILE *stream, const char *set,\n-\t\t\t\tconst char *reset, const char *ws)\n+\t\t\t\tconst char *reset, const char *ws,\n+\t\t\t\tint expand_tabs)\n {\n \tunsigned result = 0;\n \tint written = 0;\n@@ -187,20 +201,20 @@ static unsigned ws_check_emit_1(const char *line, int len, unsigned ws_rule,\n \t\t\tresult |= WS_SPACE_BEFORE_TAB;\n \t\t\tif (stream) {\n \t\t\t\tfputs(ws, stream);\n-\t\t\t\tfwrite(line + written, i - written, 1, stream);\n+\t\t\t\tws_emit(expand_tabs, line + written, i - written, stream);\n \t\t\t\tfputs(reset, stream);\n-\t\t\t\tfwrite(line + i, 1, 1, stream);\n+\t\t\t\tws_emit(expand_tabs, line + i, 1, stream);\n \t\t\t}\n \t\t} else if (ws_rule & WS_TAB_IN_INDENT) {\n \t\t\tresult |= WS_TAB_IN_INDENT;\n \t\t\tif (stream) {\n-\t\t\t\tfwrite(line + written, i - written, 1, stream);\n+\t\t\t\tws_emit(expand_tabs, line + written, i - written, stream);\n \t\t\t\tfputs(ws, stream);\n-\t\t\t\tfwrite(line + i, 1, 1, stream);\n+\t\t\t\tws_emit(expand_tabs, line + i, 1, stream);\n \t\t\t\tfputs(reset, stream);\n \t\t\t}\n \t\t} else if (stream) {\n-\t\t\tfwrite(line + written, i - written + 1, 1, stream);\n+\t\t\tws_emit(expand_tabs, line + written, i - written + 1, stream);\n \t\t}\n \t\twritten = i + 1;\n \t}\n@@ -210,7 +224,7 @@ static unsigned ws_check_emit_1(const char *line, int len, unsigned ws_rule,\n \t\tresult |= WS_INDENT_WITH_NON_TAB;\n \t\tif (stream) {\n \t\t\tfputs(ws, stream);\n-\t\t\tfwrite(line + written, i - written, 1, stream);\n+\t\t\tws_emit(expand_tabs, line + written, i - written, stream);\n \t\t\tfputs(reset, stream);\n \t\t}\n \t\twritten = i;\n@@ -225,16 +239,16 @@ static unsigned ws_check_emit_1(const char *line, int len, unsigned ws_rule,\n \t\t/* Emit non-highlighted (middle) segment. */\n \t\tif (trailing_whitespace - written > 0) {\n \t\t\tfputs(set, stream);\n-\t\t\tfwrite(line + written,\n-\t\t\t    trailing_whitespace - written, 1, stream);\n+\t\t\tws_emit(expand_tabs, line + written,\n+\t\t\t    trailing_whitespace - written, stream);\n \t\t\tfputs(reset, stream);\n \t\t}\n \n \t\t/* Highlight errors in trailing whitespace. */\n \t\tif (trailing_whitespace != len) {\n \t\t\tfputs(ws, stream);\n-\t\t\tfwrite(line + trailing_whitespace,\n-\t\t\t    len - trailing_whitespace, 1, stream);\n+\t\t\tws_emit(expand_tabs, line + trailing_whitespace,\n+\t\t\t    len - trailing_whitespace, stream);\n \t\t\tfputs(reset, stream);\n \t\t}\n \t\tif (trailing_carriage_return)\n@@ -247,14 +261,14 @@ static unsigned ws_check_emit_1(const char *line, int len, unsigned ws_rule,\n \n void ws_check_emit(const char *line, int len, unsigned ws_rule,\n \t\t   FILE *stream, const char *set,\n-\t\t   const char *reset, const char *ws)\n+\t\t   const char *reset, const char *ws, int expand_tabs)\n {\n-\t(void)ws_check_emit_1(line, len, ws_rule, stream, set, reset, ws);\n+\t(void)ws_check_emit_1(line, len, ws_rule, stream, set, reset, ws, expand_tabs);\n }\n \n unsigned ws_check(const char *line, int len, unsigned ws_rule)\n {\n-\treturn ws_check_emit_1(line, len, ws_rule, NULL, NULL, NULL, NULL);\n+\treturn ws_check_emit_1(line, len, ws_rule, NULL, NULL, NULL, NULL, 0);\n }\n \n int ws_blank_line(const char *line, int len, unsigned ws_rule)\n-- \n2.12.2.650.ga248b8c51283\n\n"},{"id":"315588","messageId":"xmqqk279cjfs.fsf@gitster.mtv.corp.google.com","threadId":"45526","inReplyTo":"20170328122209.4861-2-jacob.e.keller@intel.com","subject":"Re: [PATCH RFC 2/2] diff: teach diff to expand tabs in output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-28T19:03:51Z","receivedAt":"2017-03-28T19:04:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller <jacob.e.keller@intel.com> writes:\n\n> I'm really not a fan of how the ws code ended up. It seems pretty ugly\n> and weird to hack in the expand_tabs stuff here. However, I'm really not\n> sure how else I could handle this. Additionally, I'm not 100% sure\n> whether this interacts with format-patch or other machinery which may\n> well want some way to be excluded. Thoughts?\n\nAs long as you do the same as \"do we color the output?  no, no, we\nare format-patch and must not color\" logic to refrain from expanding\nthe tabs, you should be OK.\n\n> I think there also may be some wonky bits when performing the tab\n> expansion during whitespace checks, due to the way we expand, because I\n> don't think that the tabexpand function takes into account the \"current\"\n> location when adding a string, so it very well may not be correct.... I\n> am unsure if there is a good way to fix this.\n\nThis \"feature\" is limited to the diff output, so one way may be to\nleave the code as-is and pipe the output to a filter that is similar\nto /usr/bin/expand but knows that the first column is special (this\nis the part that \"this is limited to diff\" kicks in).  You may even\nbe able to implement it as a new option to \"expand(1)\" and then\npeople who aren't Git users would also benefit.\n\n"},{"id":"315618","messageId":"CA+P7+xqA9ZOfaeVg0wjgXVs=N=OJUZjjY1pahUn2xbvSQohfUQ@mail.gmail.com","threadId":"45526","inReplyTo":"xmqqk279cjfs.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH RFC 2/2] diff: teach diff to expand tabs in output","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2017-03-28T20:05:40Z","receivedAt":"2017-03-28T20:06:17Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Tue, Mar 28, 2017 at 12:03 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Jacob Keller <jacob.e.keller@intel.com> writes:\n>\n>> I'm really not a fan of how the ws code ended up. It seems pretty ugly\n>> and weird to hack in the expand_tabs stuff here. However, I'm really not\n>> sure how else I could handle this. Additionally, I'm not 100% sure\n>> whether this interacts with format-patch or other machinery which may\n>> well want some way to be excluded. Thoughts?\n>\n> As long as you do the same as \"do we color the output?  no, no, we\n> are format-patch and must not color\" logic to refrain from expanding\n> the tabs, you should be OK.\n>\n>> I think there also may be some wonky bits when performing the tab\n>> expansion during whitespace checks, due to the way we expand, because I\n>> don't think that the tabexpand function takes into account the \"current\"\n>> location when adding a string, so it very well may not be correct.... I\n>> am unsure if there is a good way to fix this.\n>\n> This \"feature\" is limited to the diff output, so one way may be to\n> leave the code as-is and pipe the output to a filter that is similar\n> to /usr/bin/expand but knows that the first column is special (this\n> is the part that \"this is limited to diff\" kicks in).  You may even\n> be able to implement it as a new option to \"expand(1)\" and then\n> people who aren't Git users would also benefit.\n>\n\nThat makes more sense. I'll take a look at that. It might even be\npossible to modify the pager setup so that it does that as part of its\nprocess.\n\nThanks,\nJake\n"},{"id":"315619","messageId":"20170328203814.36aiocj53wtfcjz5@sigill.intra.peff.net","threadId":"45526","inReplyTo":"CA+P7+xqA9ZOfaeVg0wjgXVs=N=OJUZjjY1pahUn2xbvSQohfUQ@mail.gmail.com","subject":"Re: [PATCH RFC 2/2] diff: teach diff to expand tabs in output","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-28T20:38:14Z","receivedAt":"2017-03-28T20:38:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 28, 2017 at 01:05:40PM -0700, Jacob Keller wrote:\n\n> On Tue, Mar 28, 2017 at 12:03 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> > Jacob Keller <jacob.e.keller@intel.com> writes:\n> >\n> >> I'm really not a fan of how the ws code ended up. It seems pretty ugly\n> >> and weird to hack in the expand_tabs stuff here. However, I'm really not\n> >> sure how else I could handle this. Additionally, I'm not 100% sure\n> >> whether this interacts with format-patch or other machinery which may\n> >> well want some way to be excluded. Thoughts?\n> >\n> > As long as you do the same as \"do we color the output?  no, no, we\n> > are format-patch and must not color\" logic to refrain from expanding\n> > the tabs, you should be OK.\n> >\n> >> I think there also may be some wonky bits when performing the tab\n> >> expansion during whitespace checks, due to the way we expand, because I\n> >> don't think that the tabexpand function takes into account the \"current\"\n> >> location when adding a string, so it very well may not be correct.... I\n> >> am unsure if there is a good way to fix this.\n> >\n> > This \"feature\" is limited to the diff output, so one way may be to\n> > leave the code as-is and pipe the output to a filter that is similar\n> > to /usr/bin/expand but knows that the first column is special (this\n> > is the part that \"this is limited to diff\" kicks in).  You may even\n> > be able to implement it as a new option to \"expand(1)\" and then\n> > people who aren't Git users would also benefit.\n> >\n> \n> That makes more sense. I'll take a look at that. It might even be\n> possible to modify the pager setup so that it does that as part of its\n> process.\n\nThis is similar to how I use diff-highlight, which is basically:\n\n  [pager]\n  log = diff-highlight | less\n  show = diff-highlight | less\n  diff = diff-highlight | less\n\nI've wanted to move diff-highlight inside git, but ran into ugly\nconflicts with the whitespace-marking code as well. Something like:\n\n  git show b16a991c1be5681b4b673d4343dfcc0c2f5ad498 |\n  perl -pe 's/^(.)(\\t+)/$1 . (\" \" x (length($2) * 8))/e'\n\nBut sticking it in your pager pipeline is tough, because you actually\nneed to skip over the color bytes when they are present. You can see in\ndiff-highlight how this is handled.\n\nThat also means an option to something like \"expand\" is tough, because\n\"first character\" really means \"first non-ANSI-code character\".\n\n-Peff\n"},{"id":"315620","messageId":"xmqqbmslcep0.fsf@gitster.mtv.corp.google.com","threadId":"45526","inReplyTo":"20170328203814.36aiocj53wtfcjz5@sigill.intra.peff.net","subject":"Re: [PATCH RFC 2/2] diff: teach diff to expand tabs in output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-28T20:46:19Z","receivedAt":"2017-03-28T20:46:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> That also means an option to something like \"expand\" is tough, because\n> \"first character\" really means \"first non-ANSI-code character\".\n\nThat is true, but once you commit to the mindset that you are\nextending \"expand\", then that becomes a mere part of what must be\ndone, i.e. if you want your \"expand\" to be able to handle coloured\ninput, you'd need to know how wide each segment of the input is.\nFor that matter, you would also want your \"expand\" to pay attention\nto the differences between display- vs byte-widths of a string,\nperhaps reusing utf8_strwidth() or something.\n\nAlso \"the first character is special\" does not have to be a \"diff\nspecific hack\".  Your extended \"expand\" can take a list of\ntab-widths and the special case for highlighting diff output happens\nto take 9,8 as the \"list of tab-widths\" parameter (whose semantics\nis that each number in the list tells how wide the tab is, and the\nlast number repeats forever, so 9,8 really means 9,8,8,8,8,...).\n"}]}