Merge lp:~doxxx/bzr/572092-ignore-dupes into lp:bzr

[Gordon Tyler]

(doxxx) 'ignore' command avoids putting duplicate entry into .bzrignore

Fixes bug 572092 by changing tree_ignores_add_patterns to check for duplicate patterns in the existing .bzrignore file.

[Alexander Belchenko (bialix)]

Perhaps, reading the file twice from the disk is not best idea, but we should hit disk cache easily. And this operation is not performance critical one.

Thanks!

review: Approve

[Gordon Tyler (doxxx)]

On 5/1/2010 2:12 AM, Alexander Belchenko wrote:
> Perhaps, reading the file twice from the disk is not best idea, but
> we should hit disk cache easily. And this operation is not
> performance critical one.

My initial implementation just read it into the set, updated the set
with the new patterns and then wrote the set back out to disk. The
problem with that was that it changed the order of the patterns in the file.

I could probably read the file once into a StringIO object and then
parse from that...

[Parth Malwankar (parthm)]

> I could probably read the file once into a StringIO object and then
> parse from that...

StringIO sounds like a good idea to me, in the edge case that
the file gets modified between reads the buffer will be
consistent for the program.

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Parth Malwankar wrote:
>> I could probably read the file once into a StringIO object and then
>> parse from that...
>
> StringIO sounds like a good idea to me, in the edge case that
> the file gets modified between reads the buffer will be
> consistent for the program.

Why not read into a list with a set check:

patterns = []
pattern_set = set()

for line in disk_file:
  ignore = XXXX(line)
  if ignore not in pattern_set:
    pattern_set.add(ignore)
    patterns.append(ignore)

return patterns

You get filtering and still retain the ordering. Also note that you can
have comments in the ignore file, which we would also like to preserve.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkvce8kACgkQJdeBCYSNAAMnUwCgqWg1ZSg9E6Fi/DChM6lZbsrL
vDQAoMIJXrt0sbT/YKOCRRjbEoBnYwYZ
=pz71
-----END PGP SIGNATURE-----

[Gordon Tyler (doxxx)]

> Why not read into a list with a set check:
>
> patterns = []
> pattern_set = set()
>
> for line in disk_file:
> ignore = XXXX(line)
> if ignore not in pattern_set:
> pattern_set.add(ignore)
> patterns.append(ignore)
>
> return patterns

I wanted to reuse the parse_ignore_file so that I didn't have to duplicate all the reading and error handling code.

> You get filtering and still retain the ordering. Also note that you can
> have comments in the ignore file, which we would also like to preserve.

Yeah, that was my intention.

[Parth Malwankar (parthm)]

I think assertPatternEquals needs to use sorted list instead
of set for tests to catch duplication error. Apart from that
this looks good to me.

    def assertPatternsEquals(self, patterns):
        self.assertEquals(sorted(patterns),
            sorted(open(".bzrignore", 'r').read().strip().split('\n')))

review: Needs Fixing

[Gordon Tyler (doxxx)]

Good point and done.

[Parth Malwankar (parthm)]

Looks good to me.

review: Approve

[Alexander Belchenko (bialix)]

Usually we importing StringIO from c-module cStringIO

from cStringIO import StringIO

Is there any special reason why you prefer python implementation?

[Gordon Tyler (doxxx)]

On 5/2/2010 3:28 PM, Alexander Belchenko wrote:
> Usually we importing StringIO from c-module cStringIO
>
> from cStringIO import StringIO
>
> Is there any special reason why you prefer python implementation?

This isn't a particular performance-sensitive situation. I figured
StringIO would be okay. I'm assuming there's a cost to import cStringIO?
Otherwise why would the python implementation exist at all?

[Andrew Bennetts (spiv)]

Gordon Tyler wrote:
> On 5/2/2010 3:28 PM, Alexander Belchenko wrote:
> > Usually we importing StringIO from c-module cStringIO
[...]
>
> This isn't a particular performance-sensitive situation. I figured
> StringIO would be okay. I'm assuming there's a cost to import cStringIO?
> Otherwise why would the python implementation exist at all?

Because it's a very old module in the standard library, and people know
better now :)

FWIW, in Python 3, there's no cStringIO any more. There's just
io.StringIO (and io.BytesIO), and they are automatically C
implementations.

[Gordon Tyler (doxxx)]

Well, alrighty then. :) cStringIO it is.

[Parth Malwankar (parthm)]

> Usually we importing StringIO from c-module cStringIO
>
> from cStringIO import StringIO
>
> Is there any special reason why you prefer python implementation?

I don't know the impact in this specific situation but
IIUC StringIO works with unicode cStringIO does not.

From the cStringIO docs[1]:
> Unlike the memory files implemented by the StringIO
> module, those provided by this module are not able
> to accept Unicode strings that cannot be encoded
> as plain ASCII strings.

[1] http://docs.python.org/library/stringio.html#module-cStringIO

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Gordon Tyler wrote:
> Well, alrighty then. :) cStringIO it is.

Well that and because doing stuff like:

cStringIO.StringIO(u'unicode')

will give you a pointer to the *raw* unicode bytes. (So on UCS2
versions, you get '\0u\0n\0i\0c\0o\0d\0e'. And on UCS4 you get
'\0\0\0u\0\0\0n\0\0\0i...')

However, the really good bit is that cStringIO forces it to be in 8-bit
encoding, which is useful when you want to make sure you aren't
switching between 8-bit strings and Unicode strings accidentally.

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkveO1EACgkQJdeBCYSNAAP/tQCgmdZqSFM2m2ANRuyzQGT8a0k8
ke4AmQE4OT/78hejuZfBo4DROcKdxuZ/
=oCIc
-----END PGP SIGNATURE-----

[Gordon Tyler (doxxx)]

On 5/2/2010 10:57 PM, John A Meinel wrote:
> Well that and because doing stuff like:
>
> cStringIO.StringIO(u'unicode')
>
> will give you a pointer to the *raw* unicode bytes. (So on UCS2
> versions, you get '\0u\0n\0i\0c\0o\0d\0e'. And on UCS4 you get
> '\0\0\0u\0\0\0n\0\0\0i...')
>
> However, the really good bit is that cStringIO forces it to be in 8-bit
> encoding, which is useful when you want to make sure you aren't
> switching between 8-bit strings and Unicode strings accidentally.

The new patterns, which may be Unicode strings, are encoded to UTF-8
before being written out to the file.

However, I added some more tests to see how it deals with non-Unix line
endings in the .bzrignore file and it isn't preserving them properly
when writing out the new contents. So back to the drawing board for me.

[Gordon Tyler (doxxx)]

Once Launchpad decides to finish processing my latest push, I believe I have solved the line endings problem and added tests for both that and Unicode handling.

[Gordon Tyler (doxxx)]

Hmmm... Launchpad is still processing my last push after 6 hours. Could somebody poke it with a sharp stick?

[Alexander Belchenko (bialix)]

Gordon Tyler пишет:
> Hmmm... Launchpad is still processing my last push after 6 hours. Could somebody poke it with a sharp stick?

http://twitter.com/launchpadstatus/status/13298943124

[Gordon Tyler (doxxx)]

Okay, looks like it's updated. Give it an eyeball or two?

[Alexander Belchenko (bialix)]

Fine for me.

review: Approve

[Parth Malwankar (parthm)]

Looks good to me.

review: Approve

[Robert Collins (lifeless)]

Launchpad will be slow for another day - its processing all of
maverick (the release after lucid) at the moment. Just a heads up.

[Parth Malwankar (parthm)]

> Launchpad will be slow for another day - its processing all of
> maverick (the release after lucid) at the moment. Just a heads up.

Thanks for the heads up Robert.
I will mark this as approved and ready to land, and try to land it tomorrow (once branch refresh is more responsive) in case no one has any further points to discuss on this mp.

=== modified file 'NEWS'
--- NEWS	2010-05-03 04:08:50 +0000
+++ NEWS	2010-05-04 00:05:38 +0000
@@ -63,6 +63,9 @@
   more comprehensible.
   (Martin Pool, #491763)
 
+* ``bzr ignore`` will no longer add duplicate patterns to .bzrignore.
+  (Gordon Tyler, #572092)
+
 Improvements
 ************
 

=== modified file 'bzrlib/ignores.py'
--- bzrlib/ignores.py	2010-04-30 07:54:33 +0000
+++ bzrlib/ignores.py	2010-05-04 00:05:38 +0000
@@ -17,6 +17,8 @@
 """Lists of ignore files, etc."""
 
 import errno
+import os
+from cStringIO import StringIO
 
 import bzrlib
 from bzrlib import (
@@ -181,28 +183,44 @@
     The ignore file will be automatically added under version control.
 
     :param tree: Working tree to update the ignore list.
+    :param name_pattern_list: List of ignore patterns.
+    :return: None
     """
+    # read in the existing ignores set
     ifn = tree.abspath(bzrlib.IGNORE_FILENAME)
     if tree.has_filename(ifn):
-        f = open(ifn, 'rt')
+        f = open(ifn, 'rU')
         try:
-            igns = f.read().decode('utf-8')
+            file_contents = f.read()
+            # figure out what kind of line endings are used
+            newline = getattr(f, 'newlines', None)
+            if type(newline) is tuple:
+                newline = newline[0]
+            elif newline is None:
+                newline = os.linesep
         finally:
             f.close()
     else:
-        igns = ""
-
-    # TODO: If the file already uses crlf-style termination, maybe
-    # we should use that for the newly added lines?
-
-    if igns and igns[-1] != '\n':
-        igns += '\n'
-    for name_pattern in name_pattern_list:
-        igns += name_pattern + '\n'
-
+        file_contents = ""
+        newline = os.linesep
+    
+    sio = StringIO(file_contents)
+    try:
+        ignores = parse_ignore_file(sio)
+    finally:
+        sio.close()
+    
+    # write out the updated ignores set
     f = atomicfile.AtomicFile(ifn, 'wb')
     try:
-        f.write(igns.encode('utf-8'))
+        # write the original contents, preserving original line endings
+        f.write(newline.join(file_contents.split('\n')))
+        if len(file_contents) > 0 and not file_contents.endswith('\n'):
+            f.write(newline)
+        for pattern in name_pattern_list:
+            if not pattern in ignores:
+                f.write(pattern.encode('utf-8'))
+                f.write(newline)
         f.commit()
     finally:
         f.close()

=== modified file 'bzrlib/tests/test_ignores.py'
--- bzrlib/tests/test_ignores.py	2009-12-26 21:31:56 +0000
+++ bzrlib/tests/test_ignores.py	2010-05-04 00:05:38 +0000
@@ -165,27 +165,55 @@
 
 
 class TestTreeIgnores(TestCaseWithTransport):
+    
+    def assertPatternsEquals(self, patterns):
+        contents = open(".bzrignore", 'rU').read().strip().split('\n')
+        self.assertEquals(sorted(patterns), sorted(contents))
 
     def test_new_file(self):
         tree = self.make_branch_and_tree(".")
         ignores.tree_ignores_add_patterns(tree, ["myentry"])
         self.assertTrue(tree.has_filename(".bzrignore"))
-        self.assertEquals("myentry\n",
-                          open(".bzrignore", 'r').read())
+        self.assertPatternsEquals(["myentry"])
 
     def test_add_to_existing(self):
         tree = self.make_branch_and_tree(".")
         self.build_tree_contents([('.bzrignore', "myentry1\n")])
         tree.add([".bzrignore"])
         ignores.tree_ignores_add_patterns(tree, ["myentry2", "foo"])
-        self.assertEquals("myentry1\nmyentry2\nfoo\n",
-                          open(".bzrignore", 'r').read())
+        self.assertPatternsEquals(["myentry1", "myentry2", "foo"])
 
     def test_adds_ending_newline(self):
         tree = self.make_branch_and_tree(".")
         self.build_tree_contents([('.bzrignore', "myentry1")])
         tree.add([".bzrignore"])
         ignores.tree_ignores_add_patterns(tree, ["myentry2"])
-        self.assertEquals("myentry1\nmyentry2\n",
-                          open(".bzrignore", 'r').read())
-
+        self.assertPatternsEquals(["myentry1", "myentry2"])
+        text = open(".bzrignore", 'r').read()
+        self.assertTrue(text.endswith('\r\n') or
+                        text.endswith('\n') or
+                        text.endswith('\r'))
+
+    def test_does_not_add_dupe(self):
+        tree = self.make_branch_and_tree(".")
+        self.build_tree_contents([('.bzrignore', "myentry\n")])
+        tree.add([".bzrignore"])
+        ignores.tree_ignores_add_patterns(tree, ["myentry"])
+        self.assertPatternsEquals(["myentry"])
+
+    def test_non_ascii(self):
+        tree = self.make_branch_and_tree(".")
+        self.build_tree_contents([('.bzrignore',
+                                   u"myentry\u1234\n".encode('utf-8'))])
+        tree.add([".bzrignore"])
+        ignores.tree_ignores_add_patterns(tree, [u"myentry\u5678"])
+        self.assertPatternsEquals([u"myentry\u1234".encode('utf-8'),
+                                   u"myentry\u5678".encode('utf-8')])
+
+    def test_crlf(self):
+        tree = self.make_branch_and_tree(".")
+        self.build_tree_contents([('.bzrignore', "myentry1\r\n")])
+        tree.add([".bzrignore"])
+        ignores.tree_ignores_add_patterns(tree, ["myentry2", "foo"])
+        self.assertEquals(open('.bzrignore', 'rb').read(), 'myentry1\r\nmyentry2\r\nfoo\r\n')
+        self.assertPatternsEquals(["myentry1", "myentry2", "foo"])