Since Karl wondered if there was unnecessary code duplication here, I took a
look to see if I could factor out some of lock_many() and unlock_many(). I
couldn't usefully do so, but while comparing the two functions side by side I
saw many cosmetic differences between them, and one apparently unintended code
difference.
VK Sameer wrote:
> Index: subversion/svnserve/serve.c
> ===================================================================
> --- subversion/svnserve/serve.c (revision 14823)
> +++ subversion/svnserve/serve.c (working copy)
> @@ -1407,6 +1407,78 @@
> return SVN_NO_ERROR;
> }
>
> +static svn_error_t *lock_many(svn_ra_svn_conn_t *conn, apr_pool_t *pool,
> + apr_array_header_t *params, void *baton)
> +{
> + apr_array_header_t *locks;
> + apr_array_header_t *lock_cmds;
> + server_baton_t *b = baton;
> + const char *comment;
> + svn_boolean_t force;
> + int i;
Cosmetic differences don't really matter much, but they include the local
variables being declared in a different order...
> + struct lock_cmd {
> + const char *path;
> + const char *full_path;
> + svn_revnum_t current_rev;
> + svn_lock_t *l;
> + };
> +
> + SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "(?c)bl", &comment, &force,
> + &locks));
> +
> + lock_cmds =
> + apr_array_make(pool, locks->nelts, sizeof(struct lock_cmd));
> +
> + /* Loop through the lock commands */
... and comments that exist in one function but not the other ...
> + for (i = 0; i < locks->nelts; ++i)
> + {
> + struct lock_cmd *cmd = apr_pcalloc(pool, sizeof(struct lock_cmd));
> + svn_ra_svn_item_t *item =
> + &APR_ARRAY_IDX(locks, i, svn_ra_svn_item_t);
> +
> + if (item->kind != SVN_RA_SVN_LIST)
> + return svn_error_create(SVN_ERR_RA_SVN_MALFORMED_DATA, NULL,
> + "Lock commands should be list of lists\n");
> +
> + SVN_ERR(svn_ra_svn_parse_tuple(item->u.list, pool, "c(?r)",
> + &cmd->path, &cmd->current_rev));
> +
> + cmd->full_path = svn_path_join(b->fs_path,
> + svn_path_canonicalize(cmd->path, pool),
> + pool);
> +
> + APR_ARRAY_PUSH(lock_cmds, struct lock_cmd) = *cmd;
> + }
> +
> + SVN_ERR(must_have_write_access(conn, pool, b, TRUE));
> +
> + /* Loop through each path to be locked */
> + for (i = 0; i < lock_cmds->nelts; i++)
> + {
> + struct lock_cmd *cmd =
> + &APR_ARRAY_IDX(lock_cmds, i, struct lock_cmd);
> +
> + SVN_CMD_ERR(
> + svn_repos_fs_lock(&cmd->l, b->repos, cmd->full_path,
> + NULL, comment, 0,
> + 0, /* No expiration time. */
> + cmd->current_rev,
> + force, pool));
> + }
> +
> + /* (success( (ccc(?c)c(?c) ... )) */
> + SVN_ERR(svn_ra_svn_write_tuple(conn, pool, "w(!", "success"));
> + for (i = 0; i < lock_cmds->nelts; i++)
> + {
> + struct lock_cmd *cmd =
> + &APR_ARRAY_IDX(lock_cmds, i, struct lock_cmd);
> + SVN_ERR(write_lock(conn, pool, cmd->l));
> + }
> + SVN_ERR(svn_ra_svn_write_tuple(conn, pool, "!)"));
> +
> + return SVN_NO_ERROR;
> +}
> +
> static svn_error_t *unlock(svn_ra_svn_conn_t *conn, apr_pool_t *pool,
> apr_array_header_t *params, void *baton)
> {
> @@ -1419,7 +1491,6 @@
>
> full_path = svn_path_join(b->fs_path, svn_path_canonicalize(path, pool),
> pool);
> -
(Was that space change intentional?)
> /* Username required unless force was specified. */
> SVN_ERR(must_have_write_access(conn, pool, b, ! force));
>
> @@ -1430,6 +1501,61 @@
> return SVN_NO_ERROR;
> }
>
> +static svn_error_t *unlock_many(svn_ra_svn_conn_t *conn, apr_pool_t *pool,
> + apr_array_header_t *params, void *baton)
> +{
> + server_baton_t *b = baton;
> + svn_boolean_t force;
> + apr_array_header_t *unlock_tokens;
> + apr_array_header_t *unlock_cmds;
> + struct unlock_cmd {
> + const char *path;
> + const char *token;
> + const char *full_path;
> + };
> + int i;
> +
> + SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "bl",&force, &unlock_tokens));
... and spaces missing before "&force" here ...
> +
> + unlock_cmds =
> + apr_array_make(pool, unlock_tokens->nelts, sizeof(struct unlock_cmd));
> +
> + for (i = 0; i< unlock_tokens->nelts; i++)
> + {
> + struct unlock_cmd *cmd = apr_pcalloc(pool,sizeof(struct unlock_cmd));
... and before "sizeof" here ...
> + svn_ra_svn_item_t *item = &APR_ARRAY_IDX(unlock_tokens, i,
> + svn_ra_svn_item_t);
> +
> + if (item->kind != SVN_RA_SVN_LIST)
> + return svn_error_create(SVN_ERR_RA_SVN_MALFORMED_DATA, NULL,
> + "Unlock command should be a list of lists\n");
> +
> + SVN_ERR(svn_ra_svn_parse_tuple(item->u.list, pool, "c(?c)",
> + &cmd->path, &cmd->token));
> +
> + cmd->full_path = svn_path_join(b->fs_path,
> + svn_path_canonicalize(cmd->path, pool),pool);
... and before "pool" here.
> +
> + APR_ARRAY_PUSH(unlock_cmds, struct unlock_cmd) = *cmd;
> + }
> +
> + /* Username required unless force was specified. */
> + SVN_ERR(must_have_write_access(conn, pool, b, ! force));
> +
> + for (i = 0; i < unlock_tokens->nelts; i++)
Here is the code difference: this loops over the number of elements in
"unlock_tokens" ...
> + {
> + struct unlock_cmd *cmd = &APR_ARRAY_IDX(unlock_cmds, i,
... but it is actually indexing into "unlock_cmds". They will have the same
number of elements, so it's not exactly a bug, just confusing.
> + struct unlock_cmd);
> +
> + SVN_CMD_ERR(svn_repos_fs_unlock(b->repos, cmd->full_path,
> + cmd->token, force, pool));
> + }
> +
> + SVN_ERR(svn_ra_svn_write_cmd_response(conn, pool, ""));
> +
> + return SVN_NO_ERROR;
> +}
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue May 31 21:19:50 2005