Code review comment for lp:~thisfred/u1db/fix-local-gen-before-sync

Revision history for this message
John A Meinel (jameinel) wrote :

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 8/15/2012 2:42 PM, Eric Casteleijn wrote:
> Eric Casteleijn has proposed merging
> lp:~thisfred/u1db/fix-local-gen-before-sync into lp:u1db.
>
> Requested reviews: Ubuntu One hackers (ubuntuone-hackers)
>
> For more details, see:
> https://code.launchpad.net/~thisfred/u1db/fix-local-gen-before-sync/+merge/119705
>
> Fixed local_gen to be an OUT parameter only, and return it from
> sync()
>

     // fprintf(stderr, "Starting\n");
- - if (db == NULL || target == NULL || local_gen_before_sync == NULL) {
+ if (db == NULL || target == NULL) {
         // fprintf(stderr, "DB, target, or local are NULL\n");

You should probably still check that "local_gen == NULL" here, so that
we generate an INVALID_PARAMETER error if it is not supplied. Or are
you intentionally making it optional? If so, you should update the
docstring to indicate that.

The rest looks good.

 merge: approve

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.12 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAlArrJsACgkQJdeBCYSNAAPoygCgnAVKFl2EoKukAwlohz/DRhsd
5bsAn3vlT/AqJR4S5yfIIlgC9tQDedtt
=dSwQ
-----END PGP SIGNATURE-----

review: Approve

« Back to merge proposal