Hi Erik. Just to let you know, I tried your current patch on a trunk build, against a repository revision in which files were added, and it crashed:
$ svnlook changed ~/vcs/green -r 16 A [...]/AddrChange/ A [...]/AddrChange/AddrLetters1-6.pdf $ svnlook changed ~/vcs/green -v -r 16 subversion/svnlook/main.c:1947: (apr_err=160013) subversion/svnlook/main.c:1511: (apr_err=160013) subversion/svnlook/main.c:458: (apr_err=160013) subversion/libsvn_delta/path_driver.c:256: (apr_err=160013) subversion/libsvn_repos/replay.c:613: (apr_err=160013) subversion/libsvn_repos/node_tree.c:411: (apr_err=160013) subversion/libsvn_fs_fs/tree.c:986: (apr_err=160013) subversion/libsvn_fs_fs/tree.c:986: (apr_err=160013) subversion/libsvn_fs_fs/tree.c:824: (apr_err=160013) subversion/libsvn_fs_fs/tree.c:666: (apr_err=160013) svnlook: File not found: revision 15, path '/[...]/AddrChange/AddrLetters1-6.pdf' I haven't tried to debug it. My repository format is 5, FSFS format 3. (It's an old one, not one that I created with this version of Subversion.) - Julian On Thu, 2010-12-09, Erik Johansson wrote: > 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? > > >> + /** 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. > > >> + /** 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. > > >> + /** 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. > > > > >> +} 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. > > // Erik >
