Re: [PATCH] Correct documentation for svn_repos_node_editor and friends

[Daniel Shahaf <d....@daniel.shahaf.name>]

Erik Johansson wrote on Tue, Dec 14, 2010 at 19:19:17 +0100:
> On Tue, Dec 14, 2010 at 18:17, Julian Foad <ju...@wandisco.com> wrote:
> > Hi Erik.  The attachment didn't reach the mailing list.  This is
> > probably because the mailing list strips off attachments that aren't
> > marked as plain text.  Please could you try again, maybe changing the
> > patch's name to '*.txt' or trying some other trick to encourage your
> > client software to mark it as a plain-text MIME type.  Cheers.
> 
> Trying again...

> [[[
> r846201 added svn_repos_replay as a "replacement not so much for
> svn_repos_dir_delta, but for at least a whole class of functionality that
> svn_repos_dir_delta was never intended to handle."
> 
> One place where svn_repos_replay replaced svn_repos_dir_delta was in
> combination with svn_repos_node_editor (only used in svnlook). This commit
> updates the documentation for svn_repos_node_editor and friends to reflect
> this.
> 
> * subversion/include/svn_repos.h
>   (svn_repos_node_editor, svn_repos_node_from_baton): Doc update
> ]]]

The patch basically does s/svn_repos_dir_delta2/svn_repos_replay2/g.
However, despite the log message, I still don't understand why that is
a correct change.

Why are we mentioning a specific driver at all?  Can svn_repos_node_editor()
be driven only by svn_repos_replay2()?

(I don't see why that would be the case.)  If not, then its doc string
should avoid mention that the returned editor can be driven only by %s,
when in fact it can be driven by anything.

> Index: subversion/include/svn_repos.h
> ===================================================================
> --- subversion/include/svn_repos.h	(revision 1045318)
> +++ subversion/include/svn_repos.h	(working copy)
> @@ -2223,20 +2223,20 @@
>  /* ---------------------------------------------------------------*/
>  
>  /**
> - * @defgroup svn_repos_inspection Data structures and editor things for
> + * @defgroup svn_repos_inspection Data structures and editor things for \
>   * repository inspection.
>   * @{
>   *
> - * As it turns out, the svn_repos_dir_delta2() interface can be
> + * As it turns out, the svn_repos_replay2() interface can be
>   * extremely useful for examining the repository, or more exactly,
> - * changes to the repository.  svn_repos_dir_delta2() allows for
> + * changes to the repository.  svn_repos_replay2() allows for
>   * differences between two trees to be described using an editor.
>   *

We have three mechanisms: svn_repos_dir_delta2(), svn_repos_begin_report2(),
and svn_repos_replay2().  I think it would be useful to just mention all of
them --- after all, none of them is deprecated and all of them can represent
a tree delta using an editor.

>   * By using the editor obtained from svn_repos_node_editor() with
> - * svn_repos_dir_delta2(), the description of how to transform one tree
> + * svn_repos_replay2(), the description of how to transform one tree
>   * into another can be used to build an in-memory linked-list tree,
>   * which each node representing a repository node that was changed as a
> - * result of having svn_repos_dir_delta2() drive that editor.
> + * result of having svn_repos_replay2() drive that editor.
>   */
>  
>  /** A node in the repository. */
> @@ -2276,7 +2276,7 @@
>  
>  
>  /** Set @a *editor and @a *edit_baton to an editor that, when driven by
> - * svn_repos_dir_delta2(), builds an <tt>svn_repos_node_t *</tt> tree
> + * svn_repos_replay2(), builds an <tt>svn_repos_node_t *</tt> tree
>   * representing the delta from @a base_root to @a root in @a repos's
>   * filesystem.
>   *

> @@ -2300,7 +2300,7 @@
>  
>  /** Return the root node of the linked-list tree generated by driving
>   * the editor created by svn_repos_node_editor() with
> - * svn_repos_dir_delta2(), which is stored in @a edit_baton.  This is
> + * svn_repos_replay2(), which is stored in @a edit_baton.  This is
>   * only really useful if used *after* the editor drive is completed.
>   */
>  svn_repos_node_t *

What do you think?

Re: [PATCH] Correct documentation for svn_repos_node_editor and friends

[Daniel Shahaf <d....@daniel.shahaf.name>]

Erik Johansson wrote on Wed, Dec 22, 2010 at 22:00:22 +0100:
> On Sat, Dec 18, 2010 at 20:43, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> > Erik Johansson wrote on Tue, Dec 14, 2010 at 19:19:17 +0100:
> >> [[[
> >> r846201 added svn_repos_replay as a "replacement not so much for
> >> svn_repos_dir_delta, but for at least a whole class of functionality that
> >> svn_repos_dir_delta was never intended to handle."
> >>
> >> One place where svn_repos_replay replaced svn_repos_dir_delta was in
> >> combination with svn_repos_node_editor (only used in svnlook). This commit
> >> updates the documentation for svn_repos_node_editor and friends to reflect
> >> this.
> >>
> >> * subversion/include/svn_repos.h
> >>   (svn_repos_node_editor, svn_repos_node_from_baton): Doc update
> >> ]]]
> >
> > The patch basically does s/svn_repos_dir_delta2/svn_repos_replay2/g.
> > However, despite the log message, I still don't understand why that is
> > a correct change.
> 
> According to the log message for r846201 it seems like
> svn_repos_replay is faster than svn_repos_dir_delta for some
> operations. I can only assume that this is one such case.
> 

IOW, the patch recommends svn_repos_replay2() because that is newer API.

> > Why are we mentioning a specific driver at all?  Can svn_repos_node_editor()
> > be driven only by svn_repos_replay2()?
> >
> > (I don't see why that would be the case.)  If not, then its doc string
> > should avoid mention that the returned editor can be driven only by %s,
> > when in fact it can be driven by anything.
> 
> I don't know the anything about the other drivers, but my assumption
> is that any driver works, but svn_repos_replay is the prefered one in
> this case.
> 

*nod*

> > We have three mechanisms: svn_repos_dir_delta2(), svn_repos_begin_report2(),
> > and svn_repos_replay2().  I think it would be useful to just mention all of
> > them --- after all, none of them is deprecated and all of them can represent
> > a tree delta using an editor.
> 
> Mention all on equal footing or say that svn_repos_replay2 is the preferred one?
> 
> I can fix the docs, but I don't know enough about svn api to actually
> determine the correct fix. E.g. do all the drivers call methods in the
> editor in the same order?
> 

In theory, yes.  That's the core of the editor API: the
driver only knows the svn_delta_editor_t vtable, and the drivee also
knows only the vtable --- they need not (in general) know each other.

For example, 'svnrdump dump' drives a commit editor; but nothing in the
commit editor knows what dumpfiles are.

> // Erik
> 
> -- 
> Erik Johansson
> Home Page: http://ejohansson.se/
> PGP Key: http://ejohansson.se/erik.asc

Re: [PATCH] Correct documentation for svn_repos_node_editor and friends

[Daniel Shahaf <d....@daniel.shahaf.name>]

Erik Johansson wrote on Wed, Dec 22, 2010 at 22:00:22 +0100:
> On Sat, Dec 18, 2010 at 20:43, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> > Erik Johansson wrote on Tue, Dec 14, 2010 at 19:19:17 +0100:
> >> [[[
> >> r846201 added svn_repos_replay as a "replacement not so much for
> >> svn_repos_dir_delta, but for at least a whole class of functionality that
> >> svn_repos_dir_delta was never intended to handle."
> >>
> >> One place where svn_repos_replay replaced svn_repos_dir_delta was in
> >> combination with svn_repos_node_editor (only used in svnlook). This commit
> >> updates the documentation for svn_repos_node_editor and friends to reflect
> >> this.
> >>
> >> * subversion/include/svn_repos.h
> >>   (svn_repos_node_editor, svn_repos_node_from_baton): Doc update
> >> ]]]
> >
> > The patch basically does s/svn_repos_dir_delta2/svn_repos_replay2/g.
> > However, despite the log message, I still don't understand why that is
> > a correct change.
> 
> According to the log message for r846201 it seems like
> svn_repos_replay is faster than svn_repos_dir_delta for some
> operations. I can only assume that this is one such case.
> 

IOW, the patch recommends svn_repos_replay2() because that is newer API.

> > Why are we mentioning a specific driver at all?  Can svn_repos_node_editor()
> > be driven only by svn_repos_replay2()?
> >
> > (I don't see why that would be the case.)  If not, then its doc string
> > should avoid mention that the returned editor can be driven only by %s,
> > when in fact it can be driven by anything.
> 
> I don't know the anything about the other drivers, but my assumption
> is that any driver works, but svn_repos_replay is the prefered one in
> this case.
> 

*nod*

> > We have three mechanisms: svn_repos_dir_delta2(), svn_repos_begin_report2(),
> > and svn_repos_replay2().  I think it would be useful to just mention all of
> > them --- after all, none of them is deprecated and all of them can represent
> > a tree delta using an editor.
> 
> Mention all on equal footing or say that svn_repos_replay2 is the preferred one?
> 
> I can fix the docs, but I don't know enough about svn api to actually
> determine the correct fix. E.g. do all the drivers call methods in the
> editor in the same order?
> 

In theory, yes.  That's the core of the editor API: the
driver only knows the svn_delta_editor_t vtable, and the drivee also
knows only the vtable --- they need not (in general) know each other.

For example, 'svnrdump dump' drives a commit editor; but nothing in the
commit editor knows what dumpfiles are.

> // Erik
> 
> -- 
> Erik Johansson
> Home Page: http://ejohansson.se/
> PGP Key: http://ejohansson.se/erik.asc

Re: [PATCH] Correct documentation for svn_repos_node_editor and friends

[Erik Johansson <er...@ejohansson.se>]

On Sat, Dec 18, 2010 at 20:43, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> Erik Johansson wrote on Tue, Dec 14, 2010 at 19:19:17 +0100:
>> [[[
>> r846201 added svn_repos_replay as a "replacement not so much for
>> svn_repos_dir_delta, but for at least a whole class of functionality that
>> svn_repos_dir_delta was never intended to handle."
>>
>> One place where svn_repos_replay replaced svn_repos_dir_delta was in
>> combination with svn_repos_node_editor (only used in svnlook). This commit
>> updates the documentation for svn_repos_node_editor and friends to reflect
>> this.
>>
>> * subversion/include/svn_repos.h
>>   (svn_repos_node_editor, svn_repos_node_from_baton): Doc update
>> ]]]
>
> The patch basically does s/svn_repos_dir_delta2/svn_repos_replay2/g.
> However, despite the log message, I still don't understand why that is
> a correct change.

According to the log message for r846201 it seems like
svn_repos_replay is faster than svn_repos_dir_delta for some
operations. I can only assume that this is one such case.

> Why are we mentioning a specific driver at all?  Can svn_repos_node_editor()
> be driven only by svn_repos_replay2()?
>
> (I don't see why that would be the case.)  If not, then its doc string
> should avoid mention that the returned editor can be driven only by %s,
> when in fact it can be driven by anything.

I don't know the anything about the other drivers, but my assumption
is that any driver works, but svn_repos_replay is the prefered one in
this case.

> We have three mechanisms: svn_repos_dir_delta2(), svn_repos_begin_report2(),
> and svn_repos_replay2().  I think it would be useful to just mention all of
> them --- after all, none of them is deprecated and all of them can represent
> a tree delta using an editor.

Mention all on equal footing or say that svn_repos_replay2 is the preferred one?

I can fix the docs, but I don't know enough about svn api to actually
determine the correct fix. E.g. do all the drivers call methods in the
editor in the same order?

// Erik

-- 
Erik Johansson
Home Page: http://ejohansson.se/
PGP Key: http://ejohansson.se/erik.asc