[merge] Fix trailing whitespace bug #49182

[John Arbash Meinel]

Aaron Bentley wrote:
> John Arbash Meinel wrote:
>>> I've gotten bit by this several times, and it seems other people have as
>>> well. So I went ahead and wrote this patch which tests bundles that have
>>> their trailing whitespace messed with, and fixes the parser.
> 
> Great.  +1.
> 
>>> The selftests pass, though for some reason I'm getting tracebacks in the
>>> SFTP Server threads. 'Broken pipe' mostly. But I'm getting those in
>>> plain 'bzr.dev', so it shouldn't be anything with my patch.
> 
> Those seem to have come in with Robey's recent SFTP timeout fix.

Yeah, that's what I guessed. I wouldn't have +1'd it if I had seen that
happening. It seems that on my FC4 machine, it doesn't happen. But on
Ubuntu Dapper it does.

> 
>>>      def _read_revision_header(self):
>>> +        found_something = False
>>>          self.info.revisions.append(RevisionInfo(None))
>>>          for line in self._next():
>>>              # The bzr header is terminated with a blank line
>>>              # which does not start with '#'
>>>              if line is None or line == '\n':
>>>                  break
>>> +            found_something = True
>>>              self._handle_next(line)
>>> +        if not found_something:
>>> +            # Nothing was there, so remove the added revision
>>> +            self.info.revisions.pop()
>>> +        return found_something
> 
> It would be nice if revisions were added after they were filled in.
> Then this method would be exception-safe.  But not a showstopper.  I
> seem to recall we used self.info.revisions[-1] in a bunch of places.
> 
> Aaron

Well, the problem is that my original work was designed only to read a
single revision. Which you updated to read multiple revisions. So the
'state' of the current parser is self.info.revisions and the last entry
is the one being processed.

We really should switch it around, so that we read a hunk at a time,
process it, and put it on the stack. (And since you pointed it out
self._next() bugs the hell out of me :)

But this fixes the immediate bug, and I think the stuff is in place so
that v0.9 can be a denser, faster format that does much more correct
than we do now. bzip + base64 encoded knit-hunks that can be directly
applied should have some nice benefits. We just need to figure out
verification/validation stuff.
I've wanted to audit the bzr codebase and see how much integrity
checking we are doing. I added the sha1 check to weave extraction, but I
don't know if knits are doing it. And I think it would be prudent to
match the sha1 in the inventory to the sha1 the VersionedFile has.
Otherwise if one is used for the testament, but a different one is used
in storage, you still have a security hole.

John
=:->


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 254 bytes
Desc: OpenPGP digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060622/9bff7e6c/attachment.pgp