Bazaar

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

  1. bug-438569
  2. Merge into 2.0
Proposed by Robert Collins
Status: Merged
Approved by: John A Meinel
Approved revision: no longer in the source branch.
Merged at revision: not available
Proposed branch: lp:~lifeless/bzr/bug-438569
Merge into: lp:bzr/2.0
Diff against target: 101 lines
5 files modified
NEWS (+3/-0)
bzrlib/_dirstate_helpers_pyx.pyx (+3/-1)
bzrlib/dirstate.py (+3/-1)
bzrlib/inventory.py (+1/-1)
bzrlib/tests/per_intertree/test_compare.py (+34/-0)
To merge this branch: bzr merge lp:~lifeless/bzr/bug-438569
Reviewer Review Type Date Requested Status
John A Meinel Approve
Vincent Ladeuil Approve
Review via email: mp+12641@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Robert Collins (lifeless) wrote :

This makes the error when someone changes a previously versioned file to something we can't handle much cleaner.

Revision history for this message
Vincent Ladeuil (vila) wrote :

Could you raise a KnownFailure so we are reminded that works is needed there ?

review: Approve
Revision history for this message
Robert Collins (lifeless) wrote :

On Wed, 2009-09-30 at 09:56 +0000, Vincent Ladeuil wrote:
> Review: Approve
> Could you raise a KnownFailure so we are reminded that works is needed there ?

I think its behaviour is fine; I'd rather file a bug that says what we'd
like, if we want to go further; I'm not sure there is a need to do that
though.

-Rob

Revision history for this message
John A Meinel (jameinel) wrote :

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

Robert Collins wrote:
> Robert Collins has proposed merging lp:~lifeless/bzr/bug-438569 into lp:bzr/2.0.
>
> Requested reviews:
> bzr-core (bzr-core)
> Related bugs:
> #438569 dirstate cannot handle versioned files that become fifo's.
> https://bugs.launchpad.net/bugs/438569
>
>
> This makes the error when someone changes a previously versioned file to something we can't handle much cleaner.
>

+ try:
+ self.do_iter_changes(tree1, tree2)
+ except errors.BzrError:
+ self.assertRaises(errors.BadFileKindError,
self.do_iter_changes,
+ tree1, tree2)
+

^- wouldn't that be better as:

# it is okay for this either to succeed, or to fail with
# BadFileKindError. Any other exception is not ok.
try:
  self.do_iter_changes(tree1, tree2)
except errors.BadFileKindError:
  pass

Also, do you have a description of what BFKE looks like to the user?

...

+ try:
+ tree1, tree2 = self.mutable_trees_to_test_trees(self,
tree1, tree2)
+ except (KeyError,):
+ raise tests.TestNotApplicable(
+ "Cannot represent a FIFO in this case %s" % self.id())

^- It seems really strange that the exception we would get is a
*KeyError*, but I'll live with that.

However, do you need to add 'self.id()'? Given that the failing test
should have that info...

Although recently I've noticed that the initial "Test...(foo) FAIL" line
has the right info but the final traceback does *not*... Which is a
pretty big PITA when debugging parameterized tests.

Anyway, you may want to re-think the exception handling for
'do_iter_changes()', but otherwise:

 review: approve
 merge: approve

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

iEYEARECAAYFAkrDYTwACgkQJdeBCYSNAANeaQCgnPlGe66w/ULa262nabr6/1MT
EJIAnR6bdjAO4XdFkWwIRXfr2vro8bHR
=wj/1
-----END PGP SIGNATURE-----

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'NEWS'
2--- NEWS 2009-09-30 00:02:11 +0000
3+++ NEWS 2009-09-30 06:05:23 +0000
4@@ -22,6 +22,9 @@
5 ``keyboard-interactive`` auth but not ``password`` auth when using
6 Paramiko. (Andrew Bennetts, #433846)
7
8+* When a file kind becomes unversionable after being added, a sensible
9+ error will be shown instead of a traceback. (Robert Collins, #438569)
10+
11 Improvements
12 ************
13
14
15=== modified file 'bzrlib/_dirstate_helpers_pyx.pyx'
16--- bzrlib/_dirstate_helpers_pyx.pyx 2009-08-26 03:20:32 +0000
17+++ bzrlib/_dirstate_helpers_pyx.pyx 2009-09-30 06:05:23 +0000
18@@ -1202,7 +1202,9 @@
19 content_change = 0
20 target_exec = False
21 else:
22- raise Exception, "unknown kind %s" % path_info[2]
23+ if path is None:
24+ path = self.pathjoin(old_dirname, old_basename)
25+ raise errors.BadFileKindError(path, path_info[2])
26 if source_minikind == c'd':
27 if path is None:
28 old_path = path = self.pathjoin(old_dirname, old_basename)
29
30=== modified file 'bzrlib/dirstate.py'
31--- bzrlib/dirstate.py 2009-08-26 03:20:32 +0000
32+++ bzrlib/dirstate.py 2009-09-30 06:05:23 +0000
33@@ -3328,7 +3328,9 @@
34 content_change = False
35 target_exec = False
36 else:
37- raise Exception, "unknown kind %s" % path_info[2]
38+ if path is None:
39+ path = pathjoin(old_dirname, old_basename)
40+ raise errors.BadFileKindError(path, path_info[2])
41 if source_minikind == 'd':
42 if path is None:
43 old_path = path = pathjoin(old_dirname, old_basename)
44
45=== modified file 'bzrlib/inventory.py'
46--- bzrlib/inventory.py 2009-09-24 20:09:36 +0000
47+++ bzrlib/inventory.py 2009-09-30 06:05:23 +0000
48@@ -2305,7 +2305,7 @@
49 try:
50 factory = entry_factory[kind]
51 except KeyError:
52- raise BzrError("unknown kind %r" % kind)
53+ raise errors.BadFileKindError(name, kind)
54 return factory(file_id, name, parent_id)
55
56
57
58=== modified file 'bzrlib/tests/per_intertree/test_compare.py'
59--- bzrlib/tests/per_intertree/test_compare.py 2009-08-26 03:20:32 +0000
60+++ bzrlib/tests/per_intertree/test_compare.py 2009-09-30 06:05:23 +0000
61@@ -948,6 +948,40 @@
62 (False, True))],
63 self.do_iter_changes(tree1, tree2))
64
65+ def test_file_becomes_unversionable_bug_438569(self):
66+ # This isn't strictly a intertree problem, but its the intertree code
67+ # path that triggers all stat cache updates on both xml and dirstate
68+ # trees.
69+ # In bug 438569, a file becoming a fifo causes an assert. Fifo's are
70+ # not versionable or diffable. For now, we simply stop cold when they
71+ # are detected (because we don't know how far through the code the
72+ # assumption 'fifo's do not exist' goes). In future we could report
73+ # the kind change and have commit refuse to go futher, or something
74+ # similar. One particular reason for choosing this approach is that
75+ # there is no minikind for 'fifo' in dirstate today, so we can't
76+ # actually update records that way.
77+ # To add confusion, the totally generic code path works - but it
78+ # doesn't update persistent metadata. So this test permits InterTrees
79+ # to either work, or fail with BadFileKindError.
80+ self.requireFeature(tests.OsFifoFeature)
81+ tree1 = self.make_branch_and_tree('1')
82+ self.build_tree(['1/a'])
83+ tree1.set_root_id('root-id')
84+ tree1.add(['a'], ['a-id'])
85+ tree2 = self.make_branch_and_tree('2')
86+ os.mkfifo('2/a')
87+ tree2.add(['a'], ['a-id'], ['file'])
88+ try:
89+ tree1, tree2 = self.mutable_trees_to_test_trees(self, tree1, tree2)
90+ except (KeyError,):
91+ raise tests.TestNotApplicable(
92+ "Cannot represent a FIFO in this case %s" % self.id())
93+ try:
94+ self.do_iter_changes(tree1, tree2)
95+ except errors.BzrError:
96+ self.assertRaises(errors.BadFileKindError, self.do_iter_changes,
97+ tree1, tree2)
98+
99 def test_missing_in_target(self):
100 """Test with the target files versioned but absent from disk."""
101 tree1 = self.make_branch_and_tree('1')

Subscribers

People subscribed via source and target branches