[MERGE][BUG #187169] rollback an invalid delta
Martin Pool
mbp at canonical.com
Fri Feb 8 08:04:42 GMT 2008
Martin Pool has voted tweak.
Status is now: Conditionally approved
Comment:
This is useful robustness, thanks for doing it. I agree with you about
not putting this in the other state fields.
I think _consistency is too vague or general a word for this purpose.
If it means "broken" or "aborted" then say so. Otherwise it raises a
question about "consistent with what?" Then it could just hold a
boolean, rather than an enumeration - particularly because this
enumeration is I think at risk of people confusing it with the
_dirblock_state and other fields.
+class CorruptDirstate(BzrError):
+
+ _fmt = ("There is an inconsistency with your dirstate file at path"
+ " %(dirstate)s.\n"
+ "Error: %(description)s")
This is implying the problem is specifically with the file on disk,
which is probably fine but I wonder if that will really always be true.
This error class is tested in test_errors with a path string as the
first parameter. However, it's actually called with a dirstate
instance.
I have mixed feelings about the general issue of constructing exceptions
with objects vs strings. The object lets you get more attributes out of
it from a debugger, or interrogate or poke it from a finally clause.
But passing the string lets the object be gc'd, is more likely to avoid
bad messages for objects with no repr or secondary failures when we try
to use the object (which may already be broken.)
To catch this kind of thing we need to check the str of the exception
returned from assertRaises, which is unfortunately similar to what
test_errors does.
if real_add and entry[1][1][0] not in absent:
- raise errors.BzrError('dirstate: inconsistent delta,
with '
- 'tree 0. %r %r' % (new_path, file_id))
+ self._consistency = DirState.INCONSISTENT
+ raise errors.BzrError(new_path, file_id,
+ 'The entry was considered to be a genuinely new
record,'
+ ' but there was already an old record for it.')
Shouldn't this be a InconsistentDelta too?
So in this particular case I would change the exception-raising code to
just pass the path.
If you could either resubmit this, or indicate you're ok for me to make
those changes, I'll merge it before 1.2rc1.
For details, see:
http://bundlebuggy.aaronbentley.com/request/%3C47A7810F.2000300%40arbash-meinel.com%3E
More information about the bazaar
mailing list