Re: svn commit: r999507 - in /subversion/trunk/subversion: svnrdump/dump_editor.c tests/cmdline/svnrdump_tests.py

Hi Daniel,

Thanks for the review.

Daniel Shahaf writes:
> > - /* Disallow a path relative to nothing. */
> > - SVN_ERR_ASSERT_NO_RETURN(!path || pb);
> > -
>
> Why did you drop this assertion?

I thought about it for a bit, and I can't seem to figure out why I
added it in the first place. My current sanity logic: Without a pb,
this function (make_dir_baton) generates a root baton. Whatever `path`
the user passes is ignored anyway- the root baton has `abspath` set to
`/`. With a `pb`, I MUST pass a non-null `path` -- there's probably
something about this I haven't thought about properly that's making
test 14 fail (there's an empty Node-path there). Either way, I don't
see where an assert fits into the picture.

> Also; if you intend to move it back, SVN_ERR_ASSERT() would be better
> (for when this code becomes a library function).

Ok, I'll keep that in mind when I write more asserts.

> > /* Construct the full path of this node. */
> > if (pb)
> > abspath = svn_uri_join("/", path, pool);
> > else
> > - abspath = "/";
> > + abspath = apr_pstrdup(pool, "/");
> >
>
> It is most likely unnecessary to duplicate a static string constant. :-)

True. However, abspath is a `char *` with no memory allocated to it: I
want whatever its value is to be allocated in `pool` since it's passed
around.

-- Ram
Received on 2010-09-21 20:56:31 CEST