Erik Johansson wrote on Wed, Dec 22, 2010 at 23:00:43 +0100:
> 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.
>
Yep; but we're designing an API, so I was trying to get it right (ie:
general) the first time.
IMO, if we're tweaking the API and we can have it put the value there
without too much effort, we should do that. (But since svnlook doesn't
need that, I'm okay with punting on providing the value in the API if
it's more than some threshold effort.)
> 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?
>
To be honest I don't recall the context exactly, but I imagine that
whoever called the API knows whose properties list it was just given, so
it could just use another API to get the values if needed.
Over RA there are sometimes calling order restrictions, but I don't
recall that we have such issues at the repos layer (which is lower).
> >> >> + /** 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.
>
I'm used to seeing 'apr_hash_t *props'.
I think an APR array (apr_array_header_t) would be better than an
explicit linked list, because we can use library functions rather than
rewrite them. Would an apr_hash_t be better still? Possibly, but I'm
not feeling strongly on that. (It's not needed for svnlook's use case,
and it might be a bit costlier.)
> // Erik
>
> --
> Erik Johansson
> Home Page: http://ejohansson.se/
> PGP Key: http://ejohansson.se/erik.asc
Received on 2010-12-29 04:03:00 CET