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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html