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 Wright <hw...@apache.org> on 2011/04/26 00:01:24 UTC
Re: svn commit: r1096619 - in /subversion/trunk/subversion/libsvn_wc:
translate.c translate.h workqueue.c
FYI: This has a slight alteration in the post-commit work queue item
skel. If you've got un-cleaned up working copies laying around, this
may negatively impact them. :)
(I went ahead with the commit, without a format bump, since work queue
items are short lived, and we don't make any promises of upgrading
them anyway.)
-Hyrum
On Mon, Apr 25, 2011 at 4:58 PM, <hw...@apache.org> wrote:
> Author: hwright
> Date: Mon Apr 25 21:58:43 2011
> New Revision: 1096619
>
> URL: http://svn.apache.org/viewvc?rev=1096619&view=rev
> Log:
> Create a single unified function to sync file permissions with those indicated
> by locks and various properties. Use it in post-commit and other processing.
>
> Note: this removes a couple of "optimizations" in the post-commit work queue
> handler. However, in doing so, we greatly simplify the code, and actually
> *reduce* the number of overall database accesses, which should actually speed
> things up.
>
> * subversion/libsvn_wc/translate.c
> (svn_wc__maybe_set_executable, svn_wc__maybe_set_read_only): Remove.
> (svn_wc__sync_flags_with_props): New.
>
> * subversion/libsvn_wc/translate.h
> (svn_wc__maybe_set_executable, svn_wc__maybe_set_read_only): Remove.
> (svn_wc__sync_flags_with_props): New.
>
> * subversion/libsvn_wc/workqueue.c
> (sync_file_flags): Make a simple wrapper around
> svn_wc__sync_flags_with_props().
> (install_committed_file): Remove extra parameters, and simply use the new
> function to do our dirty work.
> (process_commit_file_install): Remove extra params, and update a caller.
> (run_file_commit): Don't bother parsing the remove_executable and
> set_read_write flags.
> (svn_wc__wq_build_file_commit): Don't compute the internal propdiff.
>
> Modified:
> subversion/trunk/subversion/libsvn_wc/translate.c
> subversion/trunk/subversion/libsvn_wc/translate.h
> subversion/trunk/subversion/libsvn_wc/workqueue.c
>
> Modified: subversion/trunk/subversion/libsvn_wc/translate.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/translate.c?rev=1096619&r1=1096618&r2=1096619&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_wc/translate.c (original)
> +++ subversion/trunk/subversion/libsvn_wc/translate.c Mon Apr 25 21:58:43 2011
> @@ -342,89 +342,71 @@ svn_wc__expand_keywords(apr_hash_t **key
> }
>
> svn_error_t *
> -svn_wc__maybe_set_executable(svn_boolean_t *did_set,
> - svn_wc__db_t *db,
> - const char *local_abspath,
> - apr_pool_t *scratch_pool)
> +svn_wc__sync_flags_with_props(svn_boolean_t *did_set,
> + svn_wc__db_t *db,
> + const char *local_abspath,
> + apr_pool_t *scratch_pool)
> {
> -#ifndef WIN32
> svn_wc__db_status_t status;
> svn_wc__db_kind_t kind;
> - apr_hash_t *props;
> + svn_wc__db_lock_t *lock;
> + apr_hash_t *props = NULL;
>
> if (did_set)
> *did_set = FALSE;
>
> - SVN_ERR_ASSERT(svn_dirent_is_absolute(local_abspath));
> + /* ### We'll consolidate these info gathering statements in a future
> + commit. */
>
> - SVN_ERR(svn_wc__db_read_node_install_info(NULL, &status, &kind, NULL, NULL,
> - NULL,
> - db, local_abspath,
> - scratch_pool, scratch_pool));
> + SVN_ERR(svn_wc__db_read_info(&status, &kind, NULL, NULL, NULL, NULL, NULL,
> + NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL,
> + NULL, &lock, NULL, NULL, NULL, NULL, NULL,
> + NULL, NULL, NULL, NULL, NULL,
> + db, local_abspath,
> + scratch_pool, scratch_pool));
>
> SVN_ERR(svn_wc__db_read_props(&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 */
> + /* We actually only care about the following flags on files, so just
> + early-out for all other types. */
> + if (kind != svn_wc__db_kind_file)
> + return SVN_NO_ERROR;
>
> - SVN_ERR(svn_io_set_file_executable(local_abspath, TRUE, FALSE,
> - scratch_pool));
> + /* If we get this far, we're going to change *something*, so just set
> + the flag appropriately. */
> if (did_set)
> *did_set = TRUE;
> -#else
> - if (did_set)
> - *did_set = FALSE;
> -#endif
> -
> - return SVN_NO_ERROR;
> -}
> -
> -
> -svn_error_t *
> -svn_wc__maybe_set_read_only(svn_boolean_t *did_set,
> - svn_wc__db_t *db,
> - const char *local_abspath,
> - apr_pool_t *scratch_pool)
> -{
> - 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_node_install_info(NULL, &status, &kind, NULL, NULL,
> - NULL, db, local_abspath,
> - scratch_pool, scratch_pool));
> -
> - SVN_ERR(svn_wc__db_read_props(&props, db, local_abspath, scratch_pool,
> - scratch_pool));
>
> - if (kind != svn_wc__db_kind_file
> - || status != svn_wc__db_status_normal
> + /* Handle the read-write bit. */
> + if (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, &lock,
> - NULL, NULL, NULL, NULL, NULL,
> - db, local_abspath,
> - scratch_pool, scratch_pool));
> + {
> + SVN_ERR(svn_io_set_file_read_write(local_abspath, FALSE, scratch_pool));
> + }
> + else
> + {
> + if (! lock)
> + SVN_ERR(svn_io_set_file_read_only(local_abspath, FALSE, scratch_pool));
> + }
>
> - if (lock)
> - return SVN_NO_ERROR; /* We have a lock */
> +/* Windows doesn't care about the execute bit. */
> +#ifndef WIN32
>
> - SVN_ERR(svn_io_set_file_read_only(local_abspath, FALSE, scratch_pool));
> - if (did_set)
> - *did_set = TRUE;
> + if ( ( status != svn_wc__db_status_normal
> + && status != svn_wc__db_status_added )
> + || props == NULL
> + || ! apr_hash_get(props, SVN_PROP_EXECUTABLE, APR_HASH_KEY_STRING))
> + {
> + /* Turn off the execute bit */
> + SVN_ERR(svn_io_set_file_executable(local_abspath, FALSE, FALSE,
> + scratch_pool));
> + }
> + else
> + SVN_ERR(svn_io_set_file_executable(local_abspath, TRUE, FALSE,
> + scratch_pool));
> +#endif
>
> return SVN_NO_ERROR;
> }
>
> Modified: subversion/trunk/subversion/libsvn_wc/translate.h
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/translate.h?rev=1096619&r1=1096618&r2=1096619&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_wc/translate.h (original)
> +++ subversion/trunk/subversion/libsvn_wc/translate.h Mon Apr 25 21:58:43 2011
> @@ -108,29 +108,32 @@ svn_wc__expand_keywords(apr_hash_t **key
> apr_pool_t *result_pool,
> apr_pool_t *scratch_pool);
>
> -/* If the SVN_PROP_EXECUTABLE property is present at all, then set
> - LOCAL_ABSPATH in DB executable. If DID_SET is non-null, then set
> - *DID_SET to TRUE if did set LOCAL_ABSPATH executable, or to FALSE if not.
> +/* Sync the write and execute bit for LOCAL_ABSPATH with what is currently
> + indicated by the properties in the database:
>
> - Use SCRATCH_POOL for any temporary allocations.
> -*/
> -svn_error_t *
> -svn_wc__maybe_set_executable(svn_boolean_t *did_set,
> - svn_wc__db_t *db,
> - const char *local_abspath,
> - apr_pool_t *scratch_pool);
> -
> -/* If the SVN_PROP_NEEDS_LOCK property is present and there is no
> - lock token for the file in the working copy, set LOCAL_ABSPATH to
> - read-only. If DID_SET is non-null, then set *DID_SET to TRUE if
> - did set LOCAL_ABSPATH read-write, or to FALSE if not.
> + * If the SVN_PROP_NEEDS_LOCK property is present and there is no
> + lock token for the file in the working copy, set LOCAL_ABSPATH to
> + read-only.
> + * If the SVN_PROP_EXECUTABLE property is present at all, then set
> + LOCAL_ABSPATH executable.
> +
> + If DID_SET is non-null, then liberally set *DID_SET to TRUE if we might
> + have change the permissions on LOCAL_ABSPATH. (A TRUE value in *DID_SET
> + does not guarantee that we changed the permissions, simply that more
> + investigation is warrented.)
> +
> + This function looks at the current values of the above properties,
> + including any scheduled-but-not-yet-committed changes.
> +
> + If LOCAL_ABSPATH is a directory, this function is a no-op.
>
> Use SCRATCH_POOL for any temporary allocations.
> -*/
> -svn_error_t * svn_wc__maybe_set_read_only(svn_boolean_t *did_set,
> - svn_wc__db_t *db,
> - const char *local_abspath,
> - apr_pool_t *scratch_pool);
> + */
> +svn_error_t *
> +svn_wc__sync_flags_with_props(svn_boolean_t *did_set,
> + svn_wc__db_t *db,
> + const char *local_abspath,
> + apr_pool_t *scratch_pool);
>
> /* Internal version of svn_wc_translated_stream2(), which see. */
> svn_error_t *
>
> Modified: subversion/trunk/subversion/libsvn_wc/workqueue.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_wc/workqueue.c?rev=1096619&r1=1096618&r2=1096619&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_wc/workqueue.c (original)
> +++ subversion/trunk/subversion/libsvn_wc/workqueue.c Mon Apr 25 21:58:43 2011
> @@ -87,30 +87,8 @@ sync_file_flags(svn_wc__db_t *db,
> const char *local_abspath,
> apr_pool_t *scratch_pool)
> {
> - svn_boolean_t did_set;
> -
> - SVN_ERR(svn_wc__maybe_set_read_only(&did_set, db, local_abspath,
> - scratch_pool));
> - if (!did_set)
> - SVN_ERR(svn_io_set_file_read_write(local_abspath, FALSE, scratch_pool));
> -
> -#ifndef WIN32
> - SVN_ERR(svn_wc__maybe_set_executable(&did_set, db, local_abspath,
> - scratch_pool));
> -
> - if (!did_set)
> - {
> - svn_node_kind_t kind;
> -
> - SVN_ERR(svn_io_check_path(local_abspath, &kind, scratch_pool));
> -
> - /* We want to preserve whatever execute bits may be existent on
> - directories. */
> - if (kind != svn_node_dir)
> - SVN_ERR(svn_io_set_file_executable(local_abspath, FALSE, FALSE,
> - scratch_pool));
> - }
> -#endif
> + SVN_ERR(svn_wc__sync_flags_with_props(NULL, db, local_abspath,
> + scratch_pool));
>
> return SVN_NO_ERROR;
> }
> @@ -370,11 +348,10 @@ svn_wc__wq_build_base_remove(svn_skel_t
> * clobbering timestamps unnecessarily).
> *
> * Set the working file's executability according to its svn:executable
> - * property, or, if REMOVE_EXECUTABLE is TRUE, set it to not executable.
> + * property.
> *
> * Set the working file's read-only attribute according to its properties
> - * and lock status (see svn_wc__maybe_set_read_only()), or, if
> - * REMOVE_READ_ONLY is TRUE, set it to writable.
> + * and lock status (see svn_wc__maybe_set_read_only()).
> *
> * If the working file was re-translated or had its executability or
> * read-only state changed,
> @@ -387,13 +364,11 @@ static svn_error_t *
> install_committed_file(svn_boolean_t *overwrote_working,
> svn_wc__db_t *db,
> const char *file_abspath,
> - svn_boolean_t remove_executable,
> - svn_boolean_t remove_read_only,
> svn_cancel_func_t cancel_func,
> void *cancel_baton,
> apr_pool_t *scratch_pool)
> {
> - svn_boolean_t same, did_set;
> + svn_boolean_t same;
> const char *tmp_wfile;
> svn_boolean_t special;
>
> @@ -467,49 +442,13 @@ install_committed_file(svn_boolean_t *ov
> }
>
> /* ### should be using OP_SYNC_FILE_FLAGS, or an internal version of
> - ### that here. do we need to set *OVERWROTE_WORKING? */
> + ### that here. do we need to set *OVERWROTE_WORKING? */
>
> - if (remove_executable)
> - {
> - /* No need to chmod -x on a new file: new files don't have it. */
> - if (same)
> - SVN_ERR(svn_io_set_file_executable(file_abspath,
> - FALSE, /* chmod -x */
> - FALSE, scratch_pool));
> - /* ### We should avoid setting 'overwrote_working' here if we didn't
> - * change the executability. */
> - *overwrote_working = TRUE; /* entry needs wc-file's timestamp */
> - }
> - else
> - {
> - /* Set the working file's execute bit if props dictate. */
> - SVN_ERR(svn_wc__maybe_set_executable(&did_set, db, file_abspath,
> - scratch_pool));
> - if (did_set)
> - /* okay, so we didn't -overwrite- the working file, but we changed
> - its timestamp, which is the point of returning this flag. :-) */
> - *overwrote_working = TRUE;
> - }
> -
> - if (remove_read_only)
> - {
> - /* No need to make a new file read_write: new files already are. */
> - if (same)
> - SVN_ERR(svn_io_set_file_read_write(file_abspath, FALSE,
> - scratch_pool));
> - /* ### We should avoid setting 'overwrote_working' here if we didn't
> - * change the read-only-ness. */
> - *overwrote_working = TRUE; /* entry needs wc-file's timestamp */
> - }
> - else
> - {
> - SVN_ERR(svn_wc__maybe_set_read_only(&did_set, db, file_abspath,
> - scratch_pool));
> - if (did_set)
> - /* okay, so we didn't -overwrite- the working file, but we changed
> - its timestamp, which is the point of returning this flag. :-) */
> - *overwrote_working = TRUE;
> - }
> + /* ### Re: OVERWROTE_WORKING, the following function is rather liberal
> + ### with setting that flag, so we should probably decide if we really
> + ### care about it when syncing flags. */
> + SVN_ERR(svn_wc__sync_flags_with_props(overwrote_working, db, file_abspath,
> + scratch_pool));
>
> return SVN_NO_ERROR;
> }
> @@ -517,8 +456,6 @@ install_committed_file(svn_boolean_t *ov
> static svn_error_t *
> process_commit_file_install(svn_wc__db_t *db,
> const char *local_abspath,
> - svn_boolean_t remove_executable,
> - svn_boolean_t set_read_write,
> svn_cancel_func_t cancel_func,
> void *cancel_baton,
> apr_pool_t *scratch_pool)
> @@ -536,7 +473,6 @@ process_commit_file_install(svn_wc__db_t
>
> SVN_ERR(install_committed_file(&overwrote_working, db,
> local_abspath,
> - remove_executable, set_read_write,
> cancel_func, cancel_baton,
> scratch_pool));
>
> @@ -589,24 +525,13 @@ run_file_commit(svn_wc__db_t *db,
> const svn_skel_t *arg1 = work_item->children->next;
> const char *local_relpath;
> const char *local_abspath;
> - svn_boolean_t remove_executable;
> - svn_boolean_t set_read_write;
> - apr_int64_t v;
>
> local_relpath = apr_pstrmemdup(scratch_pool, arg1->data, arg1->len);
> SVN_ERR(svn_wc__db_from_relpath(&local_abspath, db, wri_abspath,
> local_relpath, scratch_pool, scratch_pool));
>
> - SVN_ERR(svn_skel__parse_int(&v, arg1->next, scratch_pool));
> - set_read_write = (v != 0);
> -
> - SVN_ERR(svn_skel__parse_int(&v, arg1->next->next, scratch_pool));
> - remove_executable = (v != 0);
> -
> return svn_error_return(
> process_commit_file_install(db, local_abspath,
> - remove_executable,
> - set_read_write,
> cancel_func, cancel_baton,
> scratch_pool));
> }
> @@ -619,42 +544,12 @@ svn_wc__wq_build_file_commit(svn_skel_t
> apr_pool_t *result_pool,
> apr_pool_t *scratch_pool)
> {
> - svn_boolean_t remove_executable = FALSE;
> - svn_boolean_t set_read_write = FALSE;
> const char *local_relpath;
> *work_item = svn_skel__make_empty_list(result_pool);
>
> - {
> - /* Examine propchanges here before installing the new properties in BASE
> - If the executable prop was -deleted-, remember this by setting
> - REMOVE_EXECUTABLE so that we can later tell install_committed_file() so.
> - The same applies to the needs-lock property, remembered by
> - setting SET_READ_WRITE. */
> -
> - int i;
> - apr_array_header_t *propchanges;
> -
> - SVN_ERR(svn_wc__internal_propdiff(&propchanges, NULL, db, local_abspath,
> - scratch_pool, scratch_pool));
> -
> - for (i = 0; i < propchanges->nelts; i++)
> - {
> - svn_prop_t *propchange = &APR_ARRAY_IDX(propchanges, i, svn_prop_t);
> -
> - if ((! strcmp(propchange->name, SVN_PROP_EXECUTABLE))
> - && (propchange->value == NULL))
> - remove_executable = TRUE;
> - else if ((! strcmp(propchange->name, SVN_PROP_NEEDS_LOCK))
> - && (propchange->value == NULL))
> - set_read_write = TRUE;
> - }
> - }
> -
> SVN_ERR(svn_wc__db_to_relpath(&local_relpath, db, local_abspath,
> local_abspath, result_pool, scratch_pool));
>
> - svn_skel__prepend_int(remove_executable, *work_item, result_pool);
> - svn_skel__prepend_int(set_read_write, *work_item, result_pool);
> svn_skel__prepend_str(local_relpath, *work_item, result_pool);
>
> svn_skel__prepend_str(OP_FILE_COMMIT, *work_item, result_pool);
>
>
>
Re: svn commit: r1096619 - in /subversion/trunk/subversion/libsvn_wc:
translate.c translate.h workqueue.c
Posted by Greg Stein <gs...@gmail.com>.
On Mon, Apr 25, 2011 at 20:56, Hyrum K Wright <hy...@hyrumwright.org> wrote:
> On Mon, Apr 25, 2011 at 5:59 PM, Greg Stein <gs...@gmail.com> wrote:
>...
>>> Instead, we've extended a skel so that the code can determine which
>>> version you're using (eg. a new, optional element at the end). Or
>>> we've chosen two different OP names ("foo" and "foo-2") that dispatch
>>> to different skel parsers and call a common function.
>
> Thanks for the background. Unfortunately, I don't know that this
> policy is documented anywhere (certainly not in workqueue.h).
Yup. I'm crap for documentation. Dunno where to put this. Maybe a
comment block where OP_* is defined? Something in the 'notes'
directory? ... just not sure what would be best, such that future devs
making changes, similar to the above, would find it.
>...
> I've re-added placeholder values in r1096641, so that the parse doesn't choke.
Cool. Will look...
Cheers,
-g
Re: svn commit: r1096619 - in /subversion/trunk/subversion/libsvn_wc:
translate.c translate.h workqueue.c
Posted by Hyrum K Wright <hy...@hyrumwright.org>.
On Mon, Apr 25, 2011 at 5:59 PM, Greg Stein <gs...@gmail.com> wrote:
> On Mon, Apr 25, 2011 at 18:51, Greg Stein <gs...@gmail.com> wrote:
>> On Mon, Apr 25, 2011 at 18:01, Hyrum Wright <hw...@apache.org> wrote:
>>> FYI: This has a slight alteration in the post-commit work queue item
>>> skel. If you've got un-cleaned up working copies laying around, this
>>> may negatively impact them. :)
>>>
>>> (I went ahead with the commit, without a format bump, since work queue
>>> items are short lived, and we don't make any promises of upgrading
>>> them anyway.)
>>
>> That is NOT the policy that we've chosen in the past.
>>
>> Instead, we've extended a skel so that the code can determine which
>> version you're using (eg. a new, optional element at the end). Or
>> we've chosen two different OP names ("foo" and "foo-2") that dispatch
>> to different skel parsers and call a common function.
Thanks for the background. Unfortunately, I don't know that this
policy is documented anywhere (certainly not in workqueue.h).
>> I disagree with changing the policy at this time, and would suggest
>> finding a solution that will not break working copies that have stale
>> work items in them.
>
> For this particular change, the "old" skel had three items in it (path
> and two flags). The "new" skel only has the path; any additional
> elements in the skel will be ignored.
>
> A new skel, processed by old code, would choke horribly... but the new
> code can (seemingly) interpret old skels just fine.
I've re-added placeholder values in r1096641, so that the parse doesn't choke.
-Hyrum
Re: svn commit: r1096619 - in /subversion/trunk/subversion/libsvn_wc:
translate.c translate.h workqueue.c
Posted by Greg Stein <gs...@gmail.com>.
On Mon, Apr 25, 2011 at 18:51, Greg Stein <gs...@gmail.com> wrote:
> On Mon, Apr 25, 2011 at 18:01, Hyrum Wright <hw...@apache.org> wrote:
>> FYI: This has a slight alteration in the post-commit work queue item
>> skel. If you've got un-cleaned up working copies laying around, this
>> may negatively impact them. :)
>>
>> (I went ahead with the commit, without a format bump, since work queue
>> items are short lived, and we don't make any promises of upgrading
>> them anyway.)
>
> That is NOT the policy that we've chosen in the past.
>
> Instead, we've extended a skel so that the code can determine which
> version you're using (eg. a new, optional element at the end). Or
> we've chosen two different OP names ("foo" and "foo-2") that dispatch
> to different skel parsers and call a common function.
>
> I disagree with changing the policy at this time, and would suggest
> finding a solution that will not break working copies that have stale
> work items in them.
For this particular change, the "old" skel had three items in it (path
and two flags). The "new" skel only has the path; any additional
elements in the skel will be ignored.
A new skel, processed by old code, would choke horribly... but the new
code can (seemingly) interpret old skels just fine.
Cheers,
-g
Re: svn commit: r1096619 - in /subversion/trunk/subversion/libsvn_wc:
translate.c translate.h workqueue.c
Posted by Greg Stein <gs...@gmail.com>.
On Mon, Apr 25, 2011 at 18:01, Hyrum Wright <hw...@apache.org> wrote:
> FYI: This has a slight alteration in the post-commit work queue item
> skel. If you've got un-cleaned up working copies laying around, this
> may negatively impact them. :)
>
> (I went ahead with the commit, without a format bump, since work queue
> items are short lived, and we don't make any promises of upgrading
> them anyway.)
That is NOT the policy that we've chosen in the past.
Instead, we've extended a skel so that the code can determine which
version you're using (eg. a new, optional element at the end). Or
we've chosen two different OP names ("foo" and "foo-2") that dispatch
to different skel parsers and call a common function.
I disagree with changing the policy at this time, and would suggest
finding a solution that will not break working copies that have stale
work items in them.
Cheers,
-g