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

On Thu, Jun 2, 2011 at 11:01 AM, Jelmer Vernooij <email address hidden> 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.

That would still mean duplicating code in loggerhead vs bzrlib, which
frankly sucks.

Whats the concern over this being a generator in bzrlib?

-Rob