cmpilato@collab.net writes:
> > /* (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.
Thanks, good catch.
(In general, do you want me to take care of these, or will you be
committing the fixes? I'm happy to do it if it's easier for you to
just fire off an email and forget about it. :-).
> > + 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);
D'oh!
> 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.
It was deliberate, here and in the other case you mentioned, yes. The
wrapping wasn't adding any more real information.
> 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.
Yup.
> I plan to finish this review tomorrow.
Cool. Thanks so much. The only thing more painful than writing this
change would be... reviewing it.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Jun 4 00:23:58 2002