MIME-Version: 1.0
References: <16108-134692e97146b7bc@postgresql.org>
 <CA+hUKG+Em5CO27qk4PC9syK+p2n0a0ULP_Vm3OqpvdXGQTn0=A@mail.gmail.com>
 <CAC+AXB2LDsO_vB-yNW8b9VBW=onGPwJiUuxAQ23TDcm8JbJ8NQ@mail.gmail.com>
 <43AA0560298D1A4893896FDC52840EA1D9C74F7D@G08CNEXMBPEKD03.g08.fujitsu.local>
 <CAC+AXB1yZXb_=DzRoPV8jY0cUX5EFYLHtmOgu7is3d3M31GppQ@mail.gmail.com>
 <CANtu0oj2z_t9d5SrTLqgZ_mOoiiAheTFp3spP7urUFiuK90=uA@mail.gmail.com>
 <CAC+AXB3cTZKR8ry-T6-ui0qLMij+K_auxO9Lq9dHy1DBj+HDHQ@mail.gmail.com>
 <CANtu0oic8bkGZ6uSA6NPp=dKCwpBd22yk2tJmUKCX5PVVAzMDA@mail.gmail.com>
In-Reply-To: 
 <CANtu0oic8bkGZ6uSA6NPp=dKCwpBd22yk2tJmUKCX5PVVAzMDA@mail.gmail.com>
From: =?UTF-8?Q?Juan_Jos=C3=A9_Santamar=C3=ADa_Flecha?=
 <juanjo.santamaria@gmail.com>
Date: Mon, 24 Feb 2020 18:56:05 +0100
Message-ID: 
 <CAC+AXB0LSzuE7veUrw0-N=cxKEV_hfZTT_RaPMfMeRY3co-RQA@mail.gmail.com>
Subject: Re: BUG #16108: Colorization to the output of command-line has
 unproperly behaviors at Windows platform
To: Michail Nikolaev <michail.nikolaev@gmail.com>
Cc: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>,
	"Tang, Haiying" <tanghy.fnst@cn.fujitsu.com>,
 Thomas Munro <thomas.munro@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: multipart/mixed; boundary="000000000000835717059f5616fa"
Precedence: bulk

--000000000000835717059f5616fa
Content-Type: multipart/alternative; boundary="000000000000835714059f5616f8"

--000000000000835714059f5616f8
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Sat, Feb 22, 2020 at 9:09 PM Michail Nikolaev <michail.nikolaev@gmail.co=
m>
wrote:

>
> I am not sure it is good idea to mix both patches because it adds some
> confusion and makes it harder to merge each.
> Maybe is it better to update current patch the way to reuse some
> function later in [1]?
>

The patch was originaly reported for Windows, but looking into Peter's
patch, I think this issue affects other systems unless we use stricter
logic to detect a colorable terminal when using the "auto" option.
Probably, the way to go is leaving this patch as WIN32 only and thinking
about a future patch.


> Also, regarding comment
> > It is disabled by default, so it must be enabled to use color outpout.
>
> It is not true for new terminal, for example. Maybe it is better to
> rephrase it to something like: "Check if TV100 support if enabled and
> attempt to enable if not".
>

The logic I have seen on new terminals is that VT100 is supported but
disabled. Would you find clearer? "Attempt to enable VT100 sequence
processing. If it is not possible consider it as unsupported."

Please find attached a patch addressing these comments.

Regards,

Juan Jos=C3=A9 Santamar=C3=ADa Flecha

--000000000000835714059f5616f8
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr"><div dir=3D"ltr"><br></div><br><div class=3D"gmail_quote">=
<div dir=3D"ltr" class=3D"gmail_attr">On Sat, Feb 22, 2020 at 9:09 PM Micha=
il Nikolaev &lt;<a href=3D"mailto:michail.nikolaev@gmail.com" target=3D"_bl=
ank">michail.nikolaev@gmail.com</a>&gt; wrote:<br></div><blockquote class=
=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rg=
b(204,204,204);padding-left:1ex"><br>
I am not sure it is good idea to mix both patches because it adds some<br>
confusion and makes it harder to merge each.<br>
Maybe is it better to update current patch the way to reuse some<br>
function later in [1]?<br></blockquote><div><br></div><div>The patch was or=
iginaly reported for Windows, but looking into Peter&#39;s patch, I think t=
his issue affects other systems unless we use stricter logic to detect a co=
lorable terminal when using the &quot;auto&quot; option. Probably, the way =
to go is leaving this patch as WIN32 only and thinking about a future patch=
.</div><div>=C2=A0</div><blockquote class=3D"gmail_quote" style=3D"margin:0=
px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Also, regarding comment<br>
&gt; It is disabled by default, so it must be enabled to use color outpout.=
<br>
<br>
It is not true for new terminal, for example. Maybe it is better to<br>
rephrase it to something like: &quot;Check if TV100 support if enabled and<=
br>
attempt to enable if not&quot;.<br></blockquote><div><br></div><div>The log=
ic I have seen on new terminals is that VT100 is supported but disabled. Wo=
uld you find clearer? &quot;Attempt to enable VT100 sequence processing. If=
 it is not possible consider it as unsupported.&quot;<br></div><div><br></d=
iv><div>Please find attached a patch addressing these comments.</div><div><=
br></div><div>Regards,</div><div><br></div><div>Juan Jos=C3=A9 Santamar=C3=
=ADa Flecha</div></div></div>

--000000000000835714059f5616f8--

--000000000000835717059f5616fa
Content-Type: application/octet-stream;
	name="v4-0001-command-line-colorization-on-windows.patch"
Content-Disposition: attachment;
	filename="v4-0001-command-line-colorization-on-windows.patch"
Content-Transfer-Encoding: base64
Content-ID: <f_k70recdt0>
X-Attachment-Id: f_k70recdt0

ZGlmZiAtLWdpdCBhL3NyYy9jb21tb24vbG9nZ2luZy5jIGIvc3JjL2NvbW1vbi9sb2dnaW5nLmMK
aW5kZXggYzc4YWU3OS4uZDU5ZWM4OCAxMDA2NDQKLS0tIGEvc3JjL2NvbW1vbi9sb2dnaW5nLmMK
KysrIGIvc3JjL2NvbW1vbi9sb2dnaW5nLmMKQEAgLTMyLDYgKzMyLDM1IEBAIHN0YXRpYyBjb25z
dCBjaGFyICpzZ3JfbG9jdXMgPSBOVUxMOwogI2RlZmluZSBBTlNJX0VTQ0FQRV9GTVQgIlx4MWJb
JXNtIgogI2RlZmluZSBBTlNJX0VTQ0FQRV9SRVNFVCAiXHgxYlswbSIKIAorI2lmZGVmIFdJTjMy
CisjaWZuZGVmIEVOQUJMRV9WSVJUVUFMX1RFUk1JTkFMX1BST0NFU1NJTkcKKyNkZWZpbmUgRU5B
QkxFX1ZJUlRVQUxfVEVSTUlOQUxfUFJPQ0VTU0lORyAweDAwMDQKKyNlbmRpZgorCisvKgorICog
QXR0ZW1wdCB0byBlbmFibGUgVlQxMDAgc2VxdWVuY2UgcHJvY2Vzc2luZy4KKyAqIElmIGl0IGlz
IG5vdCBwb3NzaWJsZSBjb25zaWRlciBpdCBhcyB1bnN1cHBvcnRlZC4KKyAqLworc3RhdGljIGJv
b2wKK2VuYWJsZV92dF9wcm9jZXNzaW5nKHZvaWQpCit7CisJLyogQ2hlY2sgc3RkZXJyICovCisJ
SEFORExFIGhPdXQgPSBHZXRTdGRIYW5kbGUoU1REX0VSUk9SX0hBTkRMRSk7CisJRFdPUkQgZHdN
b2RlID0gMDsKKworCWlmIChoT3V0ID09IElOVkFMSURfSEFORExFX1ZBTFVFKQorCQlyZXR1cm4g
ZmFsc2U7CisJaWYgKCFHZXRDb25zb2xlTW9kZShoT3V0LCAmZHdNb2RlKSkKKwkJcmV0dXJuIGZh
bHNlOworCWlmIChkd01vZGUgJiBFTkFCTEVfVklSVFVBTF9URVJNSU5BTF9QUk9DRVNTSU5HKQor
CQlyZXR1cm4gdHJ1ZTsKKwlkd01vZGUgfD0gRU5BQkxFX1ZJUlRVQUxfVEVSTUlOQUxfUFJPQ0VT
U0lORzsKKwlpZiAoIVNldENvbnNvbGVNb2RlKGhPdXQsIGR3TW9kZSkpCisJCXJldHVybiBmYWxz
ZTsKKwlyZXR1cm4gdHJ1ZTsKK30KKyNlbmRpZgkvKiBXSU4zMiAqLworCiAvKgogICogVGhpcyBz
aG91bGQgYmUgY2FsbGVkIGJlZm9yZSBhbnkgb3V0cHV0IGhhcHBlbnMuCiAgKi8KQEAgLTQwLDYg
KzY5LDEwIEBAIHBnX2xvZ2dpbmdfaW5pdChjb25zdCBjaGFyICphcmd2MCkKIHsKIAljb25zdCBj
aGFyICpwZ19jb2xvcl9lbnYgPSBnZXRlbnYoIlBHX0NPTE9SIik7CiAJYm9vbAkJbG9nX2NvbG9y
ID0gZmFsc2U7CisJYm9vbAkJY29sb3JfdGVybWluYWwgPSBpc2F0dHkoZmlsZW5vKHN0ZGVycikp
OworI2lmZGVmIFdJTjMyCisJY29sb3JfdGVybWluYWwgPSBjb2xvcl90ZXJtaW5hbCAmJiBlbmFi
bGVfdnRfcHJvY2Vzc2luZygpOworI2VuZGlmCiAKIAkvKiB1c3VhbGx5IHRoZSBkZWZhdWx0LCBi
dXQgbm90IG9uIFdpbmRvd3MgKi8KIAlzZXR2YnVmKHN0ZGVyciwgTlVMTCwgX0lPTkJGLCAwKTsK
QEAgLTUwLDcgKzgzLDcgQEAgcGdfbG9nZ2luZ19pbml0KGNvbnN0IGNoYXIgKmFyZ3YwKQogCWlm
IChwZ19jb2xvcl9lbnYpCiAJewogCQlpZiAoc3RyY21wKHBnX2NvbG9yX2VudiwgImFsd2F5cyIp
ID09IDAgfHwKLQkJCShzdHJjbXAocGdfY29sb3JfZW52LCAiYXV0byIpID09IDAgJiYgaXNhdHR5
KGZpbGVubyhzdGRlcnIpKSkpCisJCQkoc3RyY21wKHBnX2NvbG9yX2VudiwgImF1dG8iKSA9PSAw
ICYmIGNvbG9yX3Rlcm1pbmFsKSkKIAkJCWxvZ19jb2xvciA9IHRydWU7CiAJfQogCg==
--000000000000835717059f5616fa--