Julian Foad wrote on Thu, Feb 24, 2011 at 17:03:21 +0000:
> On Wed, 2011-02-23, Daniel Shahaf wrote:
> > julianf...@apache.org wrote on Tue, Feb 22, 2011 at 15:38:35 -0000:
> > > + 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. */
> {
Okay. (That's not much more expensive, and might catch some bugs.)
> 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.
>
Right, my bad: I didn't mean to suggest overwriting the DB row, just to
re-use the IO logic just after the end of the if().
> > > + }
> > > +
> > > + /* 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".
> > > +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
>
+1
