{"thread":{"id":"50705","subject":"[PATCH/RFC v1 1/1] convert.c: Escape sequences only for a tty in trace_encoding()","startedAt":"2019-03-10T06:19:28Z","lastAt":"2019-03-11T02:58:46Z","messageCount":2,"participants":["tboegi@web.de","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"371061","messageId":"20190310061914.24554-1-tboegi@web.de","threadId":"50705","inReplyTo":null,"subject":"[PATCH/RFC v1 1/1] convert.c: Escape sequences only for a tty in trace_encoding()","fromName":"","fromEmail":"tboegi@web.de","sentAt":"2019-03-10T06:19:14Z","receivedAt":"2019-03-10T06:19:28Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"From: Torsten Bögershausen <tboegi@web.de>\n\nThe content of a buffer can be dumped using trace_encoding()\nbefore and after the encoding is converted.\nThe current function trace_encoding() in convert.c tries to\nmake the output easier to read:\nThe byte position and the character itself are dimmed, allowing\nthe eye to focus on the hex values in the byte stream.\n\nANSI escape sequences are used to \"dim\" the display temporally,\nand to restore the normal brightness.\n\nWhen stdout is re-directed into a file, those sequences are not\nworking as expected (but shown in the editor) which is disturbing.\nrather then helpful.\n\nDisable them, if stdout is not a tty.\n\nSigned-off-by: Torsten Bögershausen <tboegi@web.de>\n---\n I am temped to remove the \"dim\" functionality all together,\n or to remove the printout of the values which are now dimmed,\n what do others think ?\n\nconvert.c | 20 ++++++++++++++++----\n 1 file changed, 16 insertions(+), 4 deletions(-)\n\ndiff --git a/convert.c b/convert.c\nindex 5d0307fc10..70e58f1413 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -42,6 +42,9 @@ struct text_stat {\n \tunsigned printable, nonprintable;\n };\n\n+static const char *terminal_half_bright;\n+static const char *terminal_reset_normal;\n+\n static void gather_stats(const char *buf, unsigned long size, struct text_stat *stats)\n {\n \tunsigned long i;\n@@ -330,14 +333,23 @@ static void trace_encoding(const char *context, const char *path,\n \tstatic struct trace_key coe = TRACE_KEY_INIT(WORKING_TREE_ENCODING);\n \tstruct strbuf trace = STRBUF_INIT;\n \tint i;\n-\n+\tif (!terminal_half_bright || !terminal_reset_normal) {\n+\t\tif (isatty(1)) {\n+\t\t\tterminal_half_bright  = \"\\033[2m\";\n+\t\t\tterminal_reset_normal = \"\\033[0m\";\n+\t\t} else {\n+\t\t\tterminal_half_bright = \"\";\n+\t\t\tterminal_reset_normal = \"\";\n+\t\t}\n+\t}\n \tstrbuf_addf(&trace, \"%s (%s, considered %s):\\n\", context, path, encoding);\n \tfor (i = 0; i < len && buf; ++i) {\n+\t\tchar c = buf[i] > 32 && buf[i] < 127 ? buf[i] : ' ';\n \t\tstrbuf_addf(\n-\t\t\t&trace, \"| \\033[2m%2i:\\033[0m %2x \\033[2m%c\\033[0m%c\",\n-\t\t\ti,\n+\t\t\t&trace, \"| %s%2i:%s %2x %s%c%s%c\",\n+\t\t\tterminal_half_bright, i, terminal_reset_normal,\n \t\t\t(unsigned char) buf[i],\n-\t\t\t(buf[i] > 32 && buf[i] < 127 ? buf[i] : ' '),\n+\t\t\tterminal_half_bright, c, terminal_reset_normal,\n \t\t\t((i+1) % 8 && (i+1) < len ? ' ' : '\\n')\n \t\t);\n \t}\n--\n2.21.0.135.g6e0cc67761\n\n"},{"id":"371101","messageId":"xmqqo96iktge.fsf@gitster-ct.c.googlers.com","threadId":"50705","inReplyTo":"20190310061914.24554-1-tboegi@web.de","subject":"Re: [PATCH/RFC v1 1/1] convert.c: Escape sequences only for a tty in trace_encoding()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-11T02:58:41Z","receivedAt":"2019-03-11T02:58:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"tboegi@web.de writes:\n\n>  I am temped to remove the \"dim\" functionality all together,\n>  or to remove the printout of the values which are now dimmed,\n>  what do others think ?\n\nI am for removing the color settings we see here for two reasons.\nOne is that the tracing is primarily for machine readability.\nAnother is that if we want to have an option to make the output more\nhuman friendly, we should have (a) a facility for users of the\ntracing mechanism, like the codepath we see here, to mark the parts\nof its output in a more logical ways (e.g. \"here comes a less\nimportant piece of info\"), (b) a knob to tell the tracing mechanism\nwhat kind of output (e.g. \"more friendly to humans using color\") is\nrequested, and (c) code in the tracing mechanism (e.g. the helper\nfunctions implemented at the same level as trace_printf()), and what\nwe have are far from such a structured thing---and then such a new\neffort for more structured tracing should probably go to the trace2\neffort jeffhost is spearheading.\n\nBut I am speaking as just one of the reviewers here, and if people\nfeel differently, I would want to hear it first before deciding\nanything.\n\nThanks.\n\n\n> convert.c | 20 ++++++++++++++++----\n>  1 file changed, 16 insertions(+), 4 deletions(-)\n>\n> diff --git a/convert.c b/convert.c\n> index 5d0307fc10..70e58f1413 100644\n> --- a/convert.c\n> +++ b/convert.c\n> @@ -42,6 +42,9 @@ struct text_stat {\n>  \tunsigned printable, nonprintable;\n>  };\n>\n> +static const char *terminal_half_bright;\n> +static const char *terminal_reset_normal;\n> +\n>  static void gather_stats(const char *buf, unsigned long size, struct text_stat *stats)\n>  {\n>  \tunsigned long i;\n> @@ -330,14 +333,23 @@ static void trace_encoding(const char *context, const char *path,\n>  \tstatic struct trace_key coe = TRACE_KEY_INIT(WORKING_TREE_ENCODING);\n>  \tstruct strbuf trace = STRBUF_INIT;\n>  \tint i;\n> -\n> +\tif (!terminal_half_bright || !terminal_reset_normal) {\n> +\t\tif (isatty(1)) {\n> +\t\t\tterminal_half_bright  = \"\\033[2m\";\n> +\t\t\tterminal_reset_normal = \"\\033[0m\";\n> +\t\t} else {\n> +\t\t\tterminal_half_bright = \"\";\n> +\t\t\tterminal_reset_normal = \"\";\n> +\t\t}\n> +\t}\n>  \tstrbuf_addf(&trace, \"%s (%s, considered %s):\\n\", context, path, encoding);\n>  \tfor (i = 0; i < len && buf; ++i) {\n> +\t\tchar c = buf[i] > 32 && buf[i] < 127 ? buf[i] : ' ';\n>  \t\tstrbuf_addf(\n> -\t\t\t&trace, \"| \\033[2m%2i:\\033[0m %2x \\033[2m%c\\033[0m%c\",\n> -\t\t\ti,\n> +\t\t\t&trace, \"| %s%2i:%s %2x %s%c%s%c\",\n> +\t\t\tterminal_half_bright, i, terminal_reset_normal,\n>  \t\t\t(unsigned char) buf[i],\n> -\t\t\t(buf[i] > 32 && buf[i] < 127 ? buf[i] : ' '),\n> +\t\t\tterminal_half_bright, c, terminal_reset_normal,\n>  \t\t\t((i+1) % 8 && (i+1) < len ? ' ' : '\\n')\n>  \t\t);\n>  \t}\n> --\n> 2.21.0.135.g6e0cc67761\n"}]}