Re: [PATCH] support printing changed properties in svnlook

On Fri, Dec 10, 2010 at 14:29, Daniel Shahaf <d.s_at_daniel.shahaf.name> wrote:
>> >> +  /** 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.
>>
>
> (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?

I agree that it is simple to add the value to the struct, but in
svnlook (which is the only current in-tree user of this api) I can't
see any need to get the value.

How a caller would get the property value if they indeed cared about
it is outside my knowledge of the api. Perhaps svn_repos_node_editor()
is not the editor to use if you want the property value?

>> >> +  /** 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?

A hash seemed like overkill, but if you think it is better I have no
problem using that.

// Erik

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