You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Erik Johansson <er...@ejohansson.se> on 2010/12/08 17:17:47 UTC
[PATCH] support printing changed properties in svnlook
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).
* 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.
]]]
--
Erik Johansson
Home Page: http://ejohansson.se/
PGP Key: http://ejohansson.se/erik.asc
Re: [PATCH] support printing changed properties in svnlook
Posted by Gavin Beau Baumanis <ga...@thespidernet.com>.
Ping. This patch has received no new comments.
On 08/01/2011, at 5:08 AM, Daniel Shahaf wrote:
> I'll add it to my list, but as I'll have to read the whole thing before
> answering it might take a while until I'll answer.
>
> If anyone else wants to beat me to it...
>
>
> Erik Johansson wrote on Thu, Jan 06, 2011 at 22:43:43 +0100:
>> On Thu, Jan 6, 2011 at 19:15, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
>>> I can't find a summary anywhere in your mail. Is the summary "Here is
>>> a work-in-progress patch that revvs the struct and adds a mod_props
>>> member, and below are the specific semantics I'm thinking of for that
>>> member; please review those semantics"?
>>
>> Yes it is, sorry for leaving that out. I was trying to get all the
>> cases right that I totally forgot to add a summary. I'm interested in
>> getting your input on the implementation and what you think about the
>> proposed semantics.
>>
>> Thanks,
>> // Erik
>>
>> --
>> Erik Johansson
>> Home Page: http://ejohansson.se/
>> PGP Key: http://ejohansson.se/erik.asc
Re: [PATCH] support printing changed properties in svnlook
Posted by Gavin Beau Baumanis <ga...@thespidernet.com>.
Hi Daniel / Erik,
I have created a new issue : 3821.
You can find it here;
http://subversion.tigris.org/issues/show_bug.cgi?id=3821
I have attached the latest submission to this issue.
On 23/02/2011, at 12:44 AM, Daniel Shahaf wrote:
> Yes, please file an issue, and in the meantime I'll re-add it to my
> list. (There are a few things ahead of it...)
>
> Thanks!
>
> Daniel
>
> Gavin Beau Baumanis wrote on Tue, Feb 22, 2011 at 21:29:27 +1100:
>> Hi Daniel,
>>
>> Just checking as to whether or not you think you'll be able to get to this submission or not, please?
>> I'm more than happy to log it into the Issue Tracker, if you feel that time is getting the better of you.
>>
>>
>>
>> On 08/01/2011, at 5:08 AM, Daniel Shahaf wrote:
>>
>>> I'll add it to my list, but as I'll have to read the whole thing before
>>> answering it might take a while until I'll answer.
>>>
>>> If anyone else wants to beat me to it...
>>>
>>>
>>> Erik Johansson wrote on Thu, Jan 06, 2011 at 22:43:43 +0100:
>>>> On Thu, Jan 6, 2011 at 19:15, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
>>>>> I can't find a summary anywhere in your mail. Is the summary "Here is
>>>>> a work-in-progress patch that revvs the struct and adds a mod_props
>>>>> member, and below are the specific semantics I'm thinking of for that
>>>>> member; please review those semantics"?
>>>>
>>>> Yes it is, sorry for leaving that out. I was trying to get all the
>>>> cases right that I totally forgot to add a summary. I'm interested in
>>>> getting your input on the implementation and what you think about the
>>>> proposed semantics.
>>>>
>>>> Thanks,
>>>> // Erik
>>>>
>>>> --
>>>> Erik Johansson
>>>> Home Page: http://ejohansson.se/
>>>> PGP Key: http://ejohansson.se/erik.asc
>>
Re: [PATCH] support printing changed properties in svnlook
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Yes, please file an issue, and in the meantime I'll re-add it to my
list. (There are a few things ahead of it...)
Thanks!
Daniel
Gavin Beau Baumanis wrote on Tue, Feb 22, 2011 at 21:29:27 +1100:
> Hi Daniel,
>
> Just checking as to whether or not you think you'll be able to get to this submission or not, please?
> I'm more than happy to log it into the Issue Tracker, if you feel that time is getting the better of you.
>
>
>
> On 08/01/2011, at 5:08 AM, Daniel Shahaf wrote:
>
> > I'll add it to my list, but as I'll have to read the whole thing before
> > answering it might take a while until I'll answer.
> >
> > If anyone else wants to beat me to it...
> >
> >
> > Erik Johansson wrote on Thu, Jan 06, 2011 at 22:43:43 +0100:
> >> On Thu, Jan 6, 2011 at 19:15, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> >>> I can't find a summary anywhere in your mail. Is the summary "Here is
> >>> a work-in-progress patch that revvs the struct and adds a mod_props
> >>> member, and below are the specific semantics I'm thinking of for that
> >>> member; please review those semantics"?
> >>
> >> Yes it is, sorry for leaving that out. I was trying to get all the
> >> cases right that I totally forgot to add a summary. I'm interested in
> >> getting your input on the implementation and what you think about the
> >> proposed semantics.
> >>
> >> Thanks,
> >> // Erik
> >>
> >> --
> >> Erik Johansson
> >> Home Page: http://ejohansson.se/
> >> PGP Key: http://ejohansson.se/erik.asc
>
Re: [PATCH] support printing changed properties in svnlook
Posted by Gavin Beau Baumanis <ga...@thespidernet.com>.
Hi Daniel,
Just checking as to whether or not you think you'll be able to get to this submission or not, please?
I'm more than happy to log it into the Issue Tracker, if you feel that time is getting the better of you.
On 08/01/2011, at 5:08 AM, Daniel Shahaf wrote:
> I'll add it to my list, but as I'll have to read the whole thing before
> answering it might take a while until I'll answer.
>
> If anyone else wants to beat me to it...
>
>
> Erik Johansson wrote on Thu, Jan 06, 2011 at 22:43:43 +0100:
>> On Thu, Jan 6, 2011 at 19:15, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
>>> I can't find a summary anywhere in your mail. Is the summary "Here is
>>> a work-in-progress patch that revvs the struct and adds a mod_props
>>> member, and below are the specific semantics I'm thinking of for that
>>> member; please review those semantics"?
>>
>> Yes it is, sorry for leaving that out. I was trying to get all the
>> cases right that I totally forgot to add a summary. I'm interested in
>> getting your input on the implementation and what you think about the
>> proposed semantics.
>>
>> Thanks,
>> // Erik
>>
>> --
>> Erik Johansson
>> Home Page: http://ejohansson.se/
>> PGP Key: http://ejohansson.se/erik.asc
Re: [PATCH] support printing changed properties in svnlook
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
I'll add it to my list, but as I'll have to read the whole thing before
answering it might take a while until I'll answer.
If anyone else wants to beat me to it...
Erik Johansson wrote on Thu, Jan 06, 2011 at 22:43:43 +0100:
> On Thu, Jan 6, 2011 at 19:15, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> > I can't find a summary anywhere in your mail. Is the summary "Here is
> > a work-in-progress patch that revvs the struct and adds a mod_props
> > member, and below are the specific semantics I'm thinking of for that
> > member; please review those semantics"?
>
> Yes it is, sorry for leaving that out. I was trying to get all the
> cases right that I totally forgot to add a summary. I'm interested in
> getting your input on the implementation and what you think about the
> proposed semantics.
>
> Thanks,
> // Erik
>
> --
> Erik Johansson
> Home Page: http://ejohansson.se/
> PGP Key: http://ejohansson.se/erik.asc
Re: [PATCH] support printing changed properties in svnlook
Posted by Erik Johansson <er...@ejohansson.se>.
On Thu, Jan 6, 2011 at 19:15, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> I can't find a summary anywhere in your mail. Is the summary "Here is
> a work-in-progress patch that revvs the struct and adds a mod_props
> member, and below are the specific semantics I'm thinking of for that
> member; please review those semantics"?
Yes it is, sorry for leaving that out. I was trying to get all the
cases right that I totally forgot to add a summary. I'm interested in
getting your input on the implementation and what you think about the
proposed semantics.
Thanks,
// Erik
--
Erik Johansson
Home Page: http://ejohansson.se/
PGP Key: http://ejohansson.se/erik.asc
Re: [PATCH] support printing changed properties in svnlook
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Erik Johansson wrote on Wed, Jan 05, 2011 at 22:43:51 +0100:
> Hi,
>
> I have now redone the patch to fix some of the comments given in this
> thread and I'm attaching the current working state of it so that we
> can have something real to discuss around. This does not include any
> changes to svnlook but have a test case to verify that it works as it
> should (which it at the moment doesn't, see 10 below).
>
Nice work.
I can't find a summary anywhere in your mail. Is the summary "Here is
a work-in-progress patch that revvs the struct and adds a mod_props
member, and below are the specific semantics I'm thinking of for that
member; please review those semantics"?
Thanks,
Daniel
> Further more, I've tried to enumerate all cases that I could think of
> were properties are changed and how they should be handled. For some
> of the cases I've changed my mind compared to what I've said in the
> rest of the thread. Hopefully this is a complete list.
>
> mod_props is the per node hash with properties (see svn_repos_node2_t).
>
... snip 12 enumerated items ...
>
> // Erik
>
> --
> Erik Johansson
> Home Page: http://ejohansson.se/
> PGP Key: http://ejohansson.se/erik.asc
Re: [PATCH] support printing changed properties in svnlook
Posted by Erik Johansson <er...@ejohansson.se>.
Hi,
I have now redone the patch to fix some of the comments given in this
thread and I'm attaching the current working state of it so that we
can have something real to discuss around. This does not include any
changes to svnlook but have a test case to verify that it works as it
should (which it at the moment doesn't, see 10 below).
Further more, I've tried to enumerate all cases that I could think of
were properties are changed and how they should be handled. For some
of the cases I've changed my mind compared to what I've said in the
rest of the thread. Hopefully this is a complete list.
mod_props is the per node hash with properties (see svn_repos_node2_t).
1. Add node w/o props => mod_props is NULL
2. Add node w/ props => mod_props contains all props
Pretty straight-forward; mod_props contains what can be expected and
action is A for all props.
3. Delete node w/o props => mod_props is NULL
4. Delete node w/ props => mod_props is NULL
For case 4 it could be argued that mod_props should contain all props
that the node had prior to being deleted. First I thought it should,
but after thinking some more I now have the opinion that it isn't of
any real use. The node is gone after all, and it doesn't matter if the
node had e.g. svn:eol-style set or not.
5. Change (add, delete or modify) node props => mod_props contains all
changed props
Also straight-forward. Action is A, D or M depending on type of
change. Properties that haven't changed are not included in mod_props.
6. Copy (i.e. add-with-history) node w/o props => mod_props is NULL
7. Copy node w/ props => mod_props is NULL
Case 7 is somewhat similar to case 4 in that one could argue that
mod_props should contain all props that were copied with the node. But
as nothing has been changed and text_mod and prop_mod are both FALSE
in this case I think that mod_props should be NULL.
8. Copy node w/ changed props [1] => mod_props contains all changed props
In this case one or several properties are added, deleted or modified
between coping the node and committing. Unmodified props are not
included in mod_props (case 7).
9. Replace (without history) node (w/ or w/o props) with node w/o
props => mod_props is NULL
We don't include any props from the deleted node (case 3 and 4) and no
props are added for the new node (case 1).
10. Replace node (w/ or w/o props) with node w/ props => mod_props
contains all props
We don't include any props from the deleted node (case 3 and 4) but
new props are added with the new node (case 2). The test for this
fails as new props are marked as M if they existed in the deleted
node. Don't know a good solution for this yet. Suggestions welcome.
11. Replace (with history) node (w/ or w/o props) with copied node w/
or w/o props => mod_props is NULL
We don't include any props from the deleted node (case 3 and 4) nor
any props from the unmodified copy (case 6 and 7).
12. Replace node (w/ or w/o props) with copied node w/ modified props
=> mod_props contains all modified props
We don't include any props from the deleted node (case 3 and 4) but
the modified props from the copied node (case 8).
// Erik
--
Erik Johansson
Home Page: http://ejohansson.se/
PGP Key: http://ejohansson.se/erik.asc
Re: [PATCH] support printing changed properties in svnlook
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Daniel Shahaf wrote on Tue, Dec 14, 2010 at 10:05:35 +0200:
> Erik Johansson wrote on Mon, Dec 13, 2010 at 22:27:04 +0100:
> > On Fri, Dec 10, 2010 at 14:29, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> > > Could you untangle this mess around driving the editor? (I might be
> > > able to look into this later, but not right now)
> >
> > See attempt in other thread.
> >
>
> Saw the thread, marked it for attention, will get to it hopefully soon.
Sorry, but I have to take my words back; I won't be able to get to that
patch as soon as I'd hoped to.
Re: [PATCH] support printing changed properties in svnlook
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Daniel Shahaf wrote on Tue, Dec 14, 2010 at 10:05:35 +0200:
> Erik Johansson wrote on Mon, Dec 13, 2010 at 22:27:04 +0100:
> > On Fri, Dec 10, 2010 at 14:29, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> > > Could you untangle this mess around driving the editor? (I might be
> > > able to look into this later, but not right now)
> >
> > See attempt in other thread.
> >
>
> Saw the thread, marked it for attention, will get to it hopefully soon.
Sorry, but I have to take my words back; I won't be able to get to that
patch as soon as I'd hoped to.
Re: [PATCH] support printing changed properties in svnlook
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Erik Johansson wrote on Mon, Dec 13, 2010 at 22:27:04 +0100:
> On Fri, Dec 10, 2010 at 14:29, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> > Could you untangle this mess around driving the editor? (I might be
> > able to look into this later, but not right now)
>
> See attempt in other thread.
>
Saw the thread, marked it for attention, will get to it hopefully soon.
> >> >> +} svn_repos_node_prop_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.
>
> I've been thinking some more about this:
> - svn_repos_node_t is never used as an in parameter, so there
> shouldn't be any need for applications to create one.
> - If an application creates a svn_repos_node_t, the only use for it
> would be internal to the application in which case it will use its
> version of the struct even if we change it.
> - If we add the new field to the end of the struct there should be no
> problems for applications built with an old version of the struct;
> they will simply not be able to access the new field.
>
So, there would be TWO svn_repos_node_t structs in an
appliaction:
* Those provided by the callbacks
(1.7 style, but application doesn't read the new fields, and we never
read them after the callback had had an opportunity to touch them)
* Those constructed by the application
(svn libraries should never see them so we Don't Care)
> What do you think?
>
Technically, it's probably going to work. (at least as long as we don't
add a function that takes an svn_repos_node_t parameter, etc)
But it's a grey area, and having special cases both complicates
everyone's lives and weakens our API compatibility guarantees.
Call me conservative, but I don't like this option; I would rather not
get into this corner when we could just revv the struct, add
a constructor and duplicator for the new struct, and solve the problem
once and for all. (We have precedent for struct types being defined ---
not just declared --- in the public headers, along with a comment "Don't
allocate it by yourself; use foo_create() to create it.")
> // Erik
>
Re: [PATCH] support printing changed properties in svnlook
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Erik Johansson wrote on Mon, Dec 13, 2010 at 22:27:04 +0100:
> On Fri, Dec 10, 2010 at 14:29, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> > Could you untangle this mess around driving the editor? (I might be
> > able to look into this later, but not right now)
>
> See attempt in other thread.
>
Saw the thread, marked it for attention, will get to it hopefully soon.
> >> >> +} svn_repos_node_prop_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.
>
> I've been thinking some more about this:
> - svn_repos_node_t is never used as an in parameter, so there
> shouldn't be any need for applications to create one.
> - If an application creates a svn_repos_node_t, the only use for it
> would be internal to the application in which case it will use its
> version of the struct even if we change it.
> - If we add the new field to the end of the struct there should be no
> problems for applications built with an old version of the struct;
> they will simply not be able to access the new field.
>
So, there would be TWO svn_repos_node_t structs in an
appliaction:
* Those provided by the callbacks
(1.7 style, but application doesn't read the new fields, and we never
read them after the callback had had an opportunity to touch them)
* Those constructed by the application
(svn libraries should never see them so we Don't Care)
> What do you think?
>
Technically, it's probably going to work. (at least as long as we don't
add a function that takes an svn_repos_node_t parameter, etc)
But it's a grey area, and having special cases both complicates
everyone's lives and weakens our API compatibility guarantees.
Call me conservative, but I don't like this option; I would rather not
get into this corner when we could just revv the struct, add
a constructor and duplicator for the new struct, and solve the problem
once and for all. (We have precedent for struct types being defined ---
not just declared --- in the public headers, along with a comment "Don't
allocate it by yourself; use foo_create() to create it.")
> // Erik
>
Re: [PATCH] support printing changed properties in svnlook
Posted by Erik Johansson <er...@ejohansson.se>.
On Fri, Dec 10, 2010 at 14:29, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> Could you untangle this mess around driving the editor? (I might be
> able to look into this later, but not right now)
See attempt in other thread.
[snip a discussion I'll have to get back to later]
>> >> +} svn_repos_node_prop_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.
I've been thinking some more about this:
- svn_repos_node_t is never used as an in parameter, so there
shouldn't be any need for applications to create one.
- If an application creates a svn_repos_node_t, the only use for it
would be internal to the application in which case it will use its
version of the struct even if we change it.
- If we add the new field to the end of the struct there should be no
problems for applications built with an old version of the struct;
they will simply not be able to access the new field.
What do you think?
// Erik
--
Erik Johansson
Home Page: http://ejohansson.se/
PGP Key: http://ejohansson.se/erik.asc
Re: [PATCH] support printing changed properties in svnlook
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Erik Johansson wrote on Wed, Dec 22, 2010 at 23:00:43 +0100:
> On Fri, Dec 10, 2010 at 14:29, Daniel Shahaf <d....@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
Re: [PATCH] support printing changed properties in svnlook
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Erik Johansson wrote on Wed, Dec 22, 2010 at 23:00:43 +0100:
> On Fri, Dec 10, 2010 at 14:29, Daniel Shahaf <d....@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
Re: [PATCH] support printing changed properties in svnlook
Posted by Erik Johansson <er...@ejohansson.se>.
On Fri, Dec 10, 2010 at 14:29, Daniel Shahaf <d....@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
Re: [PATCH] support printing changed properties in svnlook
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Erik Johansson wrote on Thu, Dec 09, 2010 at 21:41:38 +0100:
> On Wed, Dec 8, 2010 at 23:22, Daniel Shahaf <d....@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?
>
I think we have a problem:
* svn_repos_node_editor()'s doc string describes what happens when the
editor returned by that function is driven *by svn_repos_dir_delta2()*.
* svnlook drives svn_repos_node_editor()'s editor by calling svn_repos_replay2().
(I didn't know svnlook uses that API too.)
In other words, svnlook uses svn_repos_node_editor() in a manner not
blessed by that API's docs. We should either fix the usage in svnlook
or clarify the API docs to better say what is/isn't allowed.
Could you untangle this mess around driving the editor? (I might be
able to look into this later, but not right now)
> >> + /** 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?
> >> + /** 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?
(honest, not rhetorical, question)
> >> +} 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.
>
OK. Feel free to discuss ideas on the list if you need feedback.
The always-available method is to revv the struct (and any needed API's
in whose signature it appears) --- i.e., to introduce svn_repos_node2_t
--- but I haven't considered this case to try and find alternative
solutions specific to it.
> // Erik
>
> --
> Erik Johansson
> Home Page: http://ejohansson.se/
> PGP Key: http://ejohansson.se/erik.asc
Re: [PATCH] support printing changed properties in svnlook
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Erik Johansson wrote on Thu, Dec 09, 2010 at 21:41:38 +0100:
> On Wed, Dec 8, 2010 at 23:22, Daniel Shahaf <d....@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?
>
I think we have a problem:
* svn_repos_node_editor()'s doc string describes what happens when the
editor returned by that function is driven *by svn_repos_dir_delta2()*.
* svnlook drives svn_repos_node_editor()'s editor by calling svn_repos_replay2().
(I didn't know svnlook uses that API too.)
In other words, svnlook uses svn_repos_node_editor() in a manner not
blessed by that API's docs. We should either fix the usage in svnlook
or clarify the API docs to better say what is/isn't allowed.
Could you untangle this mess around driving the editor? (I might be
able to look into this later, but not right now)
> >> + /** 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?
> >> + /** 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?
(honest, not rhetorical, question)
> >> +} 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.
>
OK. Feel free to discuss ideas on the list if you need feedback.
The always-available method is to revv the struct (and any needed API's
in whose signature it appears) --- i.e., to introduce svn_repos_node2_t
--- but I haven't considered this case to try and find alternative
solutions specific to it.
> // Erik
>
> --
> Erik Johansson
> Home Page: http://ejohansson.se/
> PGP Key: http://ejohansson.se/erik.asc
Re: [PATCH] support printing changed properties in svnlook
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
[ The second hunk below has a discussion regarding the representation
in the API of 'svn ps k v iota; svn ci iota; svn cp iota iota2;
svn ps k2 v2 iota2; svn ci iota2', which I'd be happy to have further
input on. ]
Erik Johansson wrote on Wed, Dec 22, 2010 at 23:08:30 +0100:
> On Fri, Dec 10, 2010 at 15:08, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> > [ I'm somewhat confused about this issue. ]
> >
> > Erik Johansson wrote on Thu, Dec 09, 2010 at 21:41:38 +0100:
> >> On Wed, Dec 8, 2010 at 23:22, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> >> > Erik Johansson wrote on Wed, Dec 08, 2010 at 17:17:47 +0100:
> >> >> + /** How this property entered the node tree: 'A'dd, 'D'elete, 'R'eplace */
> >> >> + char action;
> >
> > I should have caught this earlier, but "replaced" doesn't make sense
> > for properties. (A node can be replaced by another node at the same
> > path, but for a fixed node properties can be added/modified/removed but
> > not replaced.)
>
> I'll change it to 'A'dd, 'D'elete or 'U'pdate then?
>
'M'odify, not 'U'pdate, I think. At least that's what the UI level
does, but if there's precedent for 'U'pdated then follow it.
> >> >
> >> > 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.
> >>
> > There is already a copyfrom indication in svn_repos_node_t->copyfrom_rev,
> > I don't think we need another one.
>
> I was thinking that the copyfrom flag would be useful in the case
> where you copy a node with a propery and add another property before
> committing. Here you would have two property with action A and one
> would have copyfrom = TRUE.
>
In other words, you'd like to make a distinction between "This node was
added-with-history, and this property came along" and "<ditto>, but the
property is new on this copy target and didn't exist in the copy
source".
I see, and I agree it's a good distinction to make.
I'm not sure how/whether our other API's (the editor, possibly the
reporter, others?) represent this.
> >> 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.
> >>
> > Two nodes doesn't sound like a good idea; one node marked as 'replaced'
> > should suffice.
>
> In the current api a replaced node is represented by two nodes. Should
> we change this?
Doesn't that mean that API callers who want to distinguish a replace
from a remove or an add have to walk through all the child entries (for
a given dir) before they can process any adds or removals?
I'm thinking that one entry is better, because callers can split it if they
wish; that's easier than two entries and letting callers figure out for
themselves how to recombine them.
> Should svnlook stay back-wards compatible and print a
> replaced node twice, once as Deleted and once as Added?
>
I could argue both ways: it should be back-compatible with itself, but
it should also be compatible with the cmdline client where the latter
prints just one "R" notification.
> > In case of a replacement, do we want the API to provide the replaced
> > node's properties? (at least their names)
>
I suppose we can agree to answer this as "Yes", then? :)
> It would be pretty simple if there are two separate nodes for a
> replaced node. The deleted node will have all the properties it had
> marked as Deleted. The new node will have its properties marked as
> appropiate.
>
That's a straightforward representation, yes. I suppose we could find
a straightforward in the "single node" case, though --- eg by adding
fields only used in the case of 'R'eplacements.
> // Erik
>
> --
> Erik Johansson
> Home Page: http://ejohansson.se/
> PGP Key: http://ejohansson.se/erik.asc
Re: [PATCH] support printing changed properties in svnlook
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
[ The second hunk below has a discussion regarding the representation
in the API of 'svn ps k v iota; svn ci iota; svn cp iota iota2;
svn ps k2 v2 iota2; svn ci iota2', which I'd be happy to have further
input on. ]
Erik Johansson wrote on Wed, Dec 22, 2010 at 23:08:30 +0100:
> On Fri, Dec 10, 2010 at 15:08, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> > [ I'm somewhat confused about this issue. ]
> >
> > Erik Johansson wrote on Thu, Dec 09, 2010 at 21:41:38 +0100:
> >> On Wed, Dec 8, 2010 at 23:22, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> >> > Erik Johansson wrote on Wed, Dec 08, 2010 at 17:17:47 +0100:
> >> >> + /** How this property entered the node tree: 'A'dd, 'D'elete, 'R'eplace */
> >> >> + char action;
> >
> > I should have caught this earlier, but "replaced" doesn't make sense
> > for properties. (A node can be replaced by another node at the same
> > path, but for a fixed node properties can be added/modified/removed but
> > not replaced.)
>
> I'll change it to 'A'dd, 'D'elete or 'U'pdate then?
>
'M'odify, not 'U'pdate, I think. At least that's what the UI level
does, but if there's precedent for 'U'pdated then follow it.
> >> >
> >> > 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.
> >>
> > There is already a copyfrom indication in svn_repos_node_t->copyfrom_rev,
> > I don't think we need another one.
>
> I was thinking that the copyfrom flag would be useful in the case
> where you copy a node with a propery and add another property before
> committing. Here you would have two property with action A and one
> would have copyfrom = TRUE.
>
In other words, you'd like to make a distinction between "This node was
added-with-history, and this property came along" and "<ditto>, but the
property is new on this copy target and didn't exist in the copy
source".
I see, and I agree it's a good distinction to make.
I'm not sure how/whether our other API's (the editor, possibly the
reporter, others?) represent this.
> >> 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.
> >>
> > Two nodes doesn't sound like a good idea; one node marked as 'replaced'
> > should suffice.
>
> In the current api a replaced node is represented by two nodes. Should
> we change this?
Doesn't that mean that API callers who want to distinguish a replace
from a remove or an add have to walk through all the child entries (for
a given dir) before they can process any adds or removals?
I'm thinking that one entry is better, because callers can split it if they
wish; that's easier than two entries and letting callers figure out for
themselves how to recombine them.
> Should svnlook stay back-wards compatible and print a
> replaced node twice, once as Deleted and once as Added?
>
I could argue both ways: it should be back-compatible with itself, but
it should also be compatible with the cmdline client where the latter
prints just one "R" notification.
> > In case of a replacement, do we want the API to provide the replaced
> > node's properties? (at least their names)
>
I suppose we can agree to answer this as "Yes", then? :)
> It would be pretty simple if there are two separate nodes for a
> replaced node. The deleted node will have all the properties it had
> marked as Deleted. The new node will have its properties marked as
> appropiate.
>
That's a straightforward representation, yes. I suppose we could find
a straightforward in the "single node" case, though --- eg by adding
fields only used in the case of 'R'eplacements.
> // Erik
>
> --
> Erik Johansson
> Home Page: http://ejohansson.se/
> PGP Key: http://ejohansson.se/erik.asc
Re: [PATCH] support printing changed properties in svnlook
Posted by Erik Johansson <er...@ejohansson.se>.
On Fri, Dec 10, 2010 at 15:08, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> [ I'm somewhat confused about this issue. ]
>
> Erik Johansson wrote on Thu, Dec 09, 2010 at 21:41:38 +0100:
>> On Wed, Dec 8, 2010 at 23:22, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
>> > Erik Johansson wrote on Wed, Dec 08, 2010 at 17:17:47 +0100:
>> >> + /** How this property entered the node tree: 'A'dd, 'D'elete, 'R'eplace */
>> >> + char action;
>
> I should have caught this earlier, but "replaced" doesn't make sense
> for properties. (A node can be replaced by another node at the same
> path, but for a fixed node properties can be added/modified/removed but
> not replaced.)
I'll change it to 'A'dd, 'D'elete or 'U'pdate then?
>> >
>> > 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.
>>
> There is already a copyfrom indication in svn_repos_node_t->copyfrom_rev,
> I don't think we need another one.
I was thinking that the copyfrom flag would be useful in the case
where you copy a node with a propery and add another property before
committing. Here you would have two property with action A and one
would have 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.
>>
> Two nodes doesn't sound like a good idea; one node marked as 'replaced'
> should suffice.
In the current api a replaced node is represented by two nodes. Should
we change this? Should svnlook stay back-wards compatible and print a
replaced node twice, once as Deleted and once as Added?
> In case of a replacement, do we want the API to provide the replaced
> node's properties? (at least their names)
It would be pretty simple if there are two separate nodes for a
replaced node. The deleted node will have all the properties it had
marked as Deleted. The new node will have its properties marked as
appropiate.
// Erik
--
Erik Johansson
Home Page: http://ejohansson.se/
PGP Key: http://ejohansson.se/erik.asc
Re: [PATCH] support printing changed properties in svnlook
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
[ I'm somewhat confused about this issue. ]
Erik Johansson wrote on Thu, Dec 09, 2010 at 21:41:38 +0100:
> On Wed, Dec 8, 2010 at 23:22, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> > Erik Johansson wrote on Wed, Dec 08, 2010 at 17:17:47 +0100:
> >> + /** How this property entered the node tree: 'A'dd, 'D'elete, 'R'eplace */
> >> + char action;
I should have caught this earlier, but "replaced" doesn't make sense
for properties. (A node can be replaced by another node at the same
path, but for a fixed node properties can be added/modified/removed but
not replaced.)
> >
> > 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.
>
There is already a copyfrom indication in svn_repos_node_t->copyfrom_rev,
I don't think we need another one.
> 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.
>
Two nodes doesn't sound like a good idea; one node marked as 'replaced'
should suffice.
In case of a replacement, do we want the API to provide the replaced
node's properties? (at least their names)
> >> + /** The name of the property */
> >> + const char *name;
> >> +
> >> + /** Pointer to the next sibling property */
> >> + struct svn_repos_node_prop_t *sibling;
> >> +
> >> +} svn_repos_node_prop_t;
> >> +
Re: [PATCH] support printing changed properties in svnlook
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
[ I'm somewhat confused about this issue. ]
Erik Johansson wrote on Thu, Dec 09, 2010 at 21:41:38 +0100:
> On Wed, Dec 8, 2010 at 23:22, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> > Erik Johansson wrote on Wed, Dec 08, 2010 at 17:17:47 +0100:
> >> + /** How this property entered the node tree: 'A'dd, 'D'elete, 'R'eplace */
> >> + char action;
I should have caught this earlier, but "replaced" doesn't make sense
for properties. (A node can be replaced by another node at the same
path, but for a fixed node properties can be added/modified/removed but
not replaced.)
> >
> > 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.
>
There is already a copyfrom indication in svn_repos_node_t->copyfrom_rev,
I don't think we need another one.
> 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.
>
Two nodes doesn't sound like a good idea; one node marked as 'replaced'
should suffice.
In case of a replacement, do we want the API to provide the replaced
node's properties? (at least their names)
> >> + /** The name of the property */
> >> + const char *name;
> >> +
> >> + /** Pointer to the next sibling property */
> >> + struct svn_repos_node_prop_t *sibling;
> >> +
> >> +} svn_repos_node_prop_t;
> >> +
Re: [PATCH] support printing changed properties in svnlook
Posted by Erik Johansson <er...@ejohansson.se>.
On Fri, Dec 10, 2010 at 13:53, Julian Foad <ju...@wandisco.com> wrote:
> Great. Do you feel up to writing some Python tests for this to add to
> the regression test suite? That would be very useful.
Sure, will do.
// Erik
--
Erik Johansson
Home Page: http://ejohansson.se/
PGP Key: http://ejohansson.se/erik.asc
Re: [PATCH] support printing changed properties in svnlook
Posted by Julian Foad <ju...@wandisco.com>.
On Fri, 2010-12-10 at 13:48 +0100, Erik Johansson wrote:
> On Fri, Dec 10, 2010 at 12:55, Julian Foad <ju...@wandisco.com> wrote:
> > 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:
>
> Yes, I've also found this when doing some more testing after Daniel's
> mail. I fixed it by calling svn_fs_check_path() and checking that node
> kind != svn_node_none before calling svn_fs_node_prop(). It will be in
> the next patch version.
Great. Do you feel up to writing some Python tests for this to add to
the regression test suite? That would be very useful.
- Julian
> Thanks for testing.
>
> // Erik
>
Re: [PATCH] support printing changed properties in svnlook
Posted by Erik Johansson <er...@ejohansson.se>.
On Fri, Dec 10, 2010 at 12:55, Julian Foad <ju...@wandisco.com> wrote:
> 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:
Yes, I've also found this when doing some more testing after Daniel's
mail. I fixed it by calling svn_fs_check_path() and checking that node
kind != svn_node_none before calling svn_fs_node_prop(). It will be in
the next patch version.
Thanks for testing.
// Erik
--
Erik Johansson
Home Page: http://ejohansson.se/
PGP Key: http://ejohansson.se/erik.asc
Re: [PATCH] support printing changed properties in svnlook
Posted by Julian Foad <ju...@wandisco.com>.
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....@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
>
Re: [PATCH] support printing changed properties in svnlook
Posted by Erik Johansson <er...@ejohansson.se>.
On Wed, Dec 8, 2010 at 23:22, Daniel Shahaf <d....@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
Re: [PATCH] support printing changed properties in svnlook
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
[ 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-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?
> + /** 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.