[AESH-456] Append the content to the file with name tmp|file results to 'tmp\|file' being created #282

MMarus · 2018-02-14T19:01:27Z

AESH: https://issues.jboss.org/browse/AESH-456
WFCORE: https://issues.jboss.org/browse/WFCORE-3554

MMarus · 2018-02-14T19:15:49Z

src/main/java/org/aesh/command/impl/Executions.java

-                        config.setArgument(pl.line().trim());
+                        ParsedLineIterator iterator = pl.iterator();
+                        String word = iterator.hasNextWord() ? iterator.pollWord() : "";
+                        config.setArgument(word);


pl.line() did contain unparsed content of the line. Therefore I used the first word from the ParsedLine and the rest is ignored (same behaviour can be observed in bash while running "echo version >> tmp|file anotherWord".
Do you think this is good practise or it would be better to iterate over all the words and use all of them as an argument separated by white space?

@jfdenise what do you think? The argument should only be one word afaik. It should be sufficient to call pl.lastWord() i think.

I have looked at the bash operator, actually it can be located anywhere in the command.
For ex: > foo.txt echo bar or echo > foo.txt bar are identical.
aesh syntax doesn't support this and I think that we shouldn't this is not at all intuitive.
In this case we should get the next world and ignore the following words as it is suggested.

@MMarus , you should log a bug against aesh, link the WFCORE-3554 issue to the aesh one and have the commit message to reference the aesh issue. It will be simpler to track aesh component upgrade.
Thanks.

thanks @jfdenise for the insight. let's just use the first word then. @MMarus can you change the pr to just get the first word? then I think it looks ok.

stalep

Hi, thanks for the pr. I just went on PTO, but I'll try to review it asap.

jfdenise · 2018-02-16T14:25:28Z

src/main/java/org/aesh/parser/LineParser.java

                handleCurlyEnd(c);
            }
            else if (haveEscape) {
-                builder.append(BACK_SLASH);


I think that this change impacts goes further than the fix. Any \ character contained in non quoted text is removed by the parser. I don't think tha tthis is what we want. My understanding is as follow:

Use \ to escape a space character. In this case the \ character is removed but no new word is created.

Any other usages, the \ character is kept.

The reported problem shows that we need to offer a way to escape operators too and have the \ be removed from the parsed content in this case.

So I think that the fix should be narrower, something like:
else if (haveEscape) {
//Escaping an operator?
if (!isQuoted()
&& (currentOperator = matchesOperators(operators, text, index)) != OperatorType.NONE) {
// Do not add the , that was a way to escape an operator.
} else {
builder.append(BACK_SLASH);
}
builder.append(c);
haveEscape = false;
}

ah, i missed this. good catch @jfdenise, yes we need this change.

In this method there are no operators, so I just did undu the deletion of the backslash insertion.

jfdenise · 2018-02-16T14:26:09Z

src/main/java/org/aesh/parser/LineParser.java

                    handleCurlyEnd(c);
                }
                else if (haveEscape) {
-                    builder.append(BACK_SLASH);


Same comment. I would love ;-), to have the 2 parsing operations be merged in order to avoid to duplicate fixes.

we should probably look at trying to refactor that method so it could be inlined...

@MMarus , I will try to spend some time (once your fixed is merged) to refactor the 2 methods.

I have incorporated the changes you suggested into the PR. However, I didn't refactor the 2 methods in order to keep the PR cleaner. I am not sure why the github ignores changes I made here and still requests a change.

jfdenise · 2018-02-19T09:46:59Z

@MMarus , would you mind to squash the 2 commits, this makes review simpler. Thank-you.

…to 'tmp\|file' being created

MMarus · 2018-02-19T09:50:46Z

Yes, of course it's not a problem :). Thank You for your reviews.

jfdenise · 2018-02-19T10:00:10Z

src/main/java/org/aesh/parser/LineParser.java

                    handleCurlyEnd(c);
                }
                else if (haveEscape) {
-                    builder.append(BACK_SLASH);


Seems that the \ shouldn't be removed.

As far as I understand the previous comments the addition of \ should be replaced by the lines 201-207. Which add the \ when it is not escaping an operator:

if (!isQuoted() && (currentOperator = matchesOperators(operators, text, index)) != OperatorType.NONE) { // Do not add the \ that was a way to escape an operator. } else { builder.append(BACK_SLASH); }

Otherwise the \ would be inserted 2 times in case it is not escaping an operator.

@MMarus , I read too quickly and thought that it was the other parse method. Forget about this comment.

stalep · 2018-02-20T05:23:46Z

thanks!

MMarus commented Feb 14, 2018

View reviewed changes

stalep reviewed Feb 15, 2018

View reviewed changes

jfdenise requested changes Feb 16, 2018

View reviewed changes

MMarus changed the title ~~[WFCORE-3554] Append the content to the file with name tmp|file results to 'tmp\|file' being created~~ [AESH-456] Append the content to the file with name tmp|file results to 'tmp\|file' being created Feb 17, 2018

MMarus force-pushed the WFCORE-3554-fix-handling-of-escaped-characters branch from 6a37d48 to d57fbb6 Compare February 19, 2018 09:33

[AESH-456] Append the content to the file with name tmp|file results …

1848f65

…to 'tmp\|file' being created

MMarus force-pushed the WFCORE-3554-fix-handling-of-escaped-characters branch from d57fbb6 to 1848f65 Compare February 19, 2018 09:48

jfdenise reviewed Feb 19, 2018

View reviewed changes

stalep merged commit 60e1e0d into aeshell:master Feb 20, 2018

[AESH-456] Append the content to the file with name tmp|file results to 'tmp\|file' being created #282

[AESH-456] Append the content to the file with name tmp|file results to 'tmp\|file' being created #282

Uh oh!

Conversation

MMarus commented Feb 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stalep left a comment

Choose a reason for hiding this comment

Uh oh!

jfdenise Feb 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MMarus Feb 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jfdenise commented Feb 19, 2018

Uh oh!

MMarus commented Feb 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MMarus Feb 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stalep commented Feb 20, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

MMarus commented Feb 14, 2018 •

edited

Loading

jfdenise Feb 16, 2018 •

edited

Loading

MMarus Feb 19, 2018 •

edited

Loading

MMarus commented Feb 19, 2018 •

edited

Loading

MMarus Feb 19, 2018 •

edited

Loading