Re: [PATCH/RFC v1 1/1] convert.c: Escape sequences only for a tty in trace_encoding()
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Mar 11, 2019, 02:58 UTC
- Message-ID
- <xmqqo96iktge.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <20190310061914.24554-1-tboegi@web.de>
tboegi@web.de writes:
> I am temped to remove the "dim" functionality all together, > or to remove the printout of the values which are now dimmed, > what do others think ?
I am for removing the color settings we see here for two reasons. One is that the tracing is primarily for machine readability. Another is that if we want to have an option to make the output more human friendly, we should have (a) a facility for users of the tracing mechanism, like the codepath we see here, to mark the parts of its output in a more logical ways (e.g. "here comes a less important piece of info"), (b) a knob to tell the tracing mechanism what kind of output (e.g. "more friendly to humans using color") is requested, and (c) code in the tracing mechanism (e.g. the helper functions implemented at the same level as trace_printf()), and what we have are far from such a structured thing---and then such a new effort for more structured tracing should probably go to the trace2 effort jeffhost is spearheading.
But I am speaking as just one of the reviewers here, and if people feel differently, I would want to hear it first before deciding anything.
Thanks.
Show 47 quoted lines
> convert.c | 20 ++++++++++++++++----
> 1 file changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/convert.c b/convert.c
> index 5d0307fc10..70e58f1413 100644
> --- a/convert.c
> +++ b/convert.c
> @@ -42,6 +42,9 @@ struct text_stat {
> unsigned printable, nonprintable;
> };
>
> +static const char *terminal_half_bright;
> +static const char *terminal_reset_normal;
> +
> static void gather_stats(const char *buf, unsigned long size, struct text_stat *stats)
> {
> unsigned long i;
> @@ -330,14 +333,23 @@ static void trace_encoding(const char *context, const char *path,
> static struct trace_key coe = TRACE_KEY_INIT(WORKING_TREE_ENCODING);
> struct strbuf trace = STRBUF_INIT;
> int i;
> -
> + if (!terminal_half_bright || !terminal_reset_normal) {
> + if (isatty(1)) {
> + terminal_half_bright = "\033[2m";
> + terminal_reset_normal = "\033[0m";
> + } else {
> + terminal_half_bright = "";
> + terminal_reset_normal = "";
> + }
> + }
> strbuf_addf(&trace, "%s (%s, considered %s):\n", context, path, encoding);
> for (i = 0; i < len && buf; ++i) {
> + char c = buf[i] > 32 && buf[i] < 127 ? buf[i] : ' ';
> strbuf_addf(
> - &trace, "| \033[2m%2i:\033[0m %2x \033[2m%c\033[0m%c",
> - i,
> + &trace, "| %s%2i:%s %2x %s%c%s%c",
> + terminal_half_bright, i, terminal_reset_normal,
> (unsigned char) buf[i],
> - (buf[i] > 32 && buf[i] < 127 ? buf[i] : ' '),
> + terminal_half_bright, c, terminal_reset_normal,
> ((i+1) % 8 && (i+1) < len ? ' ' : '\n')
> );
> }
> --
> 2.21.0.135.g6e0cc67761