Code review comment for lp://staging/~xaav/bzr/tarball-generator

I commend this patch ;-)

[question] It seems to me that you can get streaming by just observing the chunks being written into the file object, and having a generator as well is redundant. I see this is basically needed because PEP333 strongly deprecates having streaming data sent to a write function returned by a generator.

Perhaps it would at least be good to explain that it is done this way for the sake of PEP333.

[question] Given that wsgi does actually support having write() called repeatedly for streaming data, why not use it? I realize it's not the recommended interface, but it is the interface that does fit most naturally with the lower level tarball code we're using. It seems a bit like we're reimplementing something that could be better done in the wsgi framework -- specifically, turning push commands into pull. That would have the concrete benefit of allowing streaming just exactly as data is produced by the compressor, on finer granularity than exporting a single file.

+
+def get_export_generator(tree, dest=None, format=None, root=None, subdir=None, filtered=False,
+ per_file_timestamps=False, fileobj=None):
+
+ """Returns a generator that exports the given Tree to the specific destination.
...
+ :param fileobj: Optional file object to use

Factoring out this function makes sense.

[fix] Please make the docstring explain what the generator actually produces. I am confused whether the content is written to the file object or returned in the generator; also how fileobj interacts with dest.

[idea] Actually, perhaps it would be better to do away with 'dest' entirely at this layer? I think it is only there for the sake of heuristics about choosing the format, and then for opening the file, and

[fix] I see all the actual 'exporters', and also 'export' itself now return generators which must be consumed for them to work. Generally if we change an api in such a way that existing callers will be wrong (rather than getting an error), we ought to rename the api too, and of course add a news entry. Another option would be to keep the existing function names doing the whole job, and add new ones that are generators.

+def export_tarball_item(tree, ball, root, dp, ie, subdir=None, filtered=False,
+ force_mtime=None):

Lifting this out is good.

[tweak] Per pep8, we indent continuation lines either by 4 spaces, or to be just after the preceding parenthesis.