You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Malcolm Rowe <ma...@farside.org.uk> on 2007/03/15 17:41:24 UTC
Sparse directories - mini-review
Hi Karl,
Since you mentioned that you were planning to merge the
sparse-directories branch to trunk shortly, I thought I'd take a quick
look at the branch diff. I've not reviewed it thoroughly, but here's
some random comments:
Meta-comments:
- There's an awful lot of TODOs, but I get the sense that they're intended
to be exhaustive rather than indicate where you think there's a problem
that absolutely needs fixing.
- Is the intention to get sparse directories to work completely for 1.5,
or just to get it to work at least as well as -N currently?
- If the latter, the only blocker for release seems to be the work needed
to make a client with a sparse wc ignore data sent by sparse-ignorant
servers. What exactly is the status of this? Is this something that's
absolutely necessary for 1.5?
ra_svn:
- ra_svn: In the 'update', 'switch', 'status', and 'diff' commands,
we're adding the depth inside a tuple, whereas in the 'set-path' and
'link-path' cases we're not. The former matches what the extensibility
parts of the protocol document suggest, but the latter makes more sense
to me (I don't understand what the protocol document is saying here).
Regardless, can we pick one approach?
- libsvn_ra_svn/protocol is missing the changes to all commands apart from
set-path and link-path.
- svnserve/serve.c:diff() - You've added an optional depth tuple inside
code that checks for a 1.4 request (one without text_deltas).
You should set depth_word to NULL directly rather than rely on
svn_ra_svn_parse_tuple to do it for you - it's just confusing as-is.
Other:
- (This isn't exclusive to sparse-directories: changelist and keep-local
do it too, but:) Why are we writing out the depth attribute in the
old XML-format entries, when by definition it can never have any value
other than the default? (Also note that the specification for the
entries XML format prohibits unknown attributes, so this is
technically wrong).
(Actually, could someone remind me why we even have the capability to
write out the XML entries format? I'm sure there is a reason, but I
can't remember what it is.)
- The help for the command-line option --depth doesn't mention that
'infinity' is a valid option (though it is referenced in the help for
--recursive).
- If we're updating the XML output for 'svn info', we should update the
schema too (in subversion/schema/info.rnc).
- svn_types.h: Contrary to the comments, you use svn_depth_unknown all
over the place. However, you don't use svn_depth_exclude -- are you
fairly sure you'll need it, or would it be better to get rid of it
until a need comes up?
- libsvn_wc/entries.h has a reference to non-existent svn_depth_ignore.
- Looking at the branch diff for include/svn_ra.h, I notice you deleted
a lot of comments of the form "the RA layer assumes that all paths are
relative to the URL used to open @a session." (see r21314). Any reason?
It seems unrelated to the other changes.
Regards,
Malcolm
Re: Sparse directories - mini-review
Posted by Karl Fogel <kf...@red-bean.com>.
Daniel Rall <dl...@collab.net> writes:
> Thanks Karl. Malcolm and I briefly debated doing that, but decided we
> could do it after the merge. FWIW, you even have have a "TODO(sd)"
> note in one API where you *don't* do this (instead following the
> scheme desired by Peter).
Thanks for the heads up.
I have a houseguest this weekend, but I'll get to this as soon as I
can.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Sparse directories - mini-review
Posted by Karl Fogel <kf...@red-bean.com>.
Daniel Rall <dl...@collab.net> writes:
> Thanks Karl. Malcolm and I briefly debated doing that, but decided we
> could do it after the merge. FWIW, you even have have a "TODO(sd)"
> note in one API where you *don't* do this (instead following the
> scheme desired by Peter).
Patch-in-progress here: http://pastebin.ca/408580
Still working on it, so don't feel obligated to review or anything,
there'll be more soon. I just wanted to post this so Vlad would
understand why I haven't gotten to reviewing his patch yet! :-)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Sparse directories - mini-review
Posted by Daniel Rall <dl...@collab.net>.
On Thu, 22 Mar 2007, Karl Fogel wrote:
> plundblad@google.com (Peter Lundblad) writes:
> > Yeah, including the docstrings in the public headers if I recall correctly.
>
> Well, that does make less work for the reader -- you always only have
> to jump one reference level away, instead of N. (But I agree, it's
> not really worth it in the long run.)
>
> >> But how does it introduce code duplication? I agree the work wasn't
> >> needed, but I think it was harmless...
> >
> > In the trivial case the code duplication is obviously also trivial, but
> > sometimes compat wrappers aren't that trivial. Compat wrappers are one of the
> > areas we really undertest, so a cut-and-paste error here may well go into
> > a release undetected.
> >
> > The problem is that you set precedence by doing this in some
> > places. People who look on existing code will
> > continue this code churny activity and that's bad for
> > maintainability.
>
> That's a good point.
>
> > That's why I'd rather like to see the changes
> > to preexisting compat wrappers that were introduced on
> > the sparse-directories branch to go away again.
>
> That shouldn't be too hard, I'll take a look at the diff and see if I
> can make a reversal this weekend.
Thanks Karl. Malcolm and I briefly debated doing that, but decided we
could do it after the merge. FWIW, you even have have a "TODO(sd)"
note in one API where you *don't* do this (instead following the
scheme desired by Peter).
- Danx
Re: Sparse directories - mini-review
Posted by Karl Fogel <kf...@red-bean.com>.
plundblad@google.com (Peter Lundblad) writes:
> Yeah, including the docstrings in the public headers if I recall correctly.
Well, that does make less work for the reader -- you always only have
to jump one reference level away, instead of N. (But I agree, it's
not really worth it in the long run.)
>> But how does it introduce code duplication? I agree the work wasn't
>> needed, but I think it was harmless...
>
> In the trivial case the code duplication is obviously also trivial, but
> sometimes compat wrappers aren't that trivial. Compat wrappers are one of the
> areas we really undertest, so a cut-and-paste error here may well go into
> a release undetected.
>
> The problem is that you set precedence by doing this in some
> places. People who look on existing code will
> continue this code churny activity and that's bad for
> maintainability.
That's a good point.
> That's why I'd rather like to see the changes
> to preexisting compat wrappers that were introduced on
> the sparse-directories branch to go away again.
That shouldn't be too hard, I'll take a look at the diff and see if I
can make a reversal this weekend.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Sparse directories - mini-review
Posted by Peter Lundblad <pl...@google.com>.
Karl Fogel writes:
> You mean, the way I upgraded all the old wrappers in a chronological
> chain of API upgrades? For some reason I thought that was desirable,
> but then I realized it's pretty pointless (function-call overhead is
> not that high), and stopped doing it.
>
Yeah, including the docstrings in the public
headers if I recall correctly.
> But how does it introduce code duplication? I agree the work wasn't
> needed, but I think it was harmless...
In the trivial case the code duplication is obviously also trivial, but
sometimes compat wrappers aren't that trivial. Compat wrappers are one of the
areas we really undertest, so a cut-and-paste error here may well go into
a release undetected.
The problem is that you set precedence by doing this in some
places. People who look on existing code will
continue this code churny activity and that's bad for
maintainability.
That's why I'd rather like to see the changes
to preexisting compat wrappers that were introduced on
the sparse-directories branch to go away again.
thanks,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Sparse directories - mini-review
Posted by Karl Fogel <kf...@red-bean.com>.
plundblad@google.com (Peter Lundblad) writes:
> Another thing I noted when I reviewed, but didn't bring up yet:
> When functions are revised on this branch, *all* compatibility
> wrappers are updated. That's not what we've been doing before. Is
> there a specific reason for that? If not, I think that's bad idea
> because it introduces code duplication in code that we test very
> seldom. that's not a big deal when the wrappers are trivial, but
> sometimes they aren't. This might be easier to get fixed befre the
> branch is merged.
You mean, the way I upgraded all the old wrappers in a chronological
chain of API upgrades? For some reason I thought that was desirable,
but then I realized it's pretty pointless (function-call overhead is
not that high), and stopped doing it.
But how does it introduce code duplication? I agree the work wasn't
needed, but I think it was harmless...
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Sparse directories - mini-review
Posted by Peter Lundblad <pl...@google.com>.
Malcolm Rowe writes:
> - ra_svn: In the 'update', 'switch', 'status', and 'diff' commands,
> we're adding the depth inside a tuple, whereas in the 'set-path' and
> 'link-path' cases we're not. The former matches what the extensibility
> parts of the protocol document suggest, but the latter makes more sense
> to me (I don't understand what the protocol document is saying here).
> Regardless, can we pick one approach?
>
The depth parameters to the svn_ra_ functions that give back a
reporter are supposed to go away. They are unnecessary. That's why I
didn't fix the ra_svn protocol stuff for those.
> - (This isn't exclusive to sparse-directories: changelist and keep-local
> do it too, but:) Why are we writing out the depth attribute in the
> old XML-format entries, when by definition it can never have any value
> other than the default? (Also note that the specification for the
> entries XML format prohibits unknown attributes, so this is
> technically wrong).
>
Yup. Good catch! I didn't notice that during my review.
> (Actually, could someone remind me why we even have the capability to
> write out the XML entries format? I'm sure there is a reason, but I
> can't remember what it is.)
>
We need to be able to cleanup a directory that hasn't yet been
upgraded to
the new format. Boring, but true.
Another thing I noted when I reviewed, but didn't bring up yet:
When functions are revised on this branch, *all* compatibility
wrappers are updated. That's not what we've been doing before. Is
there a specific reason for that? If not, I think that's bad idea
because it introduces code duplication in code that we test very
seldom. that's not a big deal when the wrappers are trivial, but
sometimes they aren't. This might be easier to get fixed befre the
branch is merged.
Thanks,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Sparse directories - mini-review
Posted by Malcolm Rowe <ma...@farside.org.uk>.
On Thu, Mar 15, 2007 at 05:41:24PM +0000, Malcolm Rowe wrote:
> - svnserve/serve.c:diff() - You've added an optional depth tuple inside
> code that checks for a 1.4 request (one without text_deltas).
> You should set depth_word to NULL directly rather than rely on
> svn_ra_svn_parse_tuple to do it for you - it's just confusing as-is.
>
I fixed this in r23927.
> - The help for the command-line option --depth doesn't mention that
> 'infinity' is a valid option (though it is referenced in the help for
> --recursive).
>
Dan fixed this in r23912.
Regards,
Malcolm
Re: XML parsing/writing in libsvn_wc
Posted by Ivan Zhakov <ch...@gmail.com>.
On 3/22/07, Malcolm Rowe <ma...@farside.org.uk> wrote:
> On Tue, Mar 20, 2007 at 04:07:34AM -0700, Karl Fogel wrote:
> > > - (This isn't exclusive to sparse-directories: changelist and keep-local
> > > do it too, but:) Why are we writing out the depth attribute in the
> > > old XML-format entries, when by definition it can never have any value
> > > other than the default? (Also note that the specification for the
> > > entries XML format prohibits unknown attributes, so this is
> > > technically wrong).
> >
> > Hmmm, good point. Undone in r23931.
> >
>
> Looking at this a little harder, I see that svn_wc__atts_to_entry() is
> also used for parsing the logs produced by svn_wc__loggy_entry_modify(),
> and so changelist, keep-local, and depth should probably all be included
> in both.
>
> However, I see that none of them are in svn_wc__loggy_entry_modify(),
> which makes me wonder how they're being updated anyway.
>
> I _think_ that 'depth' should be added back as an attribute, parsed in
> svn_wc__atts_to_entry(), _not_ added to write_xml_entry(), and somehow
> modified via svn_wc__loggy_entry_modify(). Changelist and keep-local
> also should be modified via svn_wc__loggy_entry_modify().
>
> Can someone who has a better idea how libsvn_wc works confirm whether
> I'm talking rubbish or not?
Yes, you are right. svn_wc__atts_to_entry should updated every time
when we add new fields.
We urgently need test suite for libsvn_wc! May be it could SoC task?
>
> Regards,
> Malcolm
> "Help, libsvn_wc is a big steamy mess".
>
>
--
Ivan Zhakov
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
XML parsing/writing in libsvn_wc
Posted by Malcolm Rowe <ma...@farside.org.uk>.
On Tue, Mar 20, 2007 at 04:07:34AM -0700, Karl Fogel wrote:
> > - (This isn't exclusive to sparse-directories: changelist and keep-local
> > do it too, but:) Why are we writing out the depth attribute in the
> > old XML-format entries, when by definition it can never have any value
> > other than the default? (Also note that the specification for the
> > entries XML format prohibits unknown attributes, so this is
> > technically wrong).
>
> Hmmm, good point. Undone in r23931.
>
Looking at this a little harder, I see that svn_wc__atts_to_entry() is
also used for parsing the logs produced by svn_wc__loggy_entry_modify(),
and so changelist, keep-local, and depth should probably all be included
in both.
However, I see that none of them are in svn_wc__loggy_entry_modify(),
which makes me wonder how they're being updated anyway.
I _think_ that 'depth' should be added back as an attribute, parsed in
svn_wc__atts_to_entry(), _not_ added to write_xml_entry(), and somehow
modified via svn_wc__loggy_entry_modify(). Changelist and keep-local
also should be modified via svn_wc__loggy_entry_modify().
Can someone who has a better idea how libsvn_wc works confirm whether
I'm talking rubbish or not?
Regards,
Malcolm
"Help, libsvn_wc is a big steamy mess".