git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH/RFC v1 1/1] convert.c: Escape sequences only for a tty in trace_encoding()
@ 2019-03-10  6:19 tboegi
  2019-03-11  2:58 ` Junio C Hamano
  0 siblings, 1 reply; 2+ messages in thread
From: tboegi @ 2019-03-10  6:19 UTC (permalink / raw)
  To: git; +Cc: Torsten Bögershausen

From: Torsten Bögershausen <tboegi@web.de>

The content of a buffer can be dumped using trace_encoding()
before and after the encoding is converted.
The current function trace_encoding() in convert.c tries to
make the output easier to read:
The byte position and the character itself are dimmed, allowing
the eye to focus on the hex values in the byte stream.

ANSI escape sequences are used to "dim" the display temporally,
and to restore the normal brightness.

When stdout is re-directed into a file, those sequences are not
working as expected (but shown in the editor) which is disturbing.
rather then helpful.

Disable them, if stdout is not a tty.

Signed-off-by: Torsten Bögershausen <tboegi@web.de>
---
 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 ?

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


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

* Re: [PATCH/RFC v1 1/1] convert.c: Escape sequences only for a tty in trace_encoding()
  2019-03-10  6:19 [PATCH/RFC v1 1/1] convert.c: Escape sequences only for a tty in trace_encoding() tboegi
@ 2019-03-11  2:58 ` Junio C Hamano
  0 siblings, 0 replies; 2+ messages in thread
From: Junio C Hamano @ 2019-03-11  2:58 UTC (permalink / raw)
  To: tboegi; +Cc: git

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.


> 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

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

end of thread, other threads:[~2019-03-11  2:58 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-10  6:19 [PATCH/RFC v1 1/1] convert.c: Escape sequences only for a tty in trace_encoding() tboegi
2019-03-11  2:58 ` Junio C Hamano

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git