[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r1096619 - in /subversion/trunk/subversion/libsvn_wc: translate.c translate.h workqueue.c

From: Hyrum Wright <hwright_at_apache.org>
Date: Mon, 25 Apr 2011 17:01:24 -0500

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, <hwright_at_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);
>
>
>
Received on 2011-04-26 00:01:55 CEST

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.