Re: [PATCH] support printing changed properties in svnlook

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

-- 
Erik Johansson
Home Page: http://ejohansson.se/
PGP Key: http://ejohansson.se/erik.asc