On Mon, Apr 25, 2011 at 2:12 PM, Bert Huijben <b...@qqmail.nl> wrote: > > >> -----Original Message----- >> From: Hyrum K Wright [mailto:hy...@hyrumwright.org] >> Sent: maandag 25 april 2011 20:27 >> To: dev@subversion.apache.org >> Cc: rhuij...@apache.org; comm...@subversion.apache.org >> Subject: Re: svn commit: r1091349 - >> /subversion/trunk/subversion/libsvn_wc/translate.c >> >> On Tue, Apr 12, 2011 at 4:47 AM, <rhuij...@apache.org> wrote: >> > Author: rhuijben >> > Date: Tue Apr 12 09:47:06 2011 >> > New Revision: 1091349 >> > >> > URL: http://svn.apache.org/viewvc?rev=1091349&view=rev >> > Log: >> > Use one db operation instead of a few when looking whether to set read- >> only >> > and executable. >> > >> > * subversion/libsvn_wc/translate.c >> > (svn_wc__maybe_set_executable, >> > svn_wc__maybe_set_read_only): Use >> svn_wc__db_read_node_install_info >> > instead of multiple db operations to determine the state for these >> > attributes. >> > >> > Modified: >> > subversion/trunk/subversion/libsvn_wc/translate.c >> > >> > Modified: subversion/trunk/subversion/libsvn_wc/translate.c >> > URL: >> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/tran >> slate.c?rev=1091349&r1=1091348&r2=1091349&view=diff >> > >> ========================================================== >> ==================== >> > --- subversion/trunk/subversion/libsvn_wc/translate.c (original) >> > +++ subversion/trunk/subversion/libsvn_wc/translate.c Tue Apr 12 >> 09:47:06 2011 >> > @@ -350,24 +350,35 @@ svn_wc__maybe_set_executable(svn_boolean >> > apr_pool_t *scratch_pool) >> > { >> > #ifndef WIN32 >> > - const svn_string_t *propval; >> > + svn_wc__db_status_t status; >> > + svn_wc__db_kind_t kind; >> > + svn_wc__db_lock_t *lock; >> > + apr_hash_t *props; >> > + >> > + if (did_set) >> > + *did_set = FALSE; >> > >> > SVN_ERR_ASSERT(svn_dirent_is_absolute(local_abspath)); >> > >> > - SVN_ERR(svn_wc__internal_propget(&propval, db, local_abspath, >> > - SVN_PROP_EXECUTABLE, scratch_pool, >> > - scratch_pool)); >> > - if (propval != NULL) >> > - { >> > - SVN_ERR(svn_io_set_file_executable(local_abspath, TRUE, FALSE, >> > - scratch_pool)); >> > - if (did_set) >> > - *did_set = TRUE; >> > - } >> > - else >> > + SVN_ERR(svn_wc__db_read_node_install_info(NULL, &status, &kind, >> NULL, NULL, >> > + &props, >> > + db, local_abspath, >> > + scratch_pool, > scratch_pool)); >> > + >> > + if (kind != svn_wc__db_kind_file >> > + || status != svn_wc__db_status_normal >> > + || props == NULL >> > + || ! apr_hash_get(props, SVN_PROP_EXECUTABLE, >> APR_HASH_KEY_STRING)) >> > + return SVN_NO_ERROR; /* Not executable */ >> > + >> > + SVN_ERR(svn_io_set_file_executable(local_abspath, TRUE, FALSE, >> > + scratch_pool)); >> > + if (did_set) >> > + *did_set = TRUE; >> > +#else >> > + if (did_set) >> > + *did_set = FALSE; >> > #endif >> > - if (did_set) >> > - *did_set = FALSE; >> > >> > return SVN_NO_ERROR; >> > } >> > @@ -379,41 +390,39 @@ svn_wc__maybe_set_read_only(svn_boolean_ >> > const char *local_abspath, >> > apr_pool_t *scratch_pool) >> > { >> > - const svn_string_t *needs_lock; >> > svn_wc__db_status_t status; >> > svn_wc__db_kind_t kind; >> > svn_wc__db_lock_t *lock; >> > + apr_hash_t *props; >> > >> > if (did_set) >> > *did_set = FALSE; >> > >> > SVN_ERR_ASSERT(svn_dirent_is_absolute(local_abspath)); >> > >> > - SVN_ERR(svn_wc__db_read_info(&status, &kind, NULL, NULL, NULL, >> NULL, >> > - NULL, NULL, NULL, NULL, NULL, >> > - NULL, NULL, NULL, NULL, NULL, NULL, >> > - NULL, NULL, NULL, NULL, NULL, NULL, >> > - &lock, >> > - db, local_abspath, scratch_pool, > scratch_pool)); >> > + SVN_ERR(svn_wc__db_read_node_install_info(NULL, &status, &kind, >> NULL, NULL, >> > + &props, >> > + db, local_abspath, >> > + scratch_pool, > scratch_pool)); >> >> These functions are run from within work queue items. If the action >> creating the work queue items changes the properties, the above won't >> pick up those changes. svn_wc__db_read_node_install_info() is >> documented to return the pristine properties, when we want/need the >> current properties here. > > Your use case wants the current properties. > > Commit, revert and (if I remember correctly) update prefer the pristine > versions.
Be that as it may, there nothing in either the docs or the implementation to suggest that this function operates on the pristine props. There's just a certain unsettling feeling to something called 'sync file with flags' which syncs with the pristine flags, and not the current ones. For a caller wanting to make a prop change, and then sync those changes through the work queue, there just is no way to do it. (In spite of the apparent advertisement to the contrary.) FWIW, I never liked a function with 'maybe' in it's name. Just a bit too non-deterministic for my liking... -Hyrum (getting off my curmudgeony soapbox now)