You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Hyrum K Wright <hy...@hyrumwright.org> on 2011/04/25 20:26:34 UTC
Re: svn commit: r1091349 - /subversion/trunk/subversion/libsvn_wc/translate.c
On Tue, Apr 12, 2011 at 4:47 AM, <rh...@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/translate.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.
-Hyrum
> +
> + if (kind != svn_wc__db_kind_file
> + || status != svn_wc__db_status_normal
> + || props == NULL
> + || ! apr_hash_get(props, SVN_PROP_NEEDS_LOCK, APR_HASH_KEY_STRING))
> + return SVN_NO_ERROR; /* Doesn't need lock handling */
> +
> + SVN_ERR(svn_wc__db_base_get_info(NULL, NULL, NULL, NULL, NULL, NULL, NULL,
> + NULL, NULL, NULL, NULL, NULL, NULL, NULL,
> + &lock, NULL,
> + db, local_abspath,
> + scratch_pool, scratch_pool));
>
> if (lock)
> return SVN_NO_ERROR; /* We have a lock */
> - else if (kind != svn_wc__db_kind_file)
> - return SVN_NO_ERROR;
>
> - /* Files that aren't in the repository yet should be left writable. */
> - if (status != svn_wc__db_status_normal)
> - return SVN_NO_ERROR;
> -
> - SVN_ERR(svn_wc__internal_propget(&needs_lock, db, local_abspath,
> - SVN_PROP_NEEDS_LOCK, scratch_pool,
> - scratch_pool));
> - if (needs_lock != NULL)
> - {
> - SVN_ERR(svn_io_set_file_read_only(local_abspath, FALSE, scratch_pool));
> - if (did_set)
> - *did_set = TRUE;
> - }
> + SVN_ERR(svn_io_set_file_read_only(local_abspath, FALSE, scratch_pool));
> + if (did_set)
> + *did_set = TRUE;
>
> return SVN_NO_ERROR;
> }
>
>
>
Re: svn commit: r1091349 - /subversion/trunk/subversion/libsvn_wc/translate.c
Posted by Hyrum K Wright <hy...@hyrumwright.org>.
On Mon, Apr 25, 2011 at 1:26 PM, Hyrum K Wright <hy...@hyrumwright.org> wrote:
> On Tue, Apr 12, 2011 at 4:47 AM, <rh...@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/translate.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.
In the interests of correctness, I've updated the call in both
functions in r1096555.
>> +
>> + if (kind != svn_wc__db_kind_file
>> + || status != svn_wc__db_status_normal
>> + || props == NULL
>> + || ! apr_hash_get(props, SVN_PROP_NEEDS_LOCK, APR_HASH_KEY_STRING))
>> + return SVN_NO_ERROR; /* Doesn't need lock handling */
>> +
>> + SVN_ERR(svn_wc__db_base_get_info(NULL, NULL, NULL, NULL, NULL, NULL, NULL,
>> + NULL, NULL, NULL, NULL, NULL, NULL, NULL,
>> + &lock, NULL,
>> + db, local_abspath,
>> + scratch_pool, scratch_pool));
>>
>> if (lock)
>> return SVN_NO_ERROR; /* We have a lock */
>> - else if (kind != svn_wc__db_kind_file)
>> - return SVN_NO_ERROR;
>>
>> - /* Files that aren't in the repository yet should be left writable. */
>> - if (status != svn_wc__db_status_normal)
>> - return SVN_NO_ERROR;
>> -
>> - SVN_ERR(svn_wc__internal_propget(&needs_lock, db, local_abspath,
>> - SVN_PROP_NEEDS_LOCK, scratch_pool,
>> - scratch_pool));
>> - if (needs_lock != NULL)
>> - {
>> - SVN_ERR(svn_io_set_file_read_only(local_abspath, FALSE, scratch_pool));
>> - if (did_set)
>> - *did_set = TRUE;
>> - }
>> + SVN_ERR(svn_io_set_file_read_only(local_abspath, FALSE, scratch_pool));
>> + if (did_set)
>> + *did_set = TRUE;
>>
>> return SVN_NO_ERROR;
>> }
>>
>>
>>
>
Re: svn commit: r1091349 - /subversion/trunk/subversion/libsvn_wc/translate.c
Posted by Hyrum K Wright <hy...@hyrumwright.org>.
On Mon, Apr 25, 2011 at 2:12 PM, Bert Huijben <be...@qqmail.nl> wrote:
>
>
>> -----Original Message-----
>> From: Hyrum K Wright [mailto:hyrum@hyrumwright.org]
>> Sent: maandag 25 april 2011 20:27
>> To: dev@subversion.apache.org
>> Cc: rhuijben@apache.org; commits@subversion.apache.org
>> Subject: Re: svn commit: r1091349 -
>> /subversion/trunk/subversion/libsvn_wc/translate.c
>>
>> On Tue, Apr 12, 2011 at 4:47 AM, <rh...@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)
Re: svn commit: r1091349 - /subversion/trunk/subversion/libsvn_wc/translate.c
Posted by Hyrum K Wright <hy...@hyrumwright.org>.
On Mon, Apr 25, 2011 at 2:12 PM, Bert Huijben <be...@qqmail.nl> wrote:
>
>
>> -----Original Message-----
>> From: Hyrum K Wright [mailto:hyrum@hyrumwright.org]
>> Sent: maandag 25 april 2011 20:27
>> To: dev@subversion.apache.org
>> Cc: rhuijben@apache.org; commits@subversion.apache.org
>> Subject: Re: svn commit: r1091349 -
>> /subversion/trunk/subversion/libsvn_wc/translate.c
>>
>> On Tue, Apr 12, 2011 at 4:47 AM, <rh...@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)
RE: svn commit: r1091349 - /subversion/trunk/subversion/libsvn_wc/translate.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Hyrum K Wright [mailto:hyrum@hyrumwright.org]
> Sent: maandag 25 april 2011 20:27
> To: dev@subversion.apache.org
> Cc: rhuijben@apache.org; commits@subversion.apache.org
> Subject: Re: svn commit: r1091349 -
> /subversion/trunk/subversion/libsvn_wc/translate.c
>
> On Tue, Apr 12, 2011 at 4:47 AM, <rh...@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.
Bert
Re: svn commit: r1091349 - /subversion/trunk/subversion/libsvn_wc/translate.c
Posted by Hyrum K Wright <hy...@hyrumwright.org>.
On Mon, Apr 25, 2011 at 1:26 PM, Hyrum K Wright <hy...@hyrumwright.org> wrote:
> On Tue, Apr 12, 2011 at 4:47 AM, <rh...@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/translate.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.
In the interests of correctness, I've updated the call in both
functions in r1096555.
>> +
>> + if (kind != svn_wc__db_kind_file
>> + || status != svn_wc__db_status_normal
>> + || props == NULL
>> + || ! apr_hash_get(props, SVN_PROP_NEEDS_LOCK, APR_HASH_KEY_STRING))
>> + return SVN_NO_ERROR; /* Doesn't need lock handling */
>> +
>> + SVN_ERR(svn_wc__db_base_get_info(NULL, NULL, NULL, NULL, NULL, NULL, NULL,
>> + NULL, NULL, NULL, NULL, NULL, NULL, NULL,
>> + &lock, NULL,
>> + db, local_abspath,
>> + scratch_pool, scratch_pool));
>>
>> if (lock)
>> return SVN_NO_ERROR; /* We have a lock */
>> - else if (kind != svn_wc__db_kind_file)
>> - return SVN_NO_ERROR;
>>
>> - /* Files that aren't in the repository yet should be left writable. */
>> - if (status != svn_wc__db_status_normal)
>> - return SVN_NO_ERROR;
>> -
>> - SVN_ERR(svn_wc__internal_propget(&needs_lock, db, local_abspath,
>> - SVN_PROP_NEEDS_LOCK, scratch_pool,
>> - scratch_pool));
>> - if (needs_lock != NULL)
>> - {
>> - SVN_ERR(svn_io_set_file_read_only(local_abspath, FALSE, scratch_pool));
>> - if (did_set)
>> - *did_set = TRUE;
>> - }
>> + SVN_ERR(svn_io_set_file_read_only(local_abspath, FALSE, scratch_pool));
>> + if (did_set)
>> + *did_set = TRUE;
>>
>> return SVN_NO_ERROR;
>> }
>>
>>
>>
>
RE: svn commit: r1091349 - /subversion/trunk/subversion/libsvn_wc/translate.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Hyrum K Wright [mailto:hyrum@hyrumwright.org]
> Sent: maandag 25 april 2011 20:27
> To: dev@subversion.apache.org
> Cc: rhuijben@apache.org; commits@subversion.apache.org
> Subject: Re: svn commit: r1091349 -
> /subversion/trunk/subversion/libsvn_wc/translate.c
>
> On Tue, Apr 12, 2011 at 4:47 AM, <rh...@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.
Bert