[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r940898 - in /subversion/trunk/subversion: libsvn_wc/wc-queries.sql libsvn_wc/wc_db.c libsvn_wc/wc_db.h tests/libsvn_wc/pristine-store-test.c

From: Julian Foad <julian.foad_at_wandisco.com>
Date: Wed, 05 May 2010 10:31:03 +0100

Greg Stein wrote:
> On Tue, May 4, 2010 at 11:14, <julianfoad_at_apache.org> wrote:
> >...
> > +++ subversion/trunk/subversion/libsvn_wc/wc_db.c Tue May 4 15:14:13 2010
> > @@ -2560,6 +2560,75 @@ svn_wc__db_pristine_install(svn_wc__db_t
> >
> >
> > svn_error_t *
> > +svn_wc__db_pristine_remove(svn_wc__db_t *db,
> > + const char *wri_abspath,
> > + const svn_checksum_t *sha1_checksum,
> > + apr_pool_t *scratch_pool)
> > +{
> > + svn_wc__db_pdh_t *pdh;
> > + const char *local_relpath;
> > + const char *pristine_abspath;
> > + svn_error_t *err;
> > + const svn_checksum_t *md5_checksum;
> > + svn_boolean_t is_referenced = FALSE;
>
> Why initialize this? It will be unconditionally set below.

Oops, fixed.

> > +
> > + SVN_ERR_ASSERT(svn_dirent_is_absolute(wri_abspath));
> > + SVN_ERR_ASSERT(sha1_checksum->kind == svn_checksum_sha1);
> > +
> > + SVN_ERR(parse_local_abspath(&pdh, &local_relpath, db, wri_abspath,
> > + svn_sqlite__mode_readwrite,
> > + scratch_pool, scratch_pool));
> > + VERIFY_USABLE_PDH(pdh);
> > +
> > + err = get_pristine_fname(&pristine_abspath, pdh, sha1_checksum,
> > +#ifndef SVN__SKIP_SUBDIR
> > + TRUE /* create_subdir */,
> > +#endif
> > + scratch_pool, scratch_pool);
> > + SVN_ERR(err);
>
> Why compute the path so early? This won't be needed unless/until you
> actually determine the file should be removed.

Thanks, fixed.

[...]
> > + SVN_DBG(("Was referenced: '%s'\n",
> > + svn_checksum_to_cstring_display(md5_checksum, scratch_pool)));
>
> Here...
>
> > + }
> > + else
> > + SVN_DBG(("Not referenced: '%s'\n",
> > + svn_checksum_to_cstring_display(md5_checksum, scratch_pool)));
>
> ... and here. SVN_DBG cannot be left in the committed source code. The
> macro does not exist in release builds.

Oops, fixed.

All the above: r941212.

> >...
> > +++ subversion/trunk/subversion/libsvn_wc/wc_db.h Tue May 4 15:14:13 2010
> > @@ -866,6 +866,16 @@ svn_wc__db_pristine_get_md5(const svn_ch
> > apr_pool_t *scratch_pool);
> >
> >
> > +/* Remove the pristine text with SHA-1 checksum SHA1_CHECKSUM from the
> > + * pristine store, iff it is not referenced by any of the (other) WC DB
> > + * tables. */
>
> Also remember that we don't want to remove pristines why
> administrative locks (WCLOCK) are present. That may signify a commit
> is occurring and needed pristines are transiently living in the
> pristine storage.

I need to think on this some more and learn about WCLOCK.

If I'm using "pristine_remove" in a general garbage-collection manner
(outside of any code path that installs pristine texts), I would want to
bail if there are any entries in the WCLOCK table.

If I'm using "pristine_remove" within a commit or update code path
(where I am also installing new text bases, but I can ensure that the
one I'm trying to remove is not one that I'm in the process of
installing), then I would want to bail if there are any locks in the
WCLOCK table *other than* those locks that are in WC_CTX.

Does that sound right?

- Julian
Received on 2010-05-05 11:31:41 CEST

This is an archived mail posted to the Subversion Dev mailing list.