Am 06.10.2010 15:09, schrieb Timothy Brownawell: > On 10/04/2010 07:32 AM, Thomas Keller wrote: >> >> Please review the above branch. The branch name has not so much in >> common with the actual implementation anymore, though: >> >> * a new file_sizes table has been added which records the size >> in bytes of individual files >> * two new automate commands have been added, get_file_size returns >> a single recorded file size, get_extended_manifest_of returns >> a format similar to the current roster format, but without the >> local node ids. Additionally, the format prints out the recorded >> file size for each file node >> * migration code and tests have been added, some parts of the migration >> code were refactored / expanded >> * documentation and NEWS / UPGRADE was updated as well >> >> Since I did the migration pretty late in the process, I realized that >> the put_file_sizes_for_revision() method which is used during the >> migration could maybe used instead of saving the individual file sizes >> on commit, but then again I thought it was cleaner (and faster) to not >> have to iterate over the revision object again there. > > Yeah, it's probably better to figure the sizes when you already have the > file data, instead of pulling it all out of the db/cache again.
Right, this is of course another important aspect. I also thought that we _might_ start to delete old cruft in the very future, i.e. no longer support migrations from very old versions. Since put_file_sizes_for_revision is only used in migration-related code now, this can then safely removed. Would you say we should start removing this migration-relevant code earlier already? I mean, do we really still carry around the migration code and functions from the pre-roster area...? This would be - of course - part of another branch, it just came into my mind for the upcoming advent of 0.99 / 1.0. > + // FIXME: could we safely drop merge revisions here since their > + // individual file changes should be already recorded in other > + // revisions? > > They won't be for files that had to be line-merged. Totally forgot about that. Ok, I'll remove this comment then. > + // if we should regenerate more than just one specific cache, > + // we regenerate them all > + if (m->regen_type != regen_none) > + { > + if (regen_type == regen_none) > + regen_type = m->regen_type; > + else > + regen_type = regen_all; > + } > > I wonder would it be worth it to make regen_cache_type a set of flags, > so we can or together only the needed ones? > enum regen_cache_type { regen_none = 0, regent_rosters = 1, > regen_heights = 2, regen_branches = 4, > regen_file_sizes = 8, regen_all = 15 }; > > regen_type |= m->regen_type Very good idea indeed - will change this accordingly. Thanks for the review. If nobody else speaks up, I'll merge the branch into nvm after I did the mentioned changes above. Thomas. -- GPG-Key 0x160D1092 | tommyd3...@jabber.ccc.de | http://thomaskeller.biz Please note that according to the EU law on data retention, information on every electronic information exchange might be retained for a period of six months or longer: http://www.vorratsdatenspeicherung.de/?lang=en
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Monotone-devel mailing list Monotone-devel@nongnu.org http://lists.nongnu.org/mailman/listinfo/monotone-devel