Re: RFR: 8331535: Incorrect prompt for Console.readLine [v6]
On Fri, 10 May 2024 15:32:07 GMT, Jan Lahoda wrote:
>> When JLine reads a line, there may be a prompt provided. However, JLine will
>> not interpret the prompt literally, it will handle `%` specially. As a
>> consequence, doing:
>>
>> System.console().readLine("%%s");
>>
>>
>> will not print `%s`, as first `String.format` is used, which will convert
>> `%%s` to `%s`, and then JLine will interpret the `%`. The proposed solution
>> is to duplicate the `%`, so that JLine will print it.
>
> Jan Lahoda has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Update test/jdk/java/io/Console/ConsolePromptTest.java
>
> Co-authored-by: Adam Sotona <10807609+asot...@users.noreply.github.com>
Looks good to me.
-
Marked as reviewed by asotona (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/19081#pullrequestreview-2050327444
Re: RFR: 8331535: Incorrect prompt for Console.readLine [v6]
> When JLine reads a line, there may be a prompt provided. However, JLine will
> not interpret the prompt literally, it will handle `%` specially. As a
> consequence, doing:
>
> System.console().readLine("%%s");
>
>
> will not print `%s`, as first `String.format` is used, which will convert
> `%%s` to `%s`, and then JLine will interpret the `%`. The proposed solution
> is to duplicate the `%`, so that JLine will print it.
Jan Lahoda has updated the pull request incrementally with one additional
commit since the last revision:
Update test/jdk/java/io/Console/ConsolePromptTest.java
Co-authored-by: Adam Sotona <10807609+asot...@users.noreply.github.com>
-
Changes:
- all: https://git.openjdk.org/jdk/pull/19081/files
- new: https://git.openjdk.org/jdk/pull/19081/files/da84810f..441bdad8
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=19081=05
- incr: https://webrevs.openjdk.org/?repo=jdk=19081=04-05
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
Patch: https://git.openjdk.org/jdk/pull/19081.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/19081/head:pull/19081
PR: https://git.openjdk.org/jdk/pull/19081
Re: RFR: 8331535: Incorrect prompt for Console.readLine [v5]
On Fri, 10 May 2024 11:00:55 GMT, Jan Lahoda wrote:
>> When JLine reads a line, there may be a prompt provided. However, JLine will
>> not interpret the prompt literally, it will handle `%` specially. As a
>> consequence, doing:
>>
>> System.console().readLine("%%s");
>>
>>
>> will not print `%s`, as first `String.format` is used, which will convert
>> `%%s` to `%s`, and then JLine will interpret the `%`. The proposed solution
>> is to duplicate the `%`, so that JLine will print it.
>
> Jan Lahoda has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fixing the '%' prompt problem for JShell tools' console.
test/jdk/java/io/Console/ConsolePromptTest.java line 30:
> 28: * @library /test/lib
> 29: * @run main/othervm --limit-modules java.base ConsolePromptTest
> 30: * @run main/othervm -Djdk.console=java.base java.base ConsolePromptTest
Suggestion:
* @run main/othervm -Djdk.console=java.base ConsolePromptTest
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19081#discussion_r1596875588
Re: RFR: 8331535: Incorrect prompt for Console.readLine [v5]
> When JLine reads a line, there may be a prompt provided. However, JLine will
> not interpret the prompt literally, it will handle `%` specially. As a
> consequence, doing:
>
> System.console().readLine("%%s");
>
>
> will not print `%s`, as first `String.format` is used, which will convert
> `%%s` to `%s`, and then JLine will interpret the `%`. The proposed solution
> is to duplicate the `%`, so that JLine will print it.
Jan Lahoda has updated the pull request incrementally with one additional
commit since the last revision:
Fixing the '%' prompt problem for JShell tools' console.
-
Changes:
- all: https://git.openjdk.org/jdk/pull/19081/files
- new: https://git.openjdk.org/jdk/pull/19081/files/a138981e..da84810f
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=19081=04
- incr: https://webrevs.openjdk.org/?repo=jdk=19081=03-04
Stats: 85 lines in 3 files changed: 82 ins; 0 del; 3 mod
Patch: https://git.openjdk.org/jdk/pull/19081.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/19081/head:pull/19081
PR: https://git.openjdk.org/jdk/pull/19081
Re: RFR: 8331535: Incorrect prompt for Console.readLine [v4]
On Tue, 7 May 2024 07:13:23 GMT, Jan Lahoda wrote:
>> When JLine reads a line, there may be a prompt provided. However, JLine will
>> not interpret the prompt literally, it will handle `%` specially. As a
>> consequence, doing:
>>
>> System.console().readLine("%%s");
>>
>>
>> will not print `%s`, as first `String.format` is used, which will convert
>> `%%s` to `%s`, and then JLine will interpret the `%`. The proposed solution
>> is to duplicate the `%`, so that JLine will print it.
>
> Jan Lahoda has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Adding another test run with -Djdk.console=java.base, as suggested.
Jan, if you integrate it soon, I'll improve testing in
https://github.com/openjdk/jdk/pull/19112. Thanks.
-
PR Comment: https://git.openjdk.org/jdk/pull/19081#issuecomment-2102463081
Re: RFR: 8331535: Incorrect prompt for Console.readLine [v4]
> When JLine reads a line, there may be a prompt provided. However, JLine will
> not interpret the prompt literally, it will handle `%` specially. As a
> consequence, doing:
>
> System.console().readLine("%%s");
>
>
> will not print `%s`, as first `String.format` is used, which will convert
> `%%s` to `%s`, and then JLine will interpret the `%`. The proposed solution
> is to duplicate the `%`, so that JLine will print it.
Jan Lahoda has updated the pull request incrementally with one additional
commit since the last revision:
Adding another test run with -Djdk.console=java.base, as suggested.
-
Changes:
- all: https://git.openjdk.org/jdk/pull/19081/files
- new: https://git.openjdk.org/jdk/pull/19081/files/05592871..a138981e
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=19081=03
- incr: https://webrevs.openjdk.org/?repo=jdk=19081=02-03
Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod
Patch: https://git.openjdk.org/jdk/pull/19081.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/19081/head:pull/19081
PR: https://git.openjdk.org/jdk/pull/19081
Re: RFR: 8331535: Incorrect prompt for Console.readLine [v2]
On Mon, 6 May 2024 16:59:40 GMT, Naoto Sato wrote: > For the latter, since you are already at it, I just reassigned the bug to you > @lahodaj, since a test for 8331681 is added to this PR, I'd suggest you do this: `/issue add 8331681`. - PR Review Comment: https://git.openjdk.org/jdk/pull/19081#discussion_r1591423527
Re: RFR: 8331535: Incorrect prompt for Console.readLine [v3]
On Mon, 6 May 2024 16:09:19 GMT, Jan Lahoda wrote:
>> When JLine reads a line, there may be a prompt provided. However, JLine will
>> not interpret the prompt literally, it will handle `%` specially. As a
>> consequence, doing:
>>
>> System.console().readLine("%%s");
>>
>>
>> will not print `%s`, as first `String.format` is used, which will convert
>> `%%s` to `%s`, and then JLine will interpret the `%`. The proposed solution
>> is to duplicate the `%`, so that JLine will print it.
>
> Jan Lahoda has updated the pull request with a new target base due to a merge
> or a rebase. The pull request now contains eight commits:
>
> - Adjusting test as suggested.
> - Merge branch 'master' into JDK-8331535
> - 8331535: Incorrect prompt for Console.readLine
> - Fixing test.
> - Attempting to stabilize the test.
> - Improving test to really test the redirect while stdin is connected to a
> terminal.
> - Fixing typo.
> - 8330998: System.console() writes to stderr when stdout is redirected
Looks good
test/jdk/java/io/Console/ConsolePromptTest.java line 29:
> 27: * @summary Verify the java.base's console provider handles the prompt
> correctly.
> 28: * @library /test/lib
> 29: * @run main/othervm --limit-modules java.base ConsolePromptTest
It would be helpful if we do another run with `-Djdk.console=java.base` too.
-
Marked as reviewed by naoto (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/19081#pullrequestreview-2041252866
PR Review Comment: https://git.openjdk.org/jdk/pull/19081#discussion_r1591300205
Re: RFR: 8331535: Incorrect prompt for Console.readLine [v2]
On Mon, 6 May 2024 16:05:58 GMT, Jan Lahoda wrote: >>> We have a test that checks if `System.console()` returns the correct >>> Console (or null) from the expected module >>> (`test/jdk/java/io/Console/ModuleSelectionTest.java`) >>> >> >> Good; then here we should indeed specify `-Djdk.console=jdk.internal.le`. >> Initially, I suggested an alternative wording (i.e. "default"), but that's >> inappropriate. After all, the test in question resides in >> `test/jdk/jdk/internal/jline`. So it only makes sense that it tests that >> concrete implementation. >> >>> Yeah, that would be helpful. >>> >> >> I filed a bug: https://bugs.openjdk.org/browse/JDK-8331681 > > Thanks. I've added `-Djdk.console=jdk.internal.le`, and also added a test for > the `java.base`'s console: `test/jdk/java/io/Console/ConsolePromptTest.java`. > I can easily remove the latter if this is not a reasonable form. Or I can fix > it as needed. Please let me know. > > Thanks. For the latter, since you are already at it, I just reassigned the bug to you - PR Review Comment: https://git.openjdk.org/jdk/pull/19081#discussion_r1591302360
Re: RFR: 8331535: Incorrect prompt for Console.readLine [v3]
> When JLine reads a line, there may be a prompt provided. However, JLine will
> not interpret the prompt literally, it will handle `%` specially. As a
> consequence, doing:
>
> System.console().readLine("%%s");
>
>
> will not print `%s`, as first `String.format` is used, which will convert
> `%%s` to `%s`, and then JLine will interpret the `%`. The proposed solution
> is to duplicate the `%`, so that JLine will print it.
Jan Lahoda has updated the pull request with a new target base due to a merge
or a rebase. The pull request now contains eight commits:
- Adjusting test as suggested.
- Merge branch 'master' into JDK-8331535
- 8331535: Incorrect prompt for Console.readLine
- Fixing test.
- Attempting to stabilize the test.
- Improving test to really test the redirect while stdin is connected to a
terminal.
- Fixing typo.
- 8330998: System.console() writes to stderr when stdout is redirected
-
Changes: https://git.openjdk.org/jdk/pull/19081/files
Webrev: https://webrevs.openjdk.org/?repo=jdk=19081=02
Stats: 210 lines in 3 files changed: 208 ins; 0 del; 2 mod
Patch: https://git.openjdk.org/jdk/pull/19081.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/19081/head:pull/19081
PR: https://git.openjdk.org/jdk/pull/19081
Re: RFR: 8331535: Incorrect prompt for Console.readLine [v2]
On Fri, 3 May 2024 21:59:40 GMT, Pavel Rappo wrote: >>> Ideally, we should have separate tests that make sure that jdk.internal.le >>> is the default impl. >> >> We have a test that checks if `System.console()` returns the correct Console >> (or null) from the expected module >> (`test/jdk/java/io/Console/ModuleSelectionTest.java`) >> >>> We should also test this behaviour (i.e. % interpretation) on >>> jdk.internal.io Console impl. >> >> Yeah, that would be helpful. > >> We have a test that checks if `System.console()` returns the correct Console >> (or null) from the expected module >> (`test/jdk/java/io/Console/ModuleSelectionTest.java`) >> > > Good; then here we should indeed specify `-Djdk.console=jdk.internal.le`. > Initially, I suggested an alternative wording (i.e. "default"), but that's > inappropriate. After all, the test in question resides in > `test/jdk/jdk/internal/jline`. So it only makes sense that it tests that > concrete implementation. > >> Yeah, that would be helpful. >> > > I filed a bug: https://bugs.openjdk.org/browse/JDK-8331681 Thanks. I've added `-Djdk.console=jdk.internal.le`, and also added a test for the `java.base`'s console: `test/jdk/java/io/Console/ConsolePromptTest.java`. I can easily remove the latter if this is not a reasonable form. Or I can fix it as needed. Please let me know. Thanks. - PR Review Comment: https://git.openjdk.org/jdk/pull/19081#discussion_r1591228737
Re: RFR: 8331535: Incorrect prompt for Console.readLine [v2]
On Fri, 3 May 2024 11:20:52 GMT, Pavel Rappo wrote:
>> Jan Lahoda has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase.
>
> src/jdk.internal.le/share/classes/jdk/internal/org/jline/JdkConsoleProviderImpl.java
> line 101:
>
>> 99: try {
>> 100: initJLineIfNeeded();
>> 101: return jline.readLine(fmt.formatted(args).replace("%",
>> "%%"));
>
> I understand that [JLine interprets `%` in a
> prompt](https://github.com/openjdk/jdk/blob/4ed38f5ad5f822ab948257ed39717ea919fd32ed/src/jdk.internal.le/share/classes/jdk/internal/org/jline/reader/impl/LineReaderImpl.java#L4050),
> but are the interpretation rules documented on JLine GitHub page or
> elsewhere?
There's a description in the javadoc:
https://www.javadoc.io/doc/org.jline/jline/latest/org/jline/reader/LineReader.html
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19081#discussion_r1591196087
Re: RFR: 8331535: Incorrect prompt for Console.readLine [v2]
> When JLine reads a line, there may be a prompt provided. However, JLine will
> not interpret the prompt literally, it will handle `%` specially. As a
> consequence, doing:
>
> System.console().readLine("%%s");
>
>
> will not print `%s`, as first `String.format` is used, which will convert
> `%%s` to `%s`, and then JLine will interpret the `%`. The proposed solution
> is to duplicate the `%`, so that JLine will print it.
Jan Lahoda has updated the pull request with a new target base due to a merge
or a rebase. The incremental webrev excludes the unrelated changes brought in
by the merge/rebase.
-
Changes:
- all: https://git.openjdk.org/jdk/pull/19081/files
- new: https://git.openjdk.org/jdk/pull/19081/files/58cbd42e..58cbd42e
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=19081=01
- incr: https://webrevs.openjdk.org/?repo=jdk=19081=00-01
Stats: 0 lines in 0 files changed: 0 ins; 0 del; 0 mod
Patch: https://git.openjdk.org/jdk/pull/19081.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/19081/head:pull/19081
PR: https://git.openjdk.org/jdk/pull/19081
Re: RFR: 8331535: Incorrect prompt for Console.readLine
On Fri, 3 May 2024 19:02:41 GMT, Naoto Sato wrote: > We have a test that checks if `System.console()` returns the correct Console > (or null) from the expected module > (`test/jdk/java/io/Console/ModuleSelectionTest.java`) > Good; then here we should indeed specify `-Djdk.console=jdk.internal.le`. Initially, I suggested an alternative wording (i.e. "default"), but that's inappropriate. After all, the test in question resides in `test/jdk/jdk/internal/jline`. So it only makes sense that it tests that concrete implementation. > Yeah, that would be helpful. > I filed a bug: https://bugs.openjdk.org/browse/JDK-8331681 - PR Review Comment: https://git.openjdk.org/jdk/pull/19081#discussion_r1589756489
Re: RFR: 8331535: Incorrect prompt for Console.readLine
On Fri, 3 May 2024 18:52:12 GMT, Pavel Rappo wrote: > Ideally, we should have separate tests that make sure that jdk.internal.le is > the default impl. We have a test that checks if `System.console()` returns the correct Console (or null) from the expected module (`test/jdk/java/io/Console/ModuleSelectionTest.java`) > We should also test this behaviour (i.e. % interpretation) on jdk.internal.io > Console impl. Yeah, that would be helpful. - PR Review Comment: https://git.openjdk.org/jdk/pull/19081#discussion_r1589629655
Re: RFR: 8331535: Incorrect prompt for Console.readLine
On Fri, 3 May 2024 17:07:59 GMT, Naoto Sato wrote: > Or maybe we could explicitly specify `-Djdk.console=jdk.internal.le` to the > test. We could do that, or we could replace `jdk.internal.le` with "default" in the summary. Ideally, we should have separate tests that make sure that jdk.internal.le is the default impl. We should also test this behaviour (i.e. % interpretation) on jdk.internal.io Console impl. What do you think? - PR Review Comment: https://git.openjdk.org/jdk/pull/19081#discussion_r1589620826
Re: RFR: 8331535: Incorrect prompt for Console.readLine
On Fri, 3 May 2024 11:12:48 GMT, Pavel Rappo wrote:
>> When JLine reads a line, there may be a prompt provided. However, JLine will
>> not interpret the prompt literally, it will handle `%` specially. As a
>> consequence, doing:
>>
>> System.console().readLine("%%s");
>>
>>
>> will not print `%s`, as first `String.format` is used, which will convert
>> `%%s` to `%s`, and then JLine will interpret the `%`. The proposed solution
>> is to duplicate the `%`, so that JLine will print it.
>
> test/jdk/jdk/internal/jline/JLineConsoleProviderTest.java line 27:
>
>> 25: * @test
>> 26: * @bug 8331535
>> 27: * @summary Verify the jdk.internal.le's console provider works properly.
>
> There's a hidden assumption in this test below that the _default_ console is
> `jdk.internal.le`. Is there any way to assert that?
Or maybe we could explicitly specify `-Djdk.console=jdk.internal.le` to the
test.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19081#discussion_r158945
Re: RFR: 8331535: Incorrect prompt for Console.readLine
On Fri, 3 May 2024 10:11:02 GMT, Jan Lahoda wrote:
> When JLine reads a line, there may be a prompt provided. However, JLine will
> not interpret the prompt literally, it will handle `%` specially. As a
> consequence, doing:
>
> System.console().readLine("%%s");
>
>
> will not print `%s`, as first `String.format` is used, which will convert
> `%%s` to `%s`, and then JLine will interpret the `%`. The proposed solution
> is to duplicate the `%`, so that JLine will print it.
src/jdk.internal.le/share/classes/jdk/internal/org/jline/JdkConsoleProviderImpl.java
line 101:
> 99: try {
> 100: initJLineIfNeeded();
> 101: return jline.readLine(fmt.formatted(args).replace("%",
> "%%"));
I understand that [JLine interprets `%` in a
prompt](https://github.com/openjdk/jdk/blob/4ed38f5ad5f822ab948257ed39717ea919fd32ed/src/jdk.internal.le/share/classes/jdk/internal/org/jline/reader/impl/LineReaderImpl.java#L4050),
but are the interpretation rules documented on JLine GitHub page or elsewhere?
src/jdk.internal.le/share/classes/jdk/internal/org/jline/JdkConsoleProviderImpl.java
line 116:
> 114: try {
> 115: initJLineIfNeeded();
> 116: return jline.readLine(fmt.formatted(args).replace("%",
> "%%"), '\0')
I find `'\0'` more cryptic and confusing than `(char) 0`, but okay. (Had to
re-read
https://docs.oracle.com/javase/specs/jls/se22/html/jls-3.html#jls-EscapeSequence)
test/jdk/jdk/internal/jline/JLineConsoleProviderTest.java line 27:
> 25: * @test
> 26: * @bug 8331535
> 27: * @summary Verify the jdk.internal.le's console provider works properly.
There's a hidden assumption in this test below that the _default_ console is
`jdk.internal.le`. Is there any way to assert that?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19081#discussion_r1589074603
PR Review Comment: https://git.openjdk.org/jdk/pull/19081#discussion_r1589065719
PR Review Comment: https://git.openjdk.org/jdk/pull/19081#discussion_r1589067927
