Re: [Monotone-devel] [PATCH] New typesafe VA_ARGS replacement for database with operator % style

[Christof Petig]

Nathaniel Smith wrote:
> Right, my apologies yet again for taking so long to get back to this,
> just trying to catch up on things again.  This branch has been more
> trouble than it really should have been, mostly my fault... but I
> think we're almost there!

Thank you for keeping up the work on monotone!

>> - The dumping routine's callback is still difficult to understand. A
>> (future) rewrite can help here.

done. Whether it is really readable is debatable, but I tried to find a
compromise between redoing everything and redoing only the stuff I
touched for BLOBs.

>> - There's no unzip (unpack without unbase64) database function

done. I named it gunzip (which is exactly what it does). Inflate would
have been a valid choice, too.

>> - I did not touch t_migrate_schema.at and I still think that's ok
>> (refering to Tims comment)
> 
> I think the rule is if you add a migration function, then you touch
> t_migrate_schema.at (following the instructions at the top of that
> file).  I don't know why there would ever be an exception to this, but
> I missed your discussion earlier... why would it be okay in this case?

Ok. I'll try. done. I did not get the impression to touch this test at
all because I'm the first to add a nontrivial test to this file. There
simply are no old migrations in this test (e.g. pre roster) which only
yet dawns as intentional (this is the first schema change after
rosterification).

>> - monotone still does not use affinities (sql column types)
> 
> Obviously this isn't necessary; but is there some benefit to it?

It _might_ make some casting unnecessary (e.g. the value of a
certificate _might_ default to text (and not blob)). But by marking
columns as holding a specific type (text vs. blob [vs. integer]) you
also document it's use.

> Wonderful!  I notice that sqlite3_{column,value}_text_s look unused
> now.

It is. Axed. I had to introduce some reinterpret_cast<char const*> to
schema_migration.cc , though.

> Also, the change to t_sql_unpack.at is strange... XFAIL is for tests
> that are currently broken, but should be made to work in the future.
> There's no point in keeping around tests for functionality that has
> been flat-out removed.  Better to just delete the test entirely, or
> better, turn it into a test for unzip/inflate/whatever (with
> appropriate name).

done.

>   -- getting some decision on the deflate question (Matt?  Want to
>      just pick one?  It is not important enough to have a long
>      discussion over, I think :-).)

...

> Thanks for keeping on this,

It makes monotone one fourth smaller and faster, that's why I need it ;-)

   Christof

PS: What do you think about splitting cvssync (and other sync/import
features e.g. git,svn) into a separate program using monotone automate.
You did not answer to my proposal, yet. I'm all in favor of it.

signature.asc
Description: OpenPGP digital signature