Re: [PATCH] Add svnrdump

From: Ramkumar Ramachandra <artagnon@gmail.com>
Date: Jul 9, 2010, 13:42 UTC
Message-ID: <20100709134228.GB12315@debian>
In-Reply-To: <002d01cb1e7f$e0ff03c0$a2fd0b40$@nl>

Hi Bert,

Thank you for the review.

Bert Huijben writes:

Show 6 quoted lines

> > +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.

Show 7 quoted lines

> > +  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.

Show 5 quoted lines

> > +  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.

Show 8 quoted lines

> > +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