Code review comment for lp:~jameinel/bzr/1.16-commit-fulltext

I have a couple of comments, but basically:

 vote approve

John A Meinel wrote:
[...]
> + def _add_text(self, key, parents, text, nostore_sha=None, random_id=False):
> + """See VersionedFiles.add_text()."""
> + self._index._check_write_ok()
> + self._check_add(key, None, random_id, check_content=False)
> + if text.__class__ is not str:
> + raise errors.BzrBadParameterUnicode("text")

Well... 'text' might not be unicode either ;)

(I'm not really concerned, but ideally perhaps that exception would take the
param value or type as well as param name...)

[...]
> @@ -1597,7 +1615,7 @@
> if refs:
> for ref in refs:
> if ref:
> - raise KnitCorrupt(self,
> + raise errors.KnitCorrupt(self,

Thanks for fixing this. pyflakes reports two other undefined names in this
file:

bzrlib/groupcompress.py:1543: undefined name 'progress'
bzrlib/groupcompress.py:1689: undefined name 'RevisionNotPresent'

Now would be a good time to fix those too :)

> === modified file 'bzrlib/knit.py'
[...]
> + line_bytes = ''.join(lines)
> return self._add(key, lines, parents,
> - parent_texts, left_matching_blocks, nostore_sha, random_id)
> + parent_texts, left_matching_blocks, nostore_sha, random_id,
> + line_bytes=line_bytes)

You pass lines *and* line_bytes? That looks odd. I'm pretty sure it's
intentional, but it's worth pointing this out and explaining the rationale
in _add's docstring, I think.

> + def _add_text(self, key, parents, text, nostore_sha=None, random_id=False):
> + """See VersionedFiles.add_text()."""
> + self._index._check_write_ok()
> + self._check_add(key, None, random_id, check_content=False)
> + if text.__class__ is not str:
> + raise errors.BzrBadParameterUnicode("text")
> + if parents is None:
> + # The caller might pass None if there is no graph data, but kndx
> + # indexes can't directly store that, so we give them
> + # an empty tuple instead.
> + parents = ()
> + return self._add(key, None, parents,
> + None, None, nostore_sha, random_id,
> + line_bytes=text)

Hmm. It would be nice if this wasn't largely the same as _add_text in
groupcompress.py.

[...]
> - if lines:
> - if lines[-1][-1] != '\n':
> - # copy the contents of lines.
> + no_eol = False
> + # Note: line_bytes is not modified to add a newline, that is tracked
> + # via the no_eol flag. 'lines' *is* modified, because that is the
> + # general values needed by the Content code.

Gosh I hate our no_eol flag... :)

[...]
> @@ -1920,21 +1949,16 @@
> function spends less time resizing the final string.
> :return: (len, a StringIO instance with the raw data ready to read.)
> """
> - # Note: using a string copy here increases memory pressure with e.g.
> - # ISO's, but it is about 3 seconds faster on a 1.2Ghz intel machine
> - # when doing the initial commit of a mozilla tree. RBC 20070921
> - bytes = ''.join(chain(

itertools.chain is now unused in this module, delete its import.

[...]
> + for chunk in chunks:
> + if type(chunk) != str:

“is not”, seeing as you're touching this line anyway...

> === modified file 'bzrlib/tests/test_versionedfile.py'
[...]

The conditionals and repetition here are a bit ugly, but it'll do. Ideally
this would be done with the proper test parameterisation tools, though.

-Andrew.