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

Revision history for this message
Jelmer Vernooij (jelmer) wrote :

Hi Geoff,

On Wed, 2011-06-01 at 22:36 +0000, Geoff wrote:
> Geoff has proposed merging lp:~xaav/bzr/tarball-generator into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
> Related bugs:
> Bug #791005 in Bazaar: "Impossible to get data from export during progress."
> https://bugs.launchpad.net/bzr/+bug/791005
>
> For more details, see:
> https://code.launchpad.net/~xaav/bzr/tarball-generator/+merge/63180
>
> This change fixes the related bug:
> - yeild after ball.addfile is called.
> - export_tarball function wrapper.
Thanks for working on this... it'd be really nice to have export support
in Loggerhead.

I think rather than making export_tarball a generator, it would make
more sense to factor out the body of the loop in export_tarball, which
could then be used by your function and the existing export_tarball
function. I.e. a export_tarball_item function that just returns a (item,
fileobj) tuple.

Can you add a trivial test function for the new method you expose,
whatever it is? That way we won't accidentally change it, and break
loggerhead.

Cheers,

Jelmer

« Back to merge proposal