Merge lp:~bialix/bzr/2.1-528944-backslashes-strike-back into lp:bzr/2.1

[Alexander Belchenko]

I have failed to create a small change to fix the backslashes problem, so I have forced to rewrite the parser from scratch using 2 substates instead of main _state. Otherwise I have to duplicate backslashes handling in two states.

[Gordon Tyler (doxxx)]

I'm wondering why you didn't just backport the cmdline module and related changes from bzr.dev? As far as I know that should handle backslashes correctly and if it doesn't that's a bug that needs to be fixed.

review: Needs Information

[Alexander Belchenko (bialix)]

Gordon Tyler пишет:
> Review: Needs Information
> I'm wondering why you didn't just backport the cmdline module and related changes from bzr.dev? As far as I know that should handle backslashes correctly and if it doesn't that's a bug that needs to be fixed.

Because this patch explicitly created for 2.1.x series only. Your cmdline module is more like new
feature and new API, while I want only my rug back^W^W^W to fix it for 2.1 series with minimal
changes and no API breaks. Also my code is smaller than your and even smaller than previous
implementation from John although it's now handling more weird cases than 2.1.1 code.

And I'd be happy to get in backport of your tests in addition to ones in my patch, so I can check if
there is more weird cases we want to care about.

[Gordon Tyler (doxxx)]

I see your point. Conversely, it would be nice if you could add your new test cases to bzr.dev's test_cmdline if they aren't already covered.

review: Approve

[Martin Packman (gz)]

Adding the test to trunk as well would indeed be a good.

I'm not really happy about the size of the change here, both me and Alexander were hoping for a simple one or two line diff. The redeeming aspect is that the code is at least well tested, so should be reasonably safe despite the several rewrites it's gone through.

review: Approve

[Alexander Belchenko (bialix)]

John A Meinel пишет:
> The proposal to merge lp:~bialix/bzr/2.1-528944-backslashes-strike-back into lp:bzr/2.1 has been updated.
>
> Status: Needs review => Approved

Please, help me land it.

=== modified file 'bzrlib/tests/test_win32utils.py'
--- bzrlib/tests/test_win32utils.py	2010-02-17 17:11:16 +0000
+++ bzrlib/tests/test_win32utils.py	2010-05-12 16:23:24 +0000
@@ -345,9 +345,29 @@
         self.assertAsTokens([(True, u'foo"bar')], u'"foo\\"bar"')
 
     def test_double_escape(self):
-        self.assertAsTokens([(True, u'foo\\bar')], u'"foo\\\\bar"')
+        self.assertAsTokens([(True, u'foo\\\\bar')], u'"foo\\\\bar"')
         self.assertAsTokens([(False, u'foo\\\\bar')], u"foo\\\\bar")
 
+    def test_n_backslashes_handling(self):
+        # https://bugs.launchpad.net/bzr/+bug/528944
+        # actually we care about the doubled backslashes when they're
+        # represents UNC paths.
+        # But in fact there is too much weird corner cases
+        # (see https://bugs.launchpad.net/tortoisebzr/+bug/569050)
+        # so to reproduce every bit of windows command-line handling
+        # could be not worth of efforts?
+        self.requireFeature(BackslashDirSeparatorFeature)
+        self.assertAsTokens([(True, r'\\host\path')], r'"\\host\path"')
+        self.assertAsTokens([(False, r'\\host\path')], r'\\host\path')
+        # handling of " after the 2n and 2n+1 backslashes
+        # inside and outside the quoted string
+        self.assertAsTokens([(True, r'\\'), (False, r'*.py')], r'"\\\\" *.py')
+        self.assertAsTokens([(True, r'\\" *.py')], r'"\\\\\" *.py"')
+        self.assertAsTokens([(True, r'\\ *.py')], r'\\\\" *.py"')
+        self.assertAsTokens([(False, r'\\"'), (False, r'*.py')],
+                            r'\\\\\" *.py')
+        self.assertAsTokens([(True, u'\\\\')], u'"\\\\')
+
 
 class Test_CommandLineToArgv(tests.TestCaseInTempDir):
 

=== modified file 'bzrlib/win32utils.py'
--- bzrlib/win32utils.py	2010-02-17 17:11:16 +0000
+++ bzrlib/win32utils.py	2010-05-12 16:23:24 +0000
@@ -536,70 +536,49 @@
         # self._quote_match = re.compile(u'[\'"]').match
         self._escape_match = lambda x: None # Never matches
         self._escape = '\\'
-        # State can be
-        #   ' ' - after whitespace, starting a new token
-        #   'a' - after text, currently working on a token
-        #   '"' - after ", currently in a "-delimited quoted section
-        #   "\" - after '\', checking the next char
-        self._state = ' '
         self._token = [] # Current token being parsed
 
     def _get_token(self):
         # Were there quote chars as part of this token?
-        quoted = False
-        quoted_state = None
+        quoted = None   # state:
+                        #  None - the string is not quoted
+                        #  empty string ('') - there was quoted substring
+                        #  double quote (") - we're inside quoted chunk
+        number_of_backslashes = 0
         for nextchar in self._input_iter:
-            if self._state == ' ':
-                if self._whitespace_match(nextchar):
-                    # if self._token: return token
-                    continue
-                elif nextchar in self._quote_chars:
-                    self._state = nextchar # quoted state
-                elif self._word_match(nextchar):
-                    self._token.append(nextchar)
-                    self._state = 'a'
-                else:
-                    raise AssertionError('wtttf?')
-            elif self._state in self._quote_chars:
-                quoted = True
-                if nextchar == self._state: # End of quote
-                    self._state = 'a' # posix allows 'foo'bar to translate to
-                                      # foobar
-                elif self._state == '"' and nextchar == self._escape:
-                    quoted_state = self._state
-                    self._state = nextchar
-                else:
-                    self._token.append(nextchar)
-            elif self._state == self._escape:
-                if nextchar == '\\':
-                    self._token.append('\\')
-                elif nextchar == '"':
-                    self._token.append(nextchar)
-                else:
-                    self._token.append('\\' + nextchar)
-                self._state = quoted_state
-            elif self._state == 'a':
-                if self._whitespace_match(nextchar):
-                    if self._token:
-                        break # emit this token
+            if self._whitespace_match(nextchar):
+                if quoted:
+                    self._token.append(nextchar)
+                elif self._token:
+                    break
+            elif nextchar == '\\':
+                number_of_backslashes += 1
+            elif nextchar in self._quote_chars:
+                if number_of_backslashes:
+                    self._token.append('\\'*(number_of_backslashes/2))
+                    if number_of_backslashes % 2:
+                        self._token.append('"')
                     else:
-                        continue # no token to emit
-                elif nextchar in self._quote_chars:
-                    # Start a new quoted section
-                    self._state = nextchar
-                # escape?
-                elif (self._word_match(nextchar)
-                      or nextchar in self._quote_chars
-                      # or whitespace_split?
-                      ):
-                    self._token.append(nextchar)
+                        if quoted:
+                            quoted = ''
+                        else:
+                            quoted = nextchar
+                    number_of_backslashes = 0
+                elif nextchar == quoted:
+                    # end of quoted string
+                    quoted = ''
                 else:
-                    raise AssertionError('state == "a", char: %r'
-                                         % (nextchar,))
+                    quoted = nextchar
             else:
-                raise AssertionError('unknown state: %r' % (self._state,))
+                if number_of_backslashes:
+                    self._token.append('\\'*number_of_backslashes)
+                    number_of_backslashes = 0
+                self._token.append(nextchar)
+        if number_of_backslashes > 0:
+            self._token.append('\\'*number_of_backslashes)
         result = ''.join(self._token)
         self._token = []
+        quoted = quoted is not None
         if not quoted and result == '':
             result = None
         return quoted, result