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.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?
>
> >> + /** 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.
>
> >> + /** 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
>
Received on 2010-12-10 12:56:12 CET