Re: [PATCH 05/13] Drive the debug editor

[Ramkumar Ramachandra <artagnon@gmail.com>]

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