GCLI should not auto-unescape command line escapes

RESOLVED FIXED in Future

Status

defect
P2
normal
RESOLVED FIXED
7 years ago
9 months ago

People

(Reporter: jwalker, Assigned: jwalker)

Tracking

unspecified
Future
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

>> pref set foo.dir c:\nice
Priority: -- → P2
Target Milestone: --- → Future
Assignee: nobody → ravicat2013
Status: NEW → ASSIGNED
hii joe
 i have a small doubt. GCLI should not bother about any of the escape characters is it ??. 
or should it consider escape characters like "\r" and ignore "\\r"....
Thanks
Thanks for this.
I'll take a look at this, but I need to test it out and play with it for a while, and consider the wider effects of the change, which could take me a while.

A good place to look for good things to work on is this list: https://bugzilla.mozilla.org/buglist.cgi?cmdtype=runnamed;namedcmd=GCLI%20Good%20First%20Bugs;list_id=4220155

Thanks.
 the link u have given is broken .... can u pls provide it again 
thanks
Duplicate of this bug: 721911
New component triage. Filter on "Lobster Thermidor aux crevettes with a Mornay sauce"
Component: Developer Tools: Console → Developer Tools: Graphic Commandline and Toolbar
Attachment #656787 - Flags: review?(jwalker)
Blocks tools srcdir to work on Windows.
Blocks: 867109
Posted patch patch v0.2 (obsolete) — Splinter Review
Turns out that we were converting to real values of \t or \f too early. Now I check if there is no \\ before f before converting to real, and similarly for other values too.

But there is still no way to do this: ( and I am unable to think of such a way)
1) Let there be a command X with 2 arguments .
2) Pass the first argument as 'a\b\c\' and second argument as 'xyz' (anything), both without quotes
Basically, you would have to do :
>> X a\\b\\c\\ xyz
but this would be considered as a single argument of 'a\b\c xyz'
Attachment #656787 - Attachment is obsolete: true
Attachment #748473 - Flags: review?(jwalker)
Posted patch v0.3 (obsolete) — Splinter Review
Attachment #748473 - Attachment is obsolete: true
Attachment #748473 - Flags: review?(jwalker)
Attachment #748766 - Flags: feedback?(scrapmachines)
From IRC:
11:20 jwalker: there are 2 distinct reasons for escaping:
11:20 jwalker: 1. allow typing untypable chars (\t, \n, etc)
11:20 jwalker: 2. preventing gcli from treating chars like ', ", [space], {, } as special
11:21 jwalker: so the tokenizer needs to think about 2, but not 1
11:21 jwalker: 1 should be handled by the type system
11:21 jwalker: just as we convert "42" to 42 when the type is a number
11:21 jwalker: so we should convert "\t" to a TAB char when the type is a string
This patch also tweaks some tests that had wrongly ordered is params. For now, we only need to think about the changes to gcli.jsm
Comment on attachment 748766 [details] [diff] [review]
v0.3

Review of attachment 748766 [details] [diff] [review]:
-----------------------------------------------------------------

So I made an echo command and tried it (http://pastebin.mozilla.org/2400500)

Here are few inputs that I tried, followed by actual and expected:

>> echo a\\ b c
Actual output : a\\,b,c
Expected: a\,b,c

>> echo a\ b c d
Actual output: as soon as I finish typing 'a\ ', gcli converts it to 'a ' and when I am done typing 'd', gcli just says that extra argument provided and does not do anything on enter.
Expected: a b,c,d

When you said that gcli should take the input as is, shouldn't escaping characters still be valid ? like when I do alert("a\\ b") , the alert popup says "a\ b" (without quotes) . Shouldn't the same apply here ?
Attachment #748766 - Flags: feedback?(scrapmachines)
Posted patch v4 (obsolete) — Splinter Review
Updated to fix the problems you found, plus I've added unit tests for your issues.
Assignee: ravicat2013 → jwalker
Attachment #748766 - Attachment is obsolete: true
Attachment #749164 - Flags: feedback?(scrapmachines)
Comment on attachment 749164 [details] [diff] [review]
v4

Review of attachment 749164 [details] [diff] [review]:
-----------------------------------------------------------------

It fixes the problem I mentioned ("a\", "b", "c") vs ("a b", "c", "d") but it created a new issue :

using the same echo command in http://pastebin.mozilla.org/2403647

try:

>> echo abc\\ndef asd asd
Expected output: [abc\ndef, asd, asd]
Actual output:
[abc
 def, asd, asd]

Basically, while it does not converts \\x to its real value while typing (where x = r, f, n, etc). It first makes \\ into \ and then renders \n as new line character.
Attachment #749164 - Flags: feedback?(scrapmachines)
Posted patch v5 (obsolete) — Splinter Review
Good spot on the double decoding - I've added a test for that case. How does it work now?
Attachment #749164 - Attachment is obsolete: true
Attachment #750412 - Flags: feedback?(scrapmachines)
Comment on attachment 750412 [details] [diff] [review]
v5

Adding harth as a reviewer, but she'd probably like to know Optimizers thoughts on this in her review.
Attachment #750412 - Flags: review?(fayearthur)
My bad for delaying. I will get to this within 2 hours.
Comment on attachment 750412 [details] [diff] [review]
v5

Review of attachment 750412 [details] [diff] [review]:
-----------------------------------------------------------------

Everything works fine now, except for this little minor case, that I could not see why it fails :

>> echo f:\\fx-team\\ a\ b\ m\\ a\ b\\
Output: f:\fx-team,a b m\,a b\
Expected : f:\fx-team\,a b m\,a b\

I am confused as to why the second and the third argument did not get the  character. Everything is same. :|
f+ with that fixed.
Attachment #750412 - Flags: feedback?(scrapmachines)
Posted patch v6Splinter Review
Fixes the latest problem, which was due to a missing /g

Has had f+ from Optimizer.
Attachment #750412 - Attachment is obsolete: true
Attachment #750412 - Flags: review?(fayearthur)
Attachment #751385 - Flags: review?(fayearthur)
Attachment #751385 - Flags: review?(fayearthur) → review+
https://hg.mozilla.org/mozilla-central/rev/e353f92a63bd
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]

Updated

Last year
Product: Firefox → DevTools

Updated

9 months ago
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.