Erik Johansson wrote on Thu, Dec 09, 2010 at 21:41:38 +0100:
> On Wed, Dec 8, 2010 at 23:22, Daniel Shahaf <d.s_at_daniel.shahaf.name> wrote:
> > Erik Johansson wrote on Wed, Dec 08, 2010 at 17:17:47 +0100:
> >> To support this, the editor created by svn_repos_node_editor has been modified
> >> to record changes to properties (requires the replay to be done with deltas).
> >
> > Do you mean: text_deltas=FALSE should be passed to svn_repos_dir_delta2()?
>
> I mean that send_deltas=TRUE should be passed to svn_repos_replay2().
>
> > (Usually 'replay' refers to svn_repos_replay(), the API behind svnsync;
> > an editor is driven, not replayed.)
>
> I was referring to svn_repos_replay2() so that is were I got replay
> from. Is this incorrect?
>
I think we have a problem:
* svn_repos_node_editor()'s doc string describes what happens when the
editor returned by that function is driven *by svn_repos_dir_delta2()*.
* svnlook drives svn_repos_node_editor()'s editor by calling svn_repos_replay2().
(I didn't know svnlook uses that API too.)
In other words, svnlook uses svn_repos_node_editor() in a manner not
blessed by that API's docs. We should either fix the usage in svnlook
or clarify the API docs to better say what is/isn't allowed.
Could you untangle this mess around driving the editor? (I might be
able to look into this later, but not right now)
> >> + /** How this property entered the node tree: 'A'dd, 'D'elete, 'R'eplace */
> >> + char action;
> >
> > This is copied from svn_repos_node_t->action. There was recently
> > a question about that field:
> > http://mid.gmane.org/3ABD28AA-A2FC-4D7D-A502-479D37995DB9@orcaware.com
> >
> > So, that asks whether 'C'hanged is a valid answer to the question that
> > ->action is meant to answer. I'll also ask how this interacts with node
> > changes: for example; if r5 replaces-with-history a node that has
> > 'fooprop' set with another node that also has 'fooprop' set, what would
> > the value of 'action' be?
>
> What about this:
> When a node is deleted all the properties it had are listed in
> mod_prop with action D.
>
> When a node is added-with-history all the properties the source had
> are listed in mod_prop with action A and a new flag copyfrom = TRUE.
>
> A replace-with-history will result in two repos_nodes, each having a
> mod_prop list. If the same property exists in both it means it has
> been replaced.
>
(will reply later)
> >> + /** The name of the property */
> >> + const char *name;
> >
> > Where is the value of the property? How to get it?
>
> The idea was that the struct should indicate changes to properties,
> not their values. In the same way that svn_repos_node_t shows changes,
> not node content.
>
Flawed analogy: we never store node content in memory, but we do have
all property values in memory, so the cost is just to add an
svn_string_t * member to the struct.
My question was how would callers get the value if they cared about it.
i.e., I assume that whoever calls this function already has a property
hash (or, at least, an fs object) available that they can get the
property values from?
> >> + /** Pointer to the next sibling property */
> >> + struct svn_repos_node_prop_t *sibling;
> >> +
> >
> > You use a linked list. How about using apr_array_header_t *? Or a hash
> > of (prop_name -> struct)?
>
> I guess anyone of those would work, but the reason I went for a linked
> list was that svn_repos_node_t did that and I wanted them to be
> similar.
>
Hmm. I haven't seen that struct before: svn_repos_node_t indeed uses an
explicit tree structure.
What do you think will be more useful to consumers of the API? A hash
allows both random access and iteration --- do APR arrays or linked
lists have advantages that outweigh that?
(honest, not rhetorical, question)
> >> +} svn_repos_node_prop_t;
> >> +
> >> /** A node in the repository. */
> >> typedef struct svn_repos_node_t
> >> {
> >> @@ -2272,6 +2286,9 @@
> >> /** Pointer to the parent of this node */
> >> struct svn_repos_node_t *parent;
> >>
> >> + /** Pointer to the first modified property */
> >> + svn_repos_node_prop_t *mod_prop;
> >> +
> >> } svn_repos_node_t;
> >>
> >
> > I'm afraid you can't extend this struct due to binary compatibility
> > considerations (an application built against 1.6 but running against 1.7
> > will create too short a struct).
>
> This was actually one of my concerns as well. I will try to come up
> with another way of doing it.
>
OK. Feel free to discuss ideas on the list if you need feedback.
The always-available method is to revv the struct (and any needed API's
in whose signature it appears) --- i.e., to introduce svn_repos_node2_t
--- but I haven't considered this case to try and find alternative
solutions specific to it.
> // Erik
>
> --
> Erik Johansson
> Home Page: http://ejohansson.se/
> PGP Key: http://ejohansson.se/erik.asc
Received on 2010-12-10 14:32:05 CET