Erik Johansson wrote on Thu, Dec 09, 2010 at 21:41:38 +0100: > On Wed, Dec 8, 2010 at 23:22, Daniel Shahaf <d...@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-479d37995...@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
