Re: svn commit: rev 2024...

Yuck. This review is taking forEVER. I'm sure to have missed some
optimizations and such, since I reviewed only the patch, not generally
noticing whatever context was absent from the path itself. Also, I
still have about 4000 lines of code left to review. But here are some
things I found in the (what? 12000 lines of) code that I have
reviewed thus far.

--
>  svn_error_t *
> -svn_wc_process_committed (svn_stringbuf_t *path,
> +svn_wc_process_committed (const char *path,
>                            svn_boolean_t recurse,
>                            svn_revnum_t new_revnum,
>                            const char *rev_date,
> @@ -203,7 +193,8 @@
>  {
>    svn_error_t *err;
>    apr_status_t apr_err;
> -  svn_stringbuf_t *log_parent, *logtags, *base_name;
> +  const char *log_parent, *base_name;
> +  svn_stringbuf_t *logtags;
>    apr_file_t *log_fp = NULL;
>    char *revstr = apr_psprintf (pool, "%" SVN_REVNUM_T_FMT, new_revnum);
>    svn_stringbuf_t *checksum = NULL;
> @@ -217,7 +208,7 @@
>  
>    /* (First, try to write a logfile directly in PATH.) */
>    log_parent = path;
> -  base_name = svn_stringbuf_create (SVN_WC_ENTRY_THIS_DIR, pool);
> +  base_name = apr_pstrdup (pool, SVN_WC_ENTRY_THIS_DIR);
>    err = svn_wc__open_adm_file (&log_fp, log_parent, SVN_WC__ADM_LOG,
>                                 (APR_WRITE | APR_APPEND | APR_CREATE),
>                                 pool);
This should be just 'base_name = SVN_WC_ENTRY_THIS_DIR;'  No need to
dup it.
> -svn_stringbuf_t *
> -svn_wc__text_base_path (const svn_stringbuf_t *path,
> +const char *
> +svn_wc__text_base_path (const char *path,
>                          svn_boolean_t tmp,
>                          apr_pool_t *pool)
>  {
> -  svn_stringbuf_t *newpath, *base_name;
> -  svn_path_split (path, &newpath, &base_name, pool);
> -  extend_with_adm_name (newpath,
> -                        SVN_WC__BASE_EXT,
> -                        0,
> -                        pool,
> -                        tmp ? SVN_WC__ADM_TMP : "",
> -                        SVN_WC__ADM_TEXT_BASE,
> -                        base_name->data,
> -                        NULL);
> -    
> -  return newpath;
> +  const char *newpath, *base_name;
> +
> +  svn_path_split_nts (path, &newpath, &base_name, pool);
> +  return extend_with_adm_name (newpath,
> +                               SVN_WC__BASE_EXT,
> +                               0,
> +                               pool,
> +                               tmp ? SVN_WC__ADM_TMP : "",
> +                               SVN_WC__ADM_TEXT_BASE,
> +                               base_name,
> +                               NULL);
>  }
The extend_with_adm_name() function's third argument is a boolean
which denotes whether or not to construct the regular admin path for a
thing, or the temporary admin path for that thing.  So, I think that
this should just be:
   return extend_with_adm_name (newpath, 
                                SVN_WC__BASE_EXT, 
                                tmp, 
                                pool,
                                SVN_WC__ADM_TEXT_BASE, 
                                base_name,
                                NULL);
>  static svn_error_t *
> -prop_path_internal (svn_stringbuf_t **prop_path,
> -                    const svn_stringbuf_t *path,
> +prop_path_internal (const char **prop_path,
> +                    const char *path,
>                      svn_boolean_t base,
>                      svn_boolean_t tmp,
>                      apr_pool_t *pool)
[...]
> @@ -436,16 +386,17 @@
>          return svn_error_createf
>            (SVN_ERR_WC_OBSTRUCTED_UPDATE, 0, NULL, pool,
>             "svn_wc__prop_path: %s is not a working copy directory",
> -           (*prop_path)->data);
> +           *prop_path);
>  
> -      extend_with_adm_name (*prop_path,
> -                            base ? SVN_WC__BASE_EXT : NULL,
> -                            0,
> -                            pool,
> -                            tmp ? SVN_WC__ADM_TMP : "",
> -                            base ? SVN_WC__ADM_PROP_BASE : SVN_WC__ADM_PROPS,
> -                            entry_name->data,
> -                            NULL);
> +      *prop_path = extend_with_adm_name
> +        (*prop_path,
> +         base ? SVN_WC__BASE_EXT : NULL,
> +         0,
> +         pool,
> +         tmp ? SVN_WC__ADM_TMP : "",
> +         base ? SVN_WC__ADM_PROP_BASE : SVN_WC__ADM_PROPS,
> +         entry_name,
> +         NULL);
>      }
Same here (use extend_with_adm_name to its full potential).
>  svn_error_t *
> -svn_wc__wcprop_path (svn_stringbuf_t **wcprop_path,
> -                     const svn_stringbuf_t *path,
> +svn_wc__wcprop_path (const char **wcprop_path,
> +                     const char *path,
>                       svn_boolean_t tmp,
>                       apr_pool_t *pool)
>  {
>    svn_error_t *err;
>    enum svn_node_kind kind;
>    svn_boolean_t is_wc;
> -  svn_stringbuf_t *entry_name;
> +  const char *entry_name;
>  
> -  err = svn_io_check_path (path->data, &kind, pool);
> +  err = svn_io_check_path (path, &kind, pool);
>    if (err)
>      return err;
>  
> @@ -483,19 +434,17 @@
>  
>    if (is_wc)  /* It's not only a dir, it's a working copy dir */
>      {
> -      *wcprop_path = svn_stringbuf_dup (path, pool);
> -      extend_with_adm_name 
> -        (*wcprop_path,
> -         NULL,
> -         0,
> -         pool,
> -         tmp ? SVN_WC__ADM_TMP : "",
> -         SVN_WC__ADM_DIR_WCPROPS,
> -         NULL);
> +      *wcprop_path = extend_with_adm_name (path,
> +                                           NULL,
> +                                           0,
> +                                           pool,
> +                                           tmp ? SVN_WC__ADM_TMP : "",
> +                                           SVN_WC__ADM_DIR_WCPROPS,
> +                                           NULL);
...and here, too.
>      }
>    else  /* It's either a file, or a non-wc dir (i.e., maybe an ex-file) */
>      {
> -      svn_path_split (path, wcprop_path, &entry_name, pool);
> +      svn_path_split_nts (path, wcprop_path, &entry_name, pool);
>   
>        err = svn_wc_check_wc (*wcprop_path, &is_wc, pool);
>        if (err)
> @@ -503,17 +452,16 @@
>        else if (! is_wc)
>          return svn_error_createf
>            (SVN_ERR_WC_OBSTRUCTED_UPDATE, 0, NULL, pool,
> -           "wcprop_path: %s is not a working copy directory",
> -           (*wcprop_path)->data);
> +           "wcprop_path: %s is not a working copy directory", *wcprop_path);
>  
> -      extend_with_adm_name (*wcprop_path,
> -                            NULL,
> -                            0,
> -                            pool,
> -                            tmp ? SVN_WC__ADM_TMP : "",
> -                            SVN_WC__ADM_WCPROPS,
> -                            entry_name->data,
> -                            NULL);
> +      *wcprop_path = extend_with_adm_name (*wcprop_path,
> +                                           NULL,
> +                                           0,
> +                                           pool,
> +                                           tmp ? SVN_WC__ADM_TMP : "",
> +                                           SVN_WC__ADM_WCPROPS,
> +                                           entry_name,
> +                                           NULL);
>      }
...again.
>  svn_error_t *
> -svn_wc__adm_destroy (svn_stringbuf_t *path, apr_pool_t *pool)
> +svn_wc__adm_destroy (const char *path, apr_pool_t *pool)
>  {
>    /* Try to lock the admin directory, hoping that this function will
>       eject an error if we're already locked (which is fine, cause if
> @@ -1326,30 +1221,19 @@
>    SVN_ERR (svn_wc_lock (path, 0, pool));
>  
>    /* Well, I think the coast is clear for blowing away this directory
> -     (which should also remove the lock file we created above) */
> -  {
> -    svn_error_t *err;
> -    svn_stringbuf_t *adm_path = svn_stringbuf_dup (path, pool);
> -
> -    svn_path_add_component (adm_path, svn_wc__adm_subdir (pool));
> -
> -    err = svn_io_remove_dir (adm_path->data, pool);
> -    if (err)
> -      return svn_error_createf
> -        (err->apr_err, err->src_err, err, err->pool,
> -         "error removing administrative directory for %s",
> -         path->data);
> -  }
> +     (which will also remove the lock file we created above) */
> +  path = svn_path_join (path, adm_subdir (), pool);
> +  SVN_ERR (svn_io_remove_dir (path, pool));
>  
>    return SVN_NO_ERROR;
>  }
Did you specifically mean to remove the wrapping of the error from
svn_io_remove_dir?  If not, just use the SVN_ERR_W macro here.
>  svn_error_t *
> -svn_wc__adm_cleanup_tmp_area (svn_stringbuf_t *path, apr_pool_t *pool)
> +svn_wc__adm_cleanup_tmp_area (const char *path, apr_pool_t *pool)
>  {
>    svn_boolean_t was_locked;
> -  svn_stringbuf_t *tmp_path;
> -  svn_error_t *err;
> +  const char *tmp_path;
>  
>    /* Lock the admin area if it's not already locked. */
>    SVN_ERR (svn_wc_locked (&was_locked, path, pool));
> @@ -1357,15 +1241,8 @@
>      SVN_ERR (svn_wc_lock (path, 0, pool));
>  
>    /* Get the path to the tmp area, and blow it away. */
> -  tmp_path = svn_stringbuf_dup (path, pool);
> -  extend_with_adm_name (tmp_path, NULL, 0, pool, SVN_WC__ADM_TMP, NULL);
> -
> -  err = svn_io_remove_dir (tmp_path->data, pool);
> -  if (err)
> -    return svn_error_createf
> -      (err->apr_err, err->src_err, err, err->pool,
> -       "error removing tmp area in administrative directory for %s",
> -       path->data);
> +  tmp_path = extend_with_adm_name (path, NULL, 0, pool, SVN_WC__ADM_TMP, NULL);
> +  SVN_ERR (svn_io_remove_dir (tmp_path, pool));
Same thing here (SVN_ERR_W ?).
> Modified: trunk/subversion/clients/cmdline/help-cmd.c
> ==============================================================================
> --- trunk/subversion/clients/cmdline/help-cmd.c	(original)
> +++ trunk/subversion/clients/cmdline/help-cmd.c	Tue May 28 16:19:26 2002
> @@ -81,8 +81,8 @@
>    if (targets && targets->nelts)  /* help on subcommand(s) requested */
>      for (i = 0; i < targets->nelts; i++)
>        {
> -        svn_stringbuf_t *this = (((svn_stringbuf_t **) (targets)->elts))[i];
> -	svn_cl__subcommand_help (this->data, pool);
> +        const char *this = (((const char **) (targets)->elts))[i];
> +	svn_cl__subcommand_help (this, pool);
>        }
>    else if (opt_state && opt_state->version)  /* just -v or --version */
>      SVN_ERR (print_version_info (pool));        
> 
Hm...for some reason this file has an actual TAB character in it.  Not
your bug, of course, but would be nice to see fixed.
--
I plan to finish this review tomorrow.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org