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.
> 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.
I'm not sure we need that.
> > + return SVN_NO_ERROR;
> > +}
> > +
> > svn_error_t *
> > svn_wc__db_pristine_read(svn_stream_t **contents,
> > svn_wc__db_t *db,
> > @@ -173,7 +226,7 @@ svn_wc__db_pristine_read(svn_stream_t **
> > {
> > svn_wc__db_pdh_t *pdh;
> > const char *local_relpath;
> > - const char *pristine_abspath;
> > + pristine_read_baton_t b;
> ...
> > +pristine_install_txn(void *baton,
> > + svn_sqlite__db_t *sdb,
> > + apr_pool_t *scratch_pool)
> > +{
> ...
> > + /* If this pristine text is already present in the store, just keep it:
> > + * delete the new one and return. */
> > + SVN_ERR(svn_sqlite__get_statement(&stmt, sdb, STMT_SELECT_PRISTINE));
> > + SVN_ERR(svn_sqlite__bind_checksum(stmt, 1, b->sha1_checksum, scratch_pool));
> > + SVN_ERR(svn_sqlite__step(&have_row, stmt));
> > + SVN_ERR(svn_sqlite__reset(stmt));
> > + if (have_row)
> > + {
> > + /* Remove the temp file: it's already there */
> > + /* ### TODO: Maybe verify the new file matches the existing one. */
> > + SVN_ERR(svn_io_remove_file2(b->tempfile_abspath, FALSE, scratch_pool));
> > + return SVN_NO_ERROR;
>
> Perhaps check (assert?) that b->PRISTINE_ABSPATH exists?
I think what I want to do here is, only if SVN_DEBUG is defined, check
that both the old and new files exist and that they are "the same" in
some loose sense. For example:
#ifdef SVN_DEBUG
/* Consistency checks. Verify both files exist and match. */
{
apr_finfo_t finfo1, finfo2;
SVN_ERR(svn_io_stat(&finfo1, b->tempfile_abspath, APR_FINFO_SIZE,
scratch_pool));
SVN_ERR(svn_io_stat(&finfo2, b->pristine_abspath, APR_FINFO_SIZE,
scratch_pool));
if (finfo1->size != finfo2->size)
return svn_error_createf(SVN_ERR_WC_CORRUPT_TEXT_BASE, NULL,
_("New pristine doesn't match existing "
"pristine with same checksum '%s'"),
svn_checksum_to_ctring_display(
b->sha1_checksum, scratch_pool));
}
#endif
> (which reduces to dropping the "if (have_row)" check)
This 'if' block deals with the valid condition where the pristine text
that we're adding has already been added. I want to deal with that case
neatly. If I were to drop this block then in this case we would
overwrite the DB row, which would reset its ref count.
> > + }
> > +
> > + /* Move the file to its target location, or discard it if already there. */
> > + SVN_ERR(svn_io_check_path(b->pristine_abspath, &kind, scratch_pool));
Instead, I will simplify this part. This extra 'stat' and the following
if-then-else block are not really important, as they only deal with the
"orphan file" case. It doesn't matter if we overwrite an orphan file,
so we can unconditionally do the "rename".
> > + if (kind == svn_node_file)
> > + {
> > + /* Remove the temp file: it's already there */
> > + /* ### TODO: Maybe verify the new file matches the existing one. */
> > + SVN_ERR(svn_io_remove_file2(b->tempfile_abspath, FALSE, scratch_pool));
> > + }
> > + else
> > + {
> > + SVN_ERR(svn_io_file_rename(b->tempfile_abspath, b->pristine_abspath,
> > + scratch_pool));
> > + }
> ...
> > +}
>
> > +pristine_remove_if_unreferenced_txn(void *baton,
> > + svn_sqlite__db_t *sdb,
> > + apr_pool_t *scratch_pool)
> > {
> ...
> > + /* If we removed the DB row, then remove the file. */
> > if (affected_rows > 0)
> > {
> > - /* Remove the file. */
> > - SVN_ERR(get_pristine_fname(&pristine_abspath, wcroot->abspath,
> > - sha1_checksum, TRUE /* create_subdir */,
> > - scratch_pool, scratch_pool));
> > - SVN_ERR(svn_io_remove_file2(pristine_abspath, TRUE /* ignore_enoent */,
> > + /* ### TODO: If file not present, log a consistency error but return
> > + * success. */
> > + SVN_ERR(svn_io_remove_file2(b->pristine_abspath, FALSE /* ignore_enoent */,
> > scratch_pool));
>
> Shouldn't it be ignore_enoent=TRUE at least in release builds?
Yes. Thanks. I'll do it like this:
#ifdef SVN_DEBUG
/* Consistency check: If file not present, log an error. */
SVN_ERR(svn_io_remove_file2(b->pristine_abspath,
FALSE /* ignore_enoent */,
scratch_pool));
#else
/* In release mode, if file not present, ignore and return success. */
SVN_ERR(svn_io_remove_file2(b->pristine_abspath,
TRUE /* ignore_enoent */,
scratch_pool));
#endif
> > }
> >
> > return SVN_NO_ERROR;
> > }
Received on 2011-02-24 18:04:04 CET