On Mon, Apr 25, 2011 at 2:12 PM, Bert Huijben <bert_at_qqmail.nl> wrote:
>
>
>> -----Original Message-----
>> From: Hyrum K Wright [mailto:hyrum_at_hyrumwright.org]
>> Sent: maandag 25 april 2011 20:27
>> To: dev_at_subversion.apache.org
>> Cc: rhuijben_at_apache.org; commits_at_subversion.apache.org
>> Subject: Re: svn commit: r1091349 -
>> /subversion/trunk/subversion/libsvn_wc/translate.c
>>
>> On Tue, Apr 12, 2011 at 4:47 AM, <rhuijben_at_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)
Received on 2011-04-25 21:26:38 CEST