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, <hwri...@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); > > >