[ a few observations, but I haven't reviewed all of this yet ]
Erik Johansson wrote on Wed, Dec 08, 2010 at 17:17:47 +0100:
> Hi,
>
> Please see attached patch. Comments most welcome, specially if there
> is a better/less intrusive way of implementing <subject>.
>
> Please CC me as I'm not subscribed to the list.
>
> // Erik
>
> [[[
> Support printing changed properties in svnlook by passing the "-v" parameter
> to
> the "svnlook changed" command. The changed properties are printed one property
> per line with the format: <status> <path>;<prop name>
>
> 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()?
(Usually 'replay' refers to svn_repos_replay(), the API behind svnsync;
an editor is driven, not replayed.)
> * subversion/include/svn_repos.h
> (svn_repos_node_prop_t): New struct to represent a change to a property.
> (svn_repos_node_t): Add pointer to list of changed properties.
>
> * subversion/libsvn_repos/node_tree.c
> (change_node_prop): Add changed properties to the node structure.
>
> * subversion/svnlook/main.c
> (cmd_table subcommand_changed do_changed): New "verbose" option to indicate
> if changed properties should be printed or not.
> (generate_delta_tree): New parameter to make the replay with or without
> deltas.
> (do_dirs_changed do_diff): Add new parameter to generate_delta_tree call.
> (print_changed_tree): Support printing changed properties.
> ]]]
>
Log message looks good.
> Index: subversion/include/svn_repos.h
> ===================================================================
> --- subversion/include/svn_repos.h (revision 1043363)
> +++ subversion/include/svn_repos.h (working copy)
> @@ -2239,6 +2239,20 @@
> * result of having svn_repos_dir_delta2() drive that editor.
> */
>
> +/** A property on a node */
> +typedef struct svn_repos_node_prop_t
> +{
> + /** 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?
> + /** The name of the property */
> + const char *name;
> +
Where is the value of the property? How to get it?
> + /** 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)?
> +} 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).
Usually we provide constructor (and duplicator) API's for structs
defined in the headers... but we haven't done so in this case.
> Index: subversion/svnlook/main.c
> Index: subversion/libsvn_repos/node_tree.c
I haven't read these parts yet.
