Merge lp:~lifeless/bzr/bug-3918 into lp:bzr/2.0

[Robert Collins]

[Robert Collins (lifeless)]

This avoids adding filenames containing \r or \n to the dirstate via 'bzr add', which prevents a common source of 'wtf' for people with odd trees that they are importing via the command line.

[Andrew Bennetts (spiv)]

Looks sane.

(It would be nice to actually support \r and \n in filenames, but I see that even 2a still barfs on that :( )

review: Approve

[Andrew Bennetts (spiv)]

> Looks sane.

Oh, and I'm not very familiar with the dirstate code, but I assume this change will also catch filenames from sources other than add, e.g. like "bzr mv foo foo\nbar" ?

[Robert Collins (lifeless)]

It won't catch 'mv', because of layers and costs.

so, apply_inventory_delta is the right place to fully catch it, but some
code still does a full inventory serialisation.

So this really only catches the _common_ case - 'bzr add BAD'

-Rob

[Martin Pool (mbp)]

Congratulations on fixing what may be the oldest open bug.

--
Martin <http://launchpad.net/~mbp/>

[Robert Collins (lifeless)]

On Mon, 2009-09-28 at 05:09 +0000, Martin Pool wrote:
> Congratulations on fixing what may be the oldest open bug.

Thanks :). Its not really fixed, just put behind a small wall - but the
wall that /most/ users have been complaining about the lack of. We
should finish making inventory deltas The Way to update dirstate, and
then put the check in apply_inventory_delta, at which point mv and
direct API usage will also be unable to add such paths.

-Rob

[Vincent Ladeuil (vila)]

Good to see that bug caught.

Yet, shouldn't the long term plan *accept* these paths ?

I'm mostly thinking about conversions from foreign VCS here.
We already encouter the problem in such contexts.

So may be we need a way to address the issue by accepting the
path and *renaming* it on the fly in some canonical way that
allows tracking further changes on it ?

[Robert Collins (lifeless)]

On Mon, 2009-09-28 at 08:30 +0000, Vincent Ladeuil wrote:
> Good to see that bug caught.
>
> Yet, shouldn't the long term plan *accept* these paths ?
>
> I'm mostly thinking about conversions from foreign VCS here.
> We already encouter the problem in such contexts.
>
> So may be we need a way to address the issue by accepting the
> path and *renaming* it on the fly in some canonical way that
> allows tracking further changes on it ?

Follow up to the bug please :). I suggest repeating this particular
comment there, as the merge proposal will be invisible to others wanting
to think about this, now that its landed.

-Rob

[John A Meinel (jameinel)]

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

Robert Collins wrote:
> Robert Collins has proposed merging lp:~lifeless/bzr/bug-3918 into lp:bzr/2.0.
>
> Requested reviews:
> bzr-core (bzr-core)
> Related bugs:
> #3918 bzr can be caused to error with filenames containing newlines
> https://bugs.launchpad.net/bugs/3918
>
>
> This avoids adding filenames containing \r or \n to the dirstate via 'bzr add', which prevents a common source of 'wtf' for people with odd trees that they are importing via the command line.
>

I came late and this has already landed, but '\\' is also not allowed
(ATM). I think there might be some other characters. '\t' wasn't
supported in XML, IIRC, it may be supported in dirstate + 2a.

Just thinking if this gives a nice error about what is going wrong and
skipping a bad filename, it would probably be nice to unify it with some
of the other exceptional cases.

John
=:->

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

iEYEARECAAYFAkrAwrwACgkQJdeBCYSNAAOGGgCeJPEolRP+0u3xIUlHffQr59bX
Mh4AoLk9uMqrdGAzUVsAOxJmDosuJKll
=9roN
-----END PGP SIGNATURE-----

=== modified file 'NEWS'
--- NEWS	2009-09-25 21:05:28 +0000
+++ NEWS	2009-09-28 03:00:29 +0000
@@ -2,6 +2,37 @@
 Bazaar Release Notes
 ####################
 
+2.0 series (not released)
+#########################
+
+Compatibility Breaks
+********************
+
+New Features
+************
+
+Bug Fixes
+*********
+
+* ``bzr add`` in a tree that has files with ``\r`` or ``\n`` in the
+  filename will issue a warning and skip over those files.
+  (Robert Collins, #3918)
+
+Improvements
+************
+
+Documentation
+*************
+
+API Changes
+***********
+
+Internals
+*********
+
+Testing
+*******
+
 bzr 2.0.1
 ##########
 
 
=== modified file 'bzrlib/mutabletree.py'
--- bzrlib/mutabletree.py	2009-09-07 23:14:05 +0000
+++ bzrlib/mutabletree.py	2009-09-28 03:00:29 +0000
@@ -23,6 +23,7 @@
 from bzrlib.lazy_import import lazy_import
 lazy_import(globals(), """
 import os
+import re
 
 from bzrlib import (
     add,
@@ -427,6 +428,7 @@
                 dirs_to_add.append((path, None))
             prev_dir = path.raw_path
 
+        illegalpath_re = re.compile(r'[\r\n]')
         # dirs_to_add is initialised to a list of directories, but as we scan
         # directories we append files to it.
         # XXX: We should determine kind of files when we scan them rather than
@@ -443,6 +445,9 @@
             if not InventoryEntry.versionable_kind(kind):
                 warning("skipping %s (can't add file of kind '%s')", abspath, kind)
                 continue
+            if illegalpath_re.search(directory.raw_path):
+                warning("skipping %r (contains \\n or \\r)" % abspath)
+                continue
 
             if parent_ie is not None:
                 versioned = directory.base_path in parent_ie.children
 
=== modified file 'bzrlib/tests/per_workingtree/test_smart_add.py'
--- bzrlib/tests/per_workingtree/test_smart_add.py	2009-07-10 07:14:02 +0000
+++ bzrlib/tests/per_workingtree/test_smart_add.py	2009-09-28 03:00:29 +0000
@@ -51,6 +51,18 @@
         self.assertEqual([('', 'V', 'directory'), ('a', 'V', 'file')],
                          files)
 
+    def assertFilenameSkipped(self, filename):
+        tree = self.make_branch_and_tree('tree')
+        self.build_tree(['tree/'+filename])
+        tree.smart_add(['tree'])
+        self.assertEqual(None, tree.path2id(filename))
+
+    def test_path_containing_newline_skips(self):
+        self.assertFilenameSkipped('a\nb')
+
+    def test_path_containing_carriagereturn_skips(self):
+        self.assertFilenameSkipped('a\rb')
+
     def test_save_false(self):
         """Dry-run add doesn't permanently affect the tree."""
         wt = self.make_branch_and_tree('.')