Hi Jonathan,
Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
>
> > +++ b/dump_editor.c
> > @@ -128,7 +128,7 @@ svn_error_t *get_dump_editor(const svn_delta_editor_t **editor,
> > de->close_directory = close_directory;
> > de->change_dir_prop = change_dir_prop;
> > de->change_file_prop = change_file_prop;
> > - de->apply_textdelta = apply_textdelta;
> > + /* de->apply_textdelta = apply_textdelta; */
>
> Hmm...
Without this, the program segfaults because the necessary setup for
applying a text delta hasn't been set up. Perhaps I should explain
this in my commit message?
> [...]
> > +++ b/dumpr_util.h
> > @@ -1,6 +1,11 @@
> > #ifndef DUMPR_UTIL_H_
> > #define DUMPR_UTIL_H_
> >
> > +struct replay_baton {
> > + const svn_delta_editor_t *editor;
> > + void *baton;
> > +};
> > +
>
> Context during svnsync-like replay ops:
>
> - a diff replayer
> - its context object
>
> Maybe "void *edit_baton" would be clearer.
>
> > struct edit_baton {
>
> Which might involve renaming this to dump_edit_baton to avoid
> confusion.
Done. I renamed both.
> > +++ b/svndumpr.c
> > @@ -8,10 +8,40 @@
> [...]
> > +static svn_error_t *replay_revstart(svn_revnum_t revision,
> > + void *replay_baton,
> > + const svn_delta_editor_t **editor,
> > + void **edit_baton,
> > + apr_hash_t *rev_props,
> > + apr_pool_t *pool)
>
> This function is called to acquire an editor to replay one revision.
>
> > +{
> > + /* Extract editor and editor_baton from the replay_baton and
> > + set them so that the editor callbacks can use them */
>
> This comment just paraphrases the code. What in particular requires
> explanation here?
This concept took me some time to wrap my head around: I had to stuff
the replay_baton with the editor/ editor_baton so that I could set
them for use in the callback functions. Comment moved to a later
patch.
> > + struct replay_baton *rb = replay_baton;
> > + *editor = rb->editor;
> > + *edit_baton = rb->baton;
> > +
> > + return SVN_NO_ERROR;
> > +}
>
> [...]
> > @@ -47,6 +77,25 @@ svn_error_t *open_connection(const char *url)
> >
> > svn_error_t *replay_range(svn_revnum_t start_revision, svn_revnum_t end_revision)
> > {
> [...]
> > + SVN_ERR(svn_cmdline_printf(pool, SVN_REPOS_DUMPFILE_MAGIC_HEADER ": %d\n",
> > + SVN_REPOS_DUMPFILE_FORMAT_VERSION));
>
> Did this sneak in from a later patch?
Yes. Fixed now. I moved it this change to the next patch.
> > + SVN_ERR(svn_ra_replay_range(session, start_revision, end_revision,
> > + 0, TRUE, replay_revstart, replay_revend,
> > + replay_baton, pool));
>
> Makes sense.
Thanks for the excellent review.
-- Ram
Received on 2010-07-07 21:08:10 CEST