RFR: 8261692: Bugs in clhsdb history support

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: 8261692: Bugs in clhsdb history support

Chris Plummer-2
See the CR for a description of the 3 issues fixed. There's also a new test added. I'll explain the fix related to quoting the '!' here since it's not that obvious how it works.

There's a complex pattern called `historyPattern` that is used to find various uses of ! to do expansion of commands and arguments in the history

    static Pattern historyPattern = Pattern.compile("([\\\]?)((!\*)|(!\$)|(!!-?)|(!-?[0-9][0-9]*)|(![a-zA-Z][^ ]*))");

I added the `([\\\]?)` part to handle allowing a '!' to be quoted with a backslash so it will just be treated as a '!' instead of being used for history expansion. This part of the pattern basically says match 1 backslash if present. Yes, you need 4 backslashes to do this. javac will reduce it to two, and then the pattern compilation reduces it down to just one backslash (not to be used for quoting).

The above pattern for searching for the backslash before a '!' is in the first pattern grouping, and the part that matches on the '!' and what follows is in the 2nd pattern grouping. Thus the code I added checks if the first pattern grouping matches something (and at most it will match just one backslash), and if so then we eat it and don't treat the '!' that follows as a history expansion:

                    if (m.group(1).length() != 0) {
                        // This means we matched a `` before the '!'. Don't do any history
                        // expansion in this case. Just capture what matched after the ``.
                        result.append(m.group(2));
                        continue;
                    }

-------------

Commit messages:
 - Fix 3 bugs in the clhsdb history support.

Changes: https://git.openjdk.java.net/jdk/pull/2565/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2565&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8261692
  Stats: 118 lines in 2 files changed: 114 ins; 0 del; 4 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2565.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2565/head:pull/2565

PR: https://git.openjdk.java.net/jdk/pull/2565
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261692: Bugs in clhsdb history support

Alex Menkov-3
On Sun, 14 Feb 2021 08:45:26 GMT, Chris Plummer <[hidden email]> wrote:

> See the CR for a description of the 3 issues fixed. There's also a new test added. I'll explain the fix related to quoting the '!' here since it's not that obvious how it works.
>
> There's a complex pattern called `historyPattern` that is used to find various uses of ! to do expansion of commands and arguments in the history
>
>     static Pattern historyPattern = Pattern.compile("([\\\]?)((!\*)|(!\$)|(!!-?)|(!-?[0-9][0-9]*)|(![a-zA-Z][^ ]*))");
>
> I added the `([\\\]?)` part to handle allowing a '!' to be quoted with a backslash so it will just be treated as a '!' instead of being used for history expansion. This part of the pattern basically says match 1 backslash if present. Yes, you need 4 backslashes to do this. javac will reduce it to two, and then the pattern compilation reduces it down to just one backslash (not to be used for quoting).
>
> The above pattern for searching for the backslash before a '!' is in the first pattern grouping, and the part that matches on the '!' and what follows is in the 2nd pattern grouping. Thus the code I added checks if the first pattern grouping matches something (and at most it will match just one backslash), and if so then we eat it and don't treat the '!' that follows as a history expansion:
>
>                     if (m.group(1).length() != 0) {
>                         // This means we matched a `` before the '!'. Don't do any history
>                         // expansion in this case. Just capture what matched after the ``.
>                         result.append(m.group(2));
>                         continue;
>                     }

Marked as reviewed by amenkov (Reviewer).

-------------

PR: https://git.openjdk.java.net/jdk/pull/2565
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261692: Bugs in clhsdb history support

Serguei Spitsyn-2
In reply to this post by Chris Plummer-2
On Sun, 14 Feb 2021 08:45:26 GMT, Chris Plummer <[hidden email]> wrote:

> See the CR for a description of the 3 issues fixed. There's also a new test added. I'll explain the fix related to quoting the '!' here since it's not that obvious how it works.
>
> There's a complex pattern called `historyPattern` that is used to find various uses of ! to do expansion of commands and arguments in the history
>
>     static Pattern historyPattern = Pattern.compile("([\\\]?)((!\*)|(!\$)|(!!-?)|(!-?[0-9][0-9]*)|(![a-zA-Z][^ ]*))");
>
> I added the `([\\\]?)` part to handle allowing a '!' to be quoted with a backslash so it will just be treated as a '!' instead of being used for history expansion. This part of the pattern basically says match 1 backslash if present. Yes, you need 4 backslashes to do this. javac will reduce it to two, and then the pattern compilation reduces it down to just one backslash (not to be used for quoting).
>
> The above pattern for searching for the backslash before a '!' is in the first pattern grouping, and the part that matches on the '!' and what follows is in the 2nd pattern grouping. Thus the code I added checks if the first pattern grouping matches something (and at most it will match just one backslash), and if so then we eat it and don't treat the '!' that follows as a history expansion:
>
>                     if (m.group(1).length() != 0) {
>                         // This means we matched a `` before the '!'. Don't do any history
>                         // expansion in this case. Just capture what matched after the ``.
>                         result.append(m.group(2));
>                         continue;
>                     }

Looks good.

-------------

Marked as reviewed by sspitsyn (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2565
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261692: Bugs in clhsdb history support

Chris Plummer-2
On Sat, 20 Feb 2021 10:17:35 GMT, Serguei Spitsyn <[hidden email]> wrote:

>> See the CR for a description of the 3 issues fixed. There's also a new test added. I'll explain the fix related to quoting the '!' here since it's not that obvious how it works.
>>
>> There's a complex pattern called `historyPattern` that is used to find various uses of ! to do expansion of commands and arguments in the history
>>
>>     static Pattern historyPattern = Pattern.compile("([\\\]?)((!\*)|(!\$)|(!!-?)|(!-?[0-9][0-9]*)|(![a-zA-Z][^ ]*))");
>>
>> I added the `([\\\]?)` part to handle allowing a '!' to be quoted with a backslash so it will just be treated as a '!' instead of being used for history expansion. This part of the pattern basically says match 1 backslash if present. Yes, you need 4 backslashes to do this. javac will reduce it to two, and then the pattern compilation reduces it down to just one backslash (not to be used for quoting).
>>
>> The above pattern for searching for the backslash before a '!' is in the first pattern grouping, and the part that matches on the '!' and what follows is in the 2nd pattern grouping. Thus the code I added checks if the first pattern grouping matches something (and at most it will match just one backslash), and if so then we eat it and don't treat the '!' that follows as a history expansion:
>>
>>                     if (m.group(1).length() != 0) {
>>                         // This means we matched a `` before the '!'. Don't do any history
>>                         // expansion in this case. Just capture what matched after the ``.
>>                         result.append(m.group(2));
>>                         continue;
>>                     }
>
> Looks good.

Thanks for the reviews!

-------------

PR: https://git.openjdk.java.net/jdk/pull/2565
Reply | Threaded
Open this post in threaded view
|

Integrated: 8261692: Bugs in clhsdb history support

Chris Plummer-2
In reply to this post by Chris Plummer-2
On Sun, 14 Feb 2021 08:45:26 GMT, Chris Plummer <[hidden email]> wrote:

> See the CR for a description of the 3 issues fixed. There's also a new test added. I'll explain the fix related to quoting the '!' here since it's not that obvious how it works.
>
> There's a complex pattern called `historyPattern` that is used to find various uses of ! to do expansion of commands and arguments in the history
>
>     static Pattern historyPattern = Pattern.compile("([\\\]?)((!\*)|(!\$)|(!!-?)|(!-?[0-9][0-9]*)|(![a-zA-Z][^ ]*))");
>
> I added the `([\\\]?)` part to handle allowing a '!' to be quoted with a backslash so it will just be treated as a '!' instead of being used for history expansion. This part of the pattern basically says match 1 backslash if present. Yes, you need 4 backslashes to do this. javac will reduce it to two, and then the pattern compilation reduces it down to just one backslash (not to be used for quoting).
>
> The above pattern for searching for the backslash before a '!' is in the first pattern grouping, and the part that matches on the '!' and what follows is in the 2nd pattern grouping. Thus the code I added checks if the first pattern grouping matches something (and at most it will match just one backslash), and if so then we eat it and don't treat the '!' that follows as a history expansion:
>
>                     if (m.group(1).length() != 0) {
>                         // This means we matched a `` before the '!'. Don't do any history
>                         // expansion in this case. Just capture what matched after the ``.
>                         result.append(m.group(2));
>                         continue;
>                     }

This pull request has now been integrated.

Changeset: 18188c2a
Author:    Chris Plummer <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/18188c2a
Stats:     117 lines in 2 files changed: 114 ins; 0 del; 3 mod

8261692: Bugs in clhsdb history support

Reviewed-by: amenkov, sspitsyn

-------------

PR: https://git.openjdk.java.net/jdk/pull/2565