On 24.02.2011 18:03, Julian Foad wrote:
> On Wed, 2011-02-23, Daniel Shahaf wrote:
>> julianfoad_at_apache.org wrote on Tue, Feb 22, 2011 at 15:38:35 -0000:
>>> Modified: subversion/trunk/subversion/libsvn_wc/wc_db_pristine.c
>>> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/wc_db_pristine.c?rev=1073366&r1=1073365&r2=1073366&view=diff
>>> ==============================================================================
>>> --- subversion/trunk/subversion/libsvn_wc/wc_db_pristine.c (original)
>>> +++ subversion/trunk/subversion/libsvn_wc/wc_db_pristine.c Tue Feb 22 15:38:35 2011
>>> @@ -163,6 +165,57 @@ svn_wc__db_pristine_get_future_path(cons
>>> +/* Set (*BATON->contents) to a readable stream from which the pristine text
>>> + * identified by BATON->sha1_checksum can be read from the pristine store of
>>> + * SDB. If that text is not in the pristine store, return an error.
>>> + *
>>> + * Allocate the stream in BATON->result_pool.
>>> + *
>>> + * This function expects to be executed inside a SQLite txn.
>>> + *
>>> + * Implements 'notes/wc-ng/pristine-store' section A-3(d).
>>> + * Implements svn_sqlite__transaction_callback_t. */
>>> +static svn_error_t *
>>> +pristine_read_txn(void *baton,
>>> + svn_sqlite__db_t *sdb,
>>> + apr_pool_t *scratch_pool)
>>> +{
>> Should document somewhere that the returned stream is only guaranteed to
>> remain valid if the caller holds a wc lock, etc.?
>>
>> (svn_wc__db_pristine_read()'s docs don't mention locks)
> For now, I've written (on both functions):
>
> The stream will remain readable as long as the pristine text remains in
> the store; if the pristine text is removed from the store, any subsequent
> read or other operation on the stream may return an error.
Wait wait wait, I missed something there... You want to make *reads*
from the (transactionally opened) stream to error out if someone deletes
the pristine file? Woah. That implies a stat for every read, at least in
Unix-like environments, and frankly I can't see any good reason to do that.
>> Obviously there's still a bit of work here (eg, that "Delayed delete"
>> flag on Windows) and it can't all be done in one commit.
Why can't it be all done in one commit? All you need is to set
FILE_SHARE_READ and FILE_SHARE_DELETE flags in the CreateFile call ...
and surprise surprise, APR already does that by default (except on Win95
of course, but I couldn't care less about that). You don't need any
"delayed delete" flag, which does exist but is meant for creating
temporary files.
> I'm not sure we need that.
Yes we do. It is not the business of the wc_db+pristine-store to track
every process that happens to have an open handle to the pristine file.
A deletion of the pristine file should succeed even if there are open
handles referring to it. That's a trivial task on Unix (i.e., "do
nothing" ... modulo NFS most likely) and a trifle more involved on
Windows (where the trifle lies in doing the right thing if we need to
reinstate a file with the same checksum while there are open handles to it).
All it takes to make this behaviour valid is to state in the docs that
there's absolutely no guarantee that the pristine file that you've just
opened will still represent anything in the working copy by the time you
manage to read from it, or indeed that it will even continue to exist if
you close it.
-- Brane
Received on 2011-02-24 23:43:23 CET