| 1 |
On 07/12/2016 06:59 AM, Alexander Berntsen wrote: |
| 2 |
> The _chf_deserializers and _md5_deserializer stuff looks rather |
| 3 |
> overengineered. |
| 4 |
|
| 5 |
That stuff is not strictly required after the addition of the |
| 6 |
intelligent reconstruct_eclasses skipping in __getitem__. However, it's |
| 7 |
still good to have because it protects against a subtle misbehavior of |
| 8 |
reconstruct_eclasses, where it's called with chf_type='md5' and produces |
| 9 |
an invalid data structure containing mtime strings (rather than mtime ints). |
| 10 |
|
| 11 |
> I don't know what the reconstruct_eclasses skipping |
| 12 |
> entails (the comment makes it seem like "skip this because apparently |
| 13 |
> it's different lol who knows -_o_-"). |
| 14 |
|
| 15 |
The _eclasses_data contains either md5 or mtime. It's a waste of time to |
| 16 |
try to call reconstruct_eclasses with chf_type='md5' when eclasses |
| 17 |
contains mtime data (and it would also produce an invalid data structure |
| 18 |
in the absence of the _chf_deserializers and _md5_deserializer stuff). |
| 19 |
So, it's nice to take the presence of _md5_ or _mtime_ in the cache |
| 20 |
entry as a hint about whether _eclasses_ contains md5 or mtime data. |
| 21 |
|
| 22 |
> The rest of the patch lgtm. |
| 23 |
|
| 24 |
I'll add the insights that I have discussed above as comments the patch. |
| 25 |
-- |
| 26 |
Thanks, |
| 27 |
Zac |
Archives