On Mon, Feb 15, 2010 at 10:51, Julian Foad <julian.f...@wandisco.com> wrote: >... >> If you could be so kind to glance over it and straighten out my picture, if >> necessary. Upon approval, I'll check it into notes/ so we can edit. >> >> Note, if my view is correct, this design text implies small changes to the >> current state of the API: >> - no need for _pristine_write() > > Can I suggest that, instead, _install() and _get_temp_dir() should not > be exposed. _write() should accept NULL for the checksum parameter and, > in that case, calculate the checksum itself. The implementation could > (at least in that case) use _get_temp_dir() and _install() like your > pseudo-code for the use case "store". Even when the checksum is provided > it could do the same and then verify the provided checksum.
pristine_write() is broken and should go away. The implementation cannot determine if the stream was closed due to *successful* completion, or if it is being closed due to an error. >... >> USE CASES >> ========= >> >> Pseudocode implies args for reference to working copy, pool etc. as needed. >> pristine_* = svn_wc__db_pristine_* >> _usable = svn_wc__db_checkmode_usable, etc. >> >> >> Use case "new": "I have locally-created content which should be stored >> -------------- as a pristine." >> Another pristine with identical content may exist coincidentally, in which >> case we technically don't need to write the pristine at all. But, as >> discussed >> in "On the method to write pristine files" above, we should anyway copy the >> new content to a tempfile to make sure nothing changes it in-between us >> checksumming and copying. So, this is exactly the same as use case "store"! > > I think the important distinction between your use cases "new" and > "store" is whether you have the checksum available a-priori. (Whether > the source is a file or a stream is not so important.) Yes, it *is* important. If only a stream is present, then we can checksum as we go (if we don't have it). If a file is present, then we could do a filesystem copy, rather than streaming operations (ie. no detranslation or checksumming needed). This is why I suggested the use cases focus around the source constraints. >... >> Use case "could use": "I have more complex options, and I want to get a >> -------------------- *fast* count on how many of certain pristines I can >> expect to exist. Does this one show?" > > That's not really a use case, now, is it? *Why* do I care whether the > store *might* have the pristine text I'm asking about? IIRC, the use case is more "are you *missing* <this> pristine?" Higher-level code may need to contact a repository if certain pristines are missing. If the pristine is there, but unusable... that's not a likely question. >... > API comment: I hope we could reduce the checking modes to just two: a > "normal" check which tells whether the store has a pristine copy (and if > it says it does, we assume it is usable!), and a "verify" which verifies > all the metadata including the checksum. These could (should, I think) > be two separate functions rather than modes of a single function. I believe so, yes, which is why I added that comment. >... >> Use case "repair": "I've seen this pristine is børken. Restore or remove!" >> ----------------- >> pseudocode: >> pristine_repair(&present, checksum) (7) > > Commenting on the API: The idea of having a separate "repair" API > function and use case sounds flawed - it just creates unnecessary work. > We should do something simpler such as having the _check(_valid) call > automatically delete any entry that is found to be invalid. Repair may also involve fetching broken entries. Of course, that isn't possible from a pure-WC standpoint, but the repair operation (as part of 'svn cleanup'?) will need to do that. >... Cheers, -g