Re: [PATCH 05/13] Drive the debug editor
- From
Ramkumar Ramachandra <artagnon@gmail.com>
- Date
- Jul 7, 2010, 19:08 UTC
- Message-ID
- <20100707190813.GA16065@debian>
- In-Reply-To
- <20100707182631.GB2617@burratino>
Hi Jonathan,
Jonathan Nieder writes:
Show 11 quoted lines
> 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?
Show 23 quoted lines
> [...]
> > +++ 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.
Show 18 quoted lines
> > +++ 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.
Show 17 quoted lines
> > + 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.
Show 5 quoted lines
> > + 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