On 25.02.2011 16:53, Julian Foad wrote:
> On Thu, 2011-02-24, Branko Čibej wrote:
>> 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:
>>>>> +/* 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.
> Hi Brane. Thanks for helping me out here. I'm *very* glad of your
> attention.
>
> A summary of this reply: I think I get you and agree your way is better.
>
> But taking one piece at a time...
>
>> 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.
> ... you did miss something: I said it *may* return an error. That is:
> this API does not promise that you can continue to read from the file
> after removing it from the pristine store, but nor does it promise that
> any such attempt shall fail (immediately or at all).
>
> The idea was that the caller should ensure the pristine file won't be
> deleted while it's reading it. (One way the caller can do so is to hold
> a WC lock on a WC subtree that contains a reference to that pristine.
> We could define other ways.)
>
> Does that make more sense?
Mm... frankly, this looks just a bit over-engineered to me. The usage
then goes like this:
* So, I want to read this pristine, let's call that specialized
"open" function in libsvn_wc that does all the magic for us ...
* oops, now I have a file handle, but I can't safely read from it
without first taking out a WC lock ...
* ah, but I'd have been better off taking the WC lock first ... but
then, why did I have to call that special magic "open" function in
the first place ...
etc. :)
It's much easier to do open ... read ... forget.
> I thought that was the kind of non-guarantee
> (although not the specific non-guarantee) you were advocating when you
> wrote before:
>
>> * Impose constraints on what users of the file handle can expect,
>> for example:
>> o you are not allowed write access through the file (pristine
>> file creation is a magical step)
>> o you cannot assume that you will find the file that the
>> handle refers to in any directory listing
>> o you cannot assume that the file will continue to exist after
>> the handle is closed
> [...]
>> 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.
> So you're suggesting we should promise that a reader can continue
> reading the file (at least once through to the end, not sure about
> rewinding) even if something else deletes the file from the store part
> way through. I think you're suggesting those semantics are more
> reasonable than "you have to hold some sort of lock while you read it",
> which is what my design boiled down to.
Yes, indeed, they're far more reasonable because the OS already gives
them to you. On Unix, when you delete a file, it vanishes from the
directory; but open handles remain valid, and the backing store of the
data still exists. The file only really goes away when the last handle
is closed.
On Windows, the situation is pretty much the same (assuming
FILE_SHARE_DELETE which we've already determined APR always does --
guess why :), *except* that the file only vanishes from the directory
after it's been deleted once the last handle to it is closed, that's why
I mentioned the tricky part of re-instating the file.
In other words, there's really nothing extra that either the user of the
file *or* the pristine store needs to do, except for guaranteeing that
the file is opened atomically from the point of view of the
wc-db+pristine-store.
[...]
> I guess I'll have to figure out how to implement this "trifle more
> involved" part on Windows, now.
Lucky you, the name of the file is the digest of its contents, so in
order to reinstate the file on Windows you only have get the system to
twiddle it's "deleted" bit. "Only." I seem to recall that's not even
hard to do, but my last battle with Windows filesystem internals was
more than 10 years ago. If you can't find relevant docs, you could try
asking APR for that functionality. I'm sure Will Rowe will give you a
dozen reasons why doing that is not a good idea, and also explain how to
do it. :)
-- Brane
Received on 2011-02-26 02:23:43 CET