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

On 3 June 2011 10:41, Geoff <email address hidden> wrote:
> [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.
>
> I changed how this was done, because I didn't think the code in the finally statement would be executed properly. Please re-review this.
>
> It looks like you did change export() back into being a non-generator
> function, but the others still are. I am concerned there will be
> functions that count on the current behaviour so we ought to fix this.

The way the code base is currently set up, there is no need to use the other functions. They should just be passing in the information to the export() function.

I see::

- try:
- return _exporters[format](tree, dest, root, subdir, filtered=filtered,
- force_mtime=force_mtime)
- finally:
- tree.unlock()
-
+
+ for _ in _exporters[format](tree, dest, root, subdir, filtered,
force_mtime, per_file_timestamps, fileobj):
+
+ yield
+
+ tree.unlock()
+
+

> [fix] It's almost always a bug, and it is in this case, to have the
> unlock code not in either a finally block, or addCleanup, or some
> other equivalent. If the export is interrupted or errors, the tree
> won't be unlocked.

Fixed and will show up on the next push.