Re: svn commit: rev 7438 - trunk/subversion/libsvn_client

mbk@tigris.org writes:

> Author: mbk
> Date: Fri Oct 17 17:10:23 2003
> New Revision: 7438

> Modified: trunk/subversion/libsvn_client/blame.c
> ==============================================================================
> --- trunk/subversion/libsvn_client/blame.c (original)
> +++ trunk/subversion/libsvn_client/blame.c Fri Oct 17 17:10:23 2003

> svn_error_t *
> -svn_client_blame (const char *path_or_url,
> +svn_client_blame (const char *target,
> const svn_opt_revision_t *start,
> const svn_opt_revision_t *end,
> svn_boolean_t strict_node_history,
> @@ -295,19 +297,23 @@
> svn_client_ctx_t *ctx,
> apr_pool_t *pool)
> {
> + const char *reposURL;
> struct log_message_baton lmb;
> apr_array_header_t *condensed_targets;
> - svn_ra_plugin_t *ra_lib;
> - void *ra_baton, *log_session, *text_session;
> + svn_ra_plugin_t *ra_lib;
> + void *ra_baton, *session;
> const char *url;
> const char *auth_dir;
> svn_revnum_t start_revnum, end_revnum;
> struct blame *walk;
> - apr_file_t *last_file;
> - svn_stream_t *last_stream;
> - apr_pool_t *subpool;
> + apr_file_t *file;
> + svn_stream_t *stream;
> + apr_pool_t *subpool, *diffpool;
> + struct rev *rev;
> apr_status_t apr_err;
> -
> + svn_node_kind_t kind;
> + struct diff_baton db;
> + const char *last = NULL;
>
> if (start->kind == svn_opt_revision_unspecified
> || end->kind == svn_opt_revision_unspecified)
> @@ -316,11 +322,12 @@
> "svn_client_blame: caller failed to supply revisions");
>
> subpool = svn_pool_create (pool);
> + diffpool = svn_pool_create (pool);

Are two sub-pools really needed? Generally a function is expected to
use the pool it is given, it only needs a sub-pool for iterations. Of
the two, subpool looks to be redundant, given that it is never cleared
within the function.

> + for (rev = lmb.eldest; rev; rev = rev->next)
> + {
> + const char *tmp;
> + SVN_ERR (svn_io_open_unique_file (&file, &tmp, "", ".tmp",
> + FALSE, subpool));
> + stream = svn_stream_from_aprfile (file, pool);
> + SVN_ERR (ra_lib->get_file (session, rev->path + 1, rev->revision,
> + stream, NULL, NULL, subpool));
> + SVN_ERR (svn_stream_close (stream));
> + apr_err = apr_file_close (file);
> + if (apr_err != APR_SUCCESS)
> + return svn_error_createf (apr_err, NULL, "error closing %s",
> + rev->path);
> + if (last)
> + {
> + svn_diff_t *diff;
> + db.rev = rev;
> + apr_pool_clear (diffpool);
> + SVN_ERR (svn_diff_file_diff (&diff, last, tmp, diffpool));
> + SVN_ERR (svn_diff_output (diff, &db, &output_fns));
> + apr_err = apr_file_remove (last, diffpool);
> + if (apr_err != APR_SUCCESS)
> + return svn_error_createf (apr_err, NULL, "error removing %s",
> + last);
> + }
> + else
> + db.blame = blame_create (&db, rev, 0);
> +
> + last = tmp;
> + }
> + apr_pool_destroy (diffpool);

It might be best to destroy diffpool at the end of the function...

> +
> + apr_err = apr_file_open (&file, last, APR_READ, APR_OS_DEFAULT, subpool);
> if (apr_err != APR_SUCCESS)
> - return svn_error_createf (apr_err, NULL, "error opening %s",
> - lmb.last->path);
> + return svn_error_createf (apr_err, NULL, "error opening %s", last);
>
> - last_stream = svn_stream_from_aprfile (last_file, subpool);
> - for (walk = lmb.db.blame; walk; walk = walk->next)
> + stream = svn_stream_from_aprfile (file, subpool);
> + for (walk = db.blame; walk; walk = walk->next)
> {
> int i;
> for (i = walk->start; !walk->next || i < walk->next->start; i++)
> {
> svn_stringbuf_t *sb;
> - SVN_ERR (svn_stream_readline (last_stream, &sb, subpool));
> + SVN_ERR (svn_stream_readline (stream, &sb, subpool));
> if (! sb)
> break;
> SVN_ERR (receiver (receiver_baton, i, walk->rev->revision,
> @@ -393,15 +438,14 @@
> }
> }
>
> - SVN_ERR (svn_stream_close (last_stream));
> - apr_err = apr_file_close (last_file);
> + SVN_ERR (svn_stream_close (stream));
> + apr_err = apr_file_close (file);
> if (apr_err != APR_SUCCESS)
> - return svn_error_createf (apr_err, NULL, "error closing %s",
> - lmb.last->path);
> - apr_err = apr_file_remove (lmb.last->path, subpool);
> + return svn_error_createf (apr_err, NULL, "error closing %s", last);
> + apr_err = apr_file_remove (last, diffpool);

as then there will be no opportunity to use it after it has been
destroyed.

> if (apr_err != APR_SUCCESS)
> - return svn_error_createf (apr_err, NULL, "error removing %s",
> - lmb.last->path);
> + return svn_error_createf (apr_err, NULL, "error removing %s", last);
> +
> apr_pool_destroy (subpool);
> return SVN_NO_ERROR;

-- 
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org