subversion-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Daniel Shahaf <danie...@elego.de>
Subject Re: svn commit: r1464122 - in /subversion/trunk/subversion/libsvn_repos: commit.c fs-wrap.c hooks.c load-fs-vtable.c repos.h
Date Wed, 03 Apr 2013 18:03:41 GMT
cmpilato@apache.org wrote on Wed, Apr 03, 2013 at 17:51:56 -0000:
> Author: cmpilato
> Date: Wed Apr  3 17:51:56 2013
> New Revision: 1464122
> 
> URL: http://svn.apache.org/r1464122
> Log:
> Avoid parsing the hooks-env file multiple times for closely-knit hook
> invocations, specifically the pre-/post- pairs for commit, revprop
> change, lock, and unlock operations.
> 
> * subversion/libsvn_repos/repos.h,
> * subversion/libsvn_repos/hooks.c
>   (svn_repos__parse_hooks_env): Was parse_hooks_env().
>   (svn_repos__hooks_start_commit,
>    svn_repos__hooks_pre_commit,
>    svn_repos__hooks_post_commit,
>    svn_repos__hooks_pre_revprop_change,
>    svn_repos__hooks_post_revprop_change,
>    svn_repos__hooks_pre_lock,
>    svn_repos__hooks_post_lock,
>    svn_repos__hooks_pre_unlock,
>    svn_repos__hooks_post_unlock): Add 'hooks_env' parameter, used now
>     instead of calling parse_hooks_env() from within.
> 
> * subversion/libsvn_repos/commit.c
>   (complete_cb, svn_repos__get_commit_ev2): Call
>     svn_repos__parse_hooks_env(), and update calls to hook wrapper
>     functions.
> 
> * subversion/libsvn_repos/fs-wrap.c
>   (svn_repos_fs_commit_txn, svn_repos_fs_begin_txn_for_commit2,
>    svn_repos_fs_change_rev_prop4, svn_repos_fs_lock, svn_repos_fs_unlock): 
>     Call svn_repos__parse_hooks_env(), and update calls to hook wrapper
>     functions.
> 
> * subversion/libsvn_repos/load-fs-vtable.c
>   (close_revision): Call svn_repos__parse_hooks_env(), and update
>     calls to hook wrapper functions.
> 
> Modified:
>     subversion/trunk/subversion/libsvn_repos/commit.c
>     subversion/trunk/subversion/libsvn_repos/fs-wrap.c
>     subversion/trunk/subversion/libsvn_repos/hooks.c
>     subversion/trunk/subversion/libsvn_repos/load-fs-vtable.c
>     subversion/trunk/subversion/libsvn_repos/repos.h
> 
> Modified: subversion/trunk/subversion/libsvn_repos/commit.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_repos/commit.c?rev=1464122&r1=1464121&r2=1464122&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_repos/commit.c (original)
> +++ subversion/trunk/subversion/libsvn_repos/commit.c Wed Apr  3 17:51:56 2013
> @@ -1295,10 +1295,16 @@ complete_cb(void *baton,
>    const char *conflict_path;
>    svn_error_t *err;
>    const char *post_commit_errstr;
> +  apr_hash_t *hooks_env;
> +
> +  /* Parse the hooks-env file (if any). */
> +  SVN_ERR(svn_repos__parse_hooks_env(&hooks_env, eb->repos->hooks_env_path,
> +                                     scratch_pool, scratch_pool));
>  
>    /* The transaction has been fully edited. Let the pre-commit hook
>       have a look at the thing.  */
> -  SVN_ERR(svn_repos__hooks_pre_commit(eb->repos, eb->txn_name, scratch_pool));
> +  SVN_ERR(svn_repos__hooks_pre_commit(eb->repos, hooks_env,
> +                                      eb->txn_name, scratch_pool));
>  
>    /* Hook is done. Let's do the actual commit.  */
>    SVN_ERR(svn_fs__editor_commit(&revision, &post_commit_err, &conflict_path,
> @@ -1314,8 +1320,8 @@ complete_cb(void *baton,
>       Other errors may have occurred within the FS (specified by the
>       POST_COMMIT_ERR localvar), but we need to run the hooks.  */
>    SVN_ERR_ASSERT(SVN_IS_VALID_REVNUM(revision));
> -  err = svn_repos__hooks_post_commit(eb->repos, revision, eb->txn_name,
> -                                     scratch_pool);
> +  err = svn_repos__hooks_post_commit(eb->repos, hooks_env, revision,
> +                                     eb->txn_name, scratch_pool);
>    if (err)
>      err = svn_error_create(SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED, err,
>                             _("Commit succeeded, but post-commit hook failed"));
> @@ -1405,6 +1411,11 @@ svn_repos__get_commit_ev2(svn_editor_t *
>    };
>    struct ev2_baton *eb;
>    const svn_string_t *author;
> +  apr_hash_t *hooks_env;
> +
> +  /* Parse the hooks-env file (if any). */
> +  SVN_ERR(svn_repos__parse_hooks_env(&hooks_env, repos->hooks_env_path,
> +                                     scratch_pool, scratch_pool));
>  
>    /* Can the user modify the repository at all?  */
>    /* ### check against AUTHZ.  */
> @@ -1428,7 +1439,8 @@ svn_repos__get_commit_ev2(svn_editor_t *
>    SVN_ERR(apply_revprops(repos->fs, eb->txn_name, revprops, scratch_pool));
>  
>    /* Okay... some access is allowed. Let's run the start-commit hook.  */
> -  SVN_ERR(svn_repos__hooks_start_commit(repos, author ? author->data : NULL,
> +  SVN_ERR(svn_repos__hooks_start_commit(repos, hooks_env,
> +                                        author ? author->data : NULL,
>                                          repos->client_capabilities,
>                                          eb->txn_name, scratch_pool));
>  
> 
> Modified: subversion/trunk/subversion/libsvn_repos/fs-wrap.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_repos/fs-wrap.c?rev=1464122&r1=1464121&r2=1464122&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_repos/fs-wrap.c (original)
> +++ subversion/trunk/subversion/libsvn_repos/fs-wrap.c Wed Apr  3 17:51:56 2013
> @@ -54,12 +54,17 @@ svn_repos_fs_commit_txn(const char **con
>    apr_hash_t *props;
>    apr_pool_t *iterpool;
>    apr_hash_index_t *hi;
> +  apr_hash_t *hooks_env;
>  
>    *new_rev = SVN_INVALID_REVNUM;
>  
> +  /* Parse the hooks-env file (if any). */
> +  SVN_ERR(svn_repos__parse_hooks_env(&hooks_env, repos->hooks_env_path,
> +                                     pool, pool));
> +
>    /* Run pre-commit hooks. */
>    SVN_ERR(svn_fs_txn_name(&txn_name, txn, pool));
> -  SVN_ERR(svn_repos__hooks_pre_commit(repos, txn_name, pool));
> +  SVN_ERR(svn_repos__hooks_pre_commit(repos, hooks_env, txn_name, pool));
>  
>    /* Remove any ephemeral transaction properties. */
>    SVN_ERR(svn_fs_txn_proplist(&props, txn, pool));
> @@ -85,7 +90,8 @@ svn_repos_fs_commit_txn(const char **con
>      return err;
>  
>    /* Run post-commit hooks. */
> -  if ((err2 = svn_repos__hooks_post_commit(repos, *new_rev, txn_name, pool)))
> +  if ((err2 = svn_repos__hooks_post_commit(repos, hooks_env,
> +                                           *new_rev, txn_name, pool)))
>      {
>        err2 = svn_error_create
>                 (SVN_ERR_REPOS_POST_COMMIT_HOOK_FAILED, err2,
> @@ -110,6 +116,11 @@ svn_repos_fs_begin_txn_for_commit2(svn_f
>    apr_array_header_t *revprops;
>    const char *txn_name;
>    svn_string_t *author = svn_hash_gets(revprop_table, SVN_PROP_REVISION_AUTHOR);
> +  apr_hash_t *hooks_env;
> +
> +  /* Parse the hooks-env file (if any). */
> +  SVN_ERR(svn_repos__parse_hooks_env(&hooks_env, repos->hooks_env_path,
> +                                     pool, pool));
>  
>    /* Begin the transaction, ask for the fs to do on-the-fly lock checks.
>       We fetch its name, too, so the start-commit hook can use it.  */
> @@ -124,7 +135,8 @@ svn_repos_fs_begin_txn_for_commit2(svn_f
>    SVN_ERR(svn_repos_fs_change_txn_props(*txn_p, revprops, pool));
>  
>    /* Run start-commit hooks. */
> -  SVN_ERR(svn_repos__hooks_start_commit(repos, author ? author->data : NULL,
> +  SVN_ERR(svn_repos__hooks_start_commit(repos, hooks_env,
> +                                        author ? author->data : NULL,
>                                          repos->client_capabilities, txn_name,
>                                          pool));
>    return SVN_NO_ERROR;
> @@ -317,6 +329,7 @@ svn_repos_fs_change_rev_prop4(svn_repos_
>      {
>        const svn_string_t *old_value;
>        char action;
> +      apr_hash_t *hooks_env = NULL;
>  

IMO would be better not to initialise this at declaration: all codepaths
that use the value also initialise it first.

The same might apply to the load-fs-vtable.c:close_revision() changes.

> Modified: subversion/trunk/subversion/libsvn_repos/repos.h
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_repos/repos.h?rev=1464122&r1=1464121&r2=1464122&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_repos/repos.h (original)
> +++ subversion/trunk/subversion/libsvn_repos/repos.h Wed Apr  3 17:51:56 2013
> @@ -161,9 +161,25 @@ struct svn_repos_t
>  
>  /*** Hook-running Functions ***/
>  
> +/* Set *HOOKS_ENV_P to the parsed contents of the hooks-env file
> +   LOCAL_ABSPATH, allocated in RESULT_POOL.  (This result is suitable
> +   for delivery to the various hook wrapper functions which accept a
> +   'hooks_env' parameter.)
> +

As noted on IRC, suggesting to promise setting HOOKS_ENV_P to NULL if
local_abspath is NULL, since callers depend on that.

> +   Use SCRATCH_POOL for temporary allocations.  */
> +svn_error_t *
> +svn_repos__parse_hooks_env(apr_hash_t **hooks_env_p,
> +                           const char *local_abspath,
> +                           apr_pool_t *result_pool,
> +                           apr_pool_t *scratch_pool);
> +

Mime
View raw message