Re: svn commit: r10189 - trunk/subversion/libsvn_client

[kf...@collab.net]

lundblad@tigris.org writes:
> Log:
> Reimplement the blame command in terms of the new get_file_revs RA
> functionality.  Finishes issue #1715 to speed up blame.
> 
> * libsvn_client/blame.c (diff_baton): Removed.  Users converted to use
>   file_rev_baton.
>   (file_rev_baton, delta_baton): New struct.
>   (add_file_blame, window_handler, check_mimetype, file_rev_handler,
>   old_blame): New function.

You probably meant plurals: "New structs", "New functions".

>  struct rev
>  {
>    svn_revnum_t revision; /* the revision number */
>    const char *author;    /* the author of the revision */
>    const char *date;      /* the date of the revision */
> +  /* Only used by the old code. */
>    const char *path;      /* the absolute repository path */
>    struct rev *next;      /* the next revision */
>  };

The word "old" won't be very meaningful once the memory of the old way
fades from the collective consciousness.  You might want to say
"pre-1.1" instead, or something like that.

> +/* The baton used by the txdelta window hanlder. */
> +struct delta_baton {
> +  /* Our underlying handler/baton that we wrap */
> +  svn_txdelta_window_handler_t wrapped_handler;
> +  void *wrapped_baton;
> +  struct file_rev_baton *file_rev_baton;
> +  apr_file_t *file;  /* the result of the delta */
> +  const char *filename;
> +};

"handler" not "hanlder", in doc string.

> +static apr_status_t
> +cleanup_tempfile (void *f)
> +{
> +  apr_file_t *file = f;
> +  apr_status_t apr_err;
> +  const char *fname;
> +
> +  /* the file may or may not have been closed; try it */
> +  apr_file_close (file);
> +
> +  apr_err = apr_file_name_get (&fname, file);
> +  if (apr_err == APR_SUCCESS)
> +    apr_err = apr_file_remove (fname, apr_file_pool_get (file));
> +
> +  return apr_err;
> +}

Document API even for internal functions.  For example, this one
should say that it closes the file unconditionally, but returns only
the error (if any) from the removal, not the close.

> +/* Throw an error if PROP_DIFFS indicates a binary MIME type. Else, return
> +   SVN_NO_ERROR. */
> +static svn_error_t *
> +check_mimetype (apr_array_header_t *prop_diffs, const char *target,
> +                apr_pool_t *pool)
> +{
> +  int i;
> +
> +  for (i = 0; i < prop_diffs->nelts; ++i)
> +    {
> +      const svn_prop_t *prop = &APR_ARRAY_IDX(prop_diffs, i, svn_prop_t);
> +      if (strcmp (prop->name, SVN_PROP_MIME_TYPE) == 0
> +          && prop->value
> +          && svn_mime_type_is_binary (prop->value->data))
> +        return svn_error_createf 
> +          (SVN_ERR_CLIENT_IS_BINARY_FILE, 0,
> +           _("Cannot calculate blame information for binary file '%s'"),
> +           target);
> +    }
> +  return SVN_NO_ERROR;
> +}

Might as well say which error, exactly.

Same thing about 'svn_prop_t' versus 'svn_prop_t *', by the way.

> +/* This is used when there is no get_file_revs avaliable. */
> +static svn_error_t *
> +old_blame (const char *target, const char *url,
> +           svn_ra_plugin_t *ra_lib,
> +           void *session,
> +           struct file_rev_baton *frb)
> +{

"available"

Again, nice work -- all this code review is in appreciation of your
taking on such a big task.

-Karl


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