Re: [PATCH] Add svnrdump

Hi Bert,

Thank you for the review.

Bert Huijben writes:
> > +svn_error_t *open_root(void *edit_baton,
> > + svn_revnum_t base_revision,
> > + apr_pool_t *pool,
> > + void **root_baton)
>
> Static and the return type on its own line.

Fixed. Sorry about the sloppy error.

> This looks like more than 80 characters to me.

I didn't realize that it was a strict requirement. Fixed now.

> > + if (pb && ARE_VALID_COPY_ARGS(pb->cmp_path, pb->cmp_rev)) {
> > + APR_ARRAY_PUSH(compose_path, const char *) = pb->cmp_path;
> > + APR_ARRAY_PUSH(compose_path, const char *) =
> > svn_dirent_basename(path, pool);
>
> Assuming that the path doesn't start with a '/' here, this should be
> svn_relent_basename() to avoid platform specific path rules.

Where is svn_dirent_basename defined? I can't seem to find it in the
codebase at all.

> > + hb->temp_filepath = apr_psprintf(eb->pool, "%s/svn-fe-XXXXXX",
> > tempdir);
>
> Why store this path in the editor pool? Do you really need this XXXX path to
> live that long?

Excellent catch! :) Fixed now.

> > +svn_error_t *
> > +get_dump_editor(const svn_delta_editor_t **editor,
> > + void **edit_baton,
> > + svn_revnum_t to_rev,
> > + apr_pool_t *pool);
>
> These structs and this function don't follow our naming guidelines for
> libraries. But these functions are no reusable library (yet).

Right. Is it alright then? Can I re-submit the patch now? (Also fixed
a couple of things Daniel pointed out).

-- Ram
Received on 2010-07-09 15:41:27 CEST