Closed
Bug 955405
Opened 10 years ago
Closed 10 years ago
CTCP quoting is broken
Categories
(Chat Core :: IRC, defect)
Chat Core
IRC
Tracking
(Not tracked)
RESOLVED
FIXED
1.5
People
(Reporter: clokep, Assigned: clokep)
Details
Attachments
(1 file, 1 obsolete file)
3.56 KB,
patch
|
aleth
:
review+
nhnt11
:
review+
|
Details | Diff | Splinter Review |
*** Original post on bio 1968 at 2013-05-22 19:51:00 UTC *** *** Due to BzAPI limitations, the initial description is in comment 1 ***
Assignee | ||
Comment 1•10 years ago
|
||
*** Original post on bio 1968 as attmnt 2445 at 2013-05-22 19:51:00 UTC *** let input = "this \\is \\\\a \\\\\\cool \\\\\\\\string also this: \x01"; print(input) // The current Instantbird code. const highQuote = {"\\": "\\", "\x01": "a"}; const highRegex = new RegExp(Object.keys(highQuote).join("|"), "g"); print(highRegex) let output = input.replace(highRegex, function(aChar) "\x5C" + highQuote[aChar]); print(output) // New code. const pattern = /\\|\x01/g; print(pattern) let outputNew = input.replace(pattern, function(aChar) "\x5C" + highQuote[aChar]); print(outputNew) /* * Output below: 3: this \is \\a \\\cool \\\\string also this: 9: /\|/g 12: this \is \\a \\\cool \\\\string also this: 16: /\\|\x01/g 19: this \\is \\\\a \\\\\\cool \\\\\\\\string also this: \a */ We should add tests to this.
Attachment #8354212 -
Flags: review?(aleth)
Assignee | ||
Comment 2•10 years ago
|
||
*** Original post on bio 1968 as attmnt 2451 at 2013-05-23 03:11:00 UTC *** I added tests and simplified the code a bit.
Attachment #8354218 -
Flags: review?(aleth)
Assignee | ||
Comment 3•10 years ago
|
||
Comment on attachment 8354212 [details] [diff] [review] Don't generate the regexp automatically, which requires escaping twice *** Original change on bio 1968 attmnt 2445 at 2013-05-23 03:11:31 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354212 -
Attachment is obsolete: true
Attachment #8354212 -
Flags: review?(aleth)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → clokep
Status: NEW → ASSIGNED
Comment 4•10 years ago
|
||
Comment on attachment 8354218 [details] [diff] [review] Patch with tests *** Original change on bio 1968 attmnt 2451 at 2013-05-23 13:19:58 UTC *** + "\\\\\\test", + "te\\\\\\st", + "test\\\\\\", These are probably unnecessary. Additionally I think we should have tests for multiple \x01's in a row, like "\x01\x01test" (this goes for dequoting as well, bug 955399 (bio 1962))
Attachment #8354218 -
Flags: review-
Comment 5•10 years ago
|
||
Comment on attachment 8354218 [details] [diff] [review] Patch with tests *** Original change on bio 1968 attmnt 2451 at 2013-05-24 13:30:46 UTC *** Quite a subtle double-escaping bug! Fix looks fine to me. I don't think those additional tests are a problem, even if they are maybe unnecessary. I don't have a strong opinion on whether multiple /x01 tests are needed here.
Attachment #8354218 -
Flags: review?(aleth) → review+
Updated•10 years ago
|
Whiteboard: [checkin-needed]
Assignee | ||
Comment 6•10 years ago
|
||
*** Original post on bio 1968 at 2013-05-24 21:37:47 UTC *** (In reply to comment #2) > Comment on attachment 8354218 [details] [diff] [review] (bio-attmnt 2451) [details] > Patch with tests > > + "\\\\\\test", > + "te\\\\\\st", > + "test\\\\\\", > > These are probably unnecessary. I like having these tests here for completeness. > Additionally I think we should have tests for multiple \x01's in a row, like > "\x01\x01test" (this goes for dequoting as well, bug 955399 (bio 1962)) I'm not really sure we need these, I like the multiple backslash tests because of the escaping issues with backslashes. If you insist we can add them.
Comment 7•10 years ago
|
||
Comment on attachment 8354218 [details] [diff] [review] Patch with tests *** Original change on bio 1968 attmnt 2451 at 2013-05-24 22:31:51 UTC *** Okay. I can't see any other problems so r+.
Attachment #8354218 -
Flags: review- → review+
Assignee | ||
Comment 8•10 years ago
|
||
*** Original post on bio 1968 at 2013-05-24 22:42:37 UTC *** http://hg.instantbird.org/instantbird/rev/079a63e7c003
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [checkin-needed]
Target Milestone: --- → 1.5
You need to log in
before you can comment on or make changes to this bug.
Description
•