Re: [Monotone-devel] Please review nvm.automate_get_roster

[Thomas Keller]

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 | address@hidden | 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