You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by "Sergey A. Lipnevich" <se...@optimaltec.com> on 2003/08/08 04:23:08 UTC
Neon 0.24 support
Hi All,
I've applied Neon 0.24-related changes to the branch in several stages.
It passes all the tests (over DAV) for Neon 0.23.9, and most of the
tests with 0.24 (it fails 17 tests total in 8 test packages). While I
continue banging on remaining issues, which are mostly to do with SVN
properties support, I'd greatly appreciate an intermediate review so
that I know I'm moving forward. Could somebody (MBK?) please review the
neon-0.24 branch changes, at least briefly?
Below is the list of failing tests, please let me know if you have any
ideas on what might be a uniting error behind them all.
Thank you!
Sergey.
FAIL: basic_tests.py 18: basic import of executable files
FAIL: update_tests.py 6: delete files and update to resolve text conflicts
FAIL: update_tests.py 12: receive prop update to file scheduled for
deletion
FAIL: prop_tests.py 3: receive properties via update
FAIL: prop_tests.py 6: update with conflicting props
FAIL: prop_tests.py 13: test binary property support
FAIL: copy_tests.py 8: copy files with properties
FAIL: copy_tests.py 12: working-copy to repository copy
FAIL: diff_tests.py 11: don't diff file marked as binary type
FAIL: externals_tests.py 1: check out a directory with some external
modules attached
FAIL: externals_tests.py 2: update to receive a new external module.
FAIL: externals_tests.py 3: update to lose an external module.
FAIL: externals_tests.py 5: update to receive a change to a modified
external module.
FAIL: externals_tests.py 6: update to receive a change under an
external module.
FAIL: merge_tests.py 4: some simple property merges
FAIL: trans_tests.py 7: keyword expanded on checkout for only file in a
directory
FAIL: trans_tests.py 8: keyword expanded on cat
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Neon 0.24 support
Posted by kf...@collab.net.
"Sergey A. Lipnevich" <se...@optimaltec.com> writes:
> No, no! I'm not going to get build/* files changes in, just
> ra_dav. The way it's done right now, the brunt of the change is to
> replace many identifiers and introduce some bridging code, and do Neon
> 0.24 support within #defines which will only be activated with newer
> Neon (which is not going to happen because of build check doesn't
> change). Hence, the current logic stays intact, Neon 0.24 is not
> supported on the trunk, and subsequent merges become easier.
> I'm almost done with file:// tests, http:// will be next, than I merge
> the subversion/* part, and then merge trunk back into branch (after
> finding out what revision the branch was made in).
Ah! Heh, sorry, I didn't need to post this, I guess:
> Sergey, if you want to merge just so you can get a clean
> rebranch, that's fine. Just make sure that the 0.24 side gets
> disabled somehow (in the configury check maybe?), so that trunk
> Subversion remains formally incompatible with Neon 0.24. This
> way will be least confusing for users.
Should have read all your mails before replying.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Neon 0.24 support
Posted by "Sergey A. Lipnevich" <se...@optimaltec.com>.
Ben Collins-Sussman wrote:
>kfogel@collab.net writes:
>
>
>
>>If all the tests pass, +1 on merging. You do the honors :-) (your
>>commit access will work fine, there's no automated restriction on the
>>server, just human convention).
>>
>>
>
>Do we really want to merge the neon-0.24 branch just yet? David Waite
>still has a ton of SSL changes to make. If we merge the branch now,
>then it's possible someone will casually link HEAD svn against
>neon-0.24, and bam, the cert stuff either won't compile or won't run.
>
>
>
No, no! I'm not going to get build/* files changes in, just ra_dav. The
way it's done right now, the brunt of the change is to replace many
identifiers and introduce some bridging code, and do Neon 0.24 support
within #defines which will only be activated with newer Neon (which is
not going to happen because of build check doesn't change). Hence, the
current logic stays intact, Neon 0.24 is not supported on the trunk, and
subsequent merges become easier.
I'm almost done with file:// tests, http:// will be next, than I merge
the subversion/* part, and then merge trunk back into branch (after
finding out what revision the branch was made in).
Sergey.
Re: Neon 0.24 support
Posted by Ben Collins-Sussman <su...@collab.net>.
kfogel@collab.net writes:
> If all the tests pass, +1 on merging. You do the honors :-) (your
> commit access will work fine, there's no automated restriction on the
> server, just human convention).
Do we really want to merge the neon-0.24 branch just yet? David Waite
still has a ton of SSL changes to make. If we merge the branch now,
then it's possible someone will casually link HEAD svn against
neon-0.24, and bam, the cert stuff either won't compile or won't run.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Neon 0.24 support
Posted by kf...@collab.net.
"Sergey A. Lipnevich" <se...@optimaltec.com> writes:
> Done. I must add that there were not many of such extensive comments
> in ra_dav implementation ;-).
Yes, sorry about that. The DAV code is less documented than the rest
of Subversion. That's something we'd like to change, but... only 24
hours in a day.
> > So I guess the whole review boils down to: documentation,
> > documentation, documentation :-). But even as it is, the changes are
> > pretty clear. Looking forward to the merge...
>
> I've run the tests and I'd like to ask for an intermediate merge of
> the branch except for build/buildcheck.sh.
If all the tests pass, +1 on merging. You do the honors :-) (your
commit access will work fine, there's no automated restriction on the
server, just human convention).
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Neon 0.24 support
Posted by "Sergey A. Lipnevich" <se...@optimaltec.com>.
Thanks for such a thorough review! Comments below.
kfogel@collab.net wrote:
> kfogel@collab.net writes:
>
> Sergey, I've reviewed the changes so far. Nice work! I like your
> one-step-at-a-time approach very much, it makes the changes quite
> comprehensible.
Thanks :-)!
>
> My review was more about the overall shape of the changes; I haven't
> looked into the failing tests (sorry -- I hope they're just a simple
> matter of debugging).
I think they may be related to the COLLECT flag for properties elements.
Specifically, there's a single usage of it like this:
{ "", "", ELEM_unknown, SVN_RA_DAV__XML_COLLECT }
which means all unknown properties must be collected as CDATA.
So far, I don't see how Neon 0.24 interface allows for that, but I'll
keep looking. I think I'd have to bug Joe Orton about this ;-).
> In ra_dav.h, you added:
>
> +/* Push an XML handler onto Neon's handler stack */
> +void svn_ra_dav__xml_push_handler(ne_xml_parser *p,
> + const svn_ra_dav__xml_elm_t *elements,
> + svn_ra_dav__xml_validate_cb validate_cb,
> + svn_ra_dav__xml_startelm_cb startelm_cb,
> + svn_ra_dav__xml_endelm_cb endelm_cb,
> + void *userdata,
> + apr_pool_t *pool);
>
> In general, we document every argument to a function. But there can
> be exceptions, when the function is just a trivial wrapper around
> something else, which is the case here. But then, you should mention
> the something else, and explain why to use the wrapper.
Done. I must add that there were not many of such extensive comments in
ra_dav implementation ;-).
> In revision 6670, you added the 'davautocheck.sh' script. Again, the
> log message documents the script, but there is no documentation in the
> tree (not even in the script itself!). Even if you intend it to live
> only on the branch, it's best to give some explanation of what it is
> for. (By the way, if you plan to transfer it to trunk eventually,
> consider putting it in subversion/tests/.)
Done.
>
> Regarding this (from rev 6671's log message):
>
> > The replacements have been done using `perl -pi -e' command, so I
> > have to apologize if the lines got longer than 80 characters in
> > some files.
>
> No problem for the branch, but note that the long lines should be easy
> to find & fix: just run 'svn diff' and look for overly long lines :-).
> Consider doing this in the branch before you merge to trunk.
MBK fixed that for me, yeeha! I don't like this line length issue.
> Thanks for the "/* FIXME: Neon SSL API change */" comments -- and I'm
> sure David Waite agrees :-).
Sure.
> In libsvn_ra_dav/util.c, you define:
>
> +/* Finds a given element in the table of elements. If element is not found
> + * tries to find and return `unknown' element. If that is not found, returns
> + * NULL pointer. Uses a single loop to save CPU cycles. */
> +static const svn_ra_dav__xml_elm_t *lookup_elem(
> + const svn_ra_dav__xml_elm_t *table,
> + const char *nspace,
> + const char *name)
>
> Even for internal static functions, the documentation string should
> mention the arguments by name and state precisely how they are used,
> and state the return value formally (it's really "ELEM_unknown", not
> just "unknown"). The fact that it uses a single loop to save CPU
> cycles is not part of the function's interface -- a comment like that
> belongs inside the function, above the for-loop, not in the function's
> documentation.
Done.
> Similar documentation tips apply to 'shim_startelm' and all the other
> new wrappers. If they are wrappers, state what they wrap, and
> formally describe the functionality they add. You don't need to be
> verbose, just complete: supply all the information a reader needs to
> understand what the function does. For example, in the shim_*
> functions, the single most important fact is what type the 'userdata'
> argument is interpreted as.
Done.
> So I guess the whole review boils down to: documentation,
> documentation, documentation :-). But even as it is, the changes are
> pretty clear. Looking forward to the merge...
I've run the tests and I'd like to ask for an intermediate merge of the
branch except for build/buildcheck.sh.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Neon 0.24 support
Posted by cm...@collab.net.
kfogel@collab.net writes:
> In general, we document every argument to a function. But there can
> be exceptions, when the function is just a trivial wrapper around
> something else, which is the case here. But then, you should mention
> the something else, and explain why to use the wrapper.
Doh! Thanks Karl, you just reminded me about one of my TODO tasks
(unrelated to Neon 0.24).
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Neon 0.24 support
Posted by kf...@collab.net.
kfogel@collab.net writes:
> "Sergey A. Lipnevich" <se...@optimaltec.com> writes:
> > I've applied Neon 0.24-related changes to the branch in several
> > stages. It passes all the tests (over DAV) for Neon 0.23.9, and most
> > of the tests with 0.24 (it fails 17 tests total in 8 test
> > packages). While I continue banging on remaining issues, which are
> > mostly to do with SVN properties support, I'd greatly appreciate an
> > intermediate review so that I know I'm moving forward. Could somebody
> > (MBK?) please review the neon-0.24 branch changes, at least briefly?
>
> I will take a look at these tomorrow too, Sergey. (Though I don't
> want to stop MBK or anyone else from doing so as well.)
Sergey, I've reviewed the changes so far. Nice work! I like your
one-step-at-a-time approach very much, it makes the changes quite
comprehensible.
My review was more about the overall shape of the changes; I haven't
looked into the failing tests (sorry -- I hope they're just a simple
matter of debugging).
Just a few suggestions:
In ra_dav.h, you added:
+/* Push an XML handler onto Neon's handler stack */
+void svn_ra_dav__xml_push_handler(ne_xml_parser *p,
+ const svn_ra_dav__xml_elm_t *elements,
+ svn_ra_dav__xml_validate_cb validate_cb,
+ svn_ra_dav__xml_startelm_cb startelm_cb,
+ svn_ra_dav__xml_endelm_cb endelm_cb,
+ void *userdata,
+ apr_pool_t *pool);
In general, we document every argument to a function. But there can
be exceptions, when the function is just a trivial wrapper around
something else, which is the case here. But then, you should mention
the something else, and explain why to use the wrapper.
Actually, you did document it in your log message:
"new function on top of ne_xml_push_handler, adds memory pool as the
last parameter. This parameter is not used with Neon 0.23.9, but
will be with Neon 0.24."
...so what I'm saying is, that bit from the log message should be in
the code instead. As the HACKING file says:
> One should never need the log entries to understand the current
> code. If you find yourself writing a significant explanation in
> the log, you should consider carefully whether your text doesn't
> actually belong in a comment, alongside the code it explains.
(I would have said the same for the other new definitions added to
ra_dav.h in rev 6672, but I see that 6673 added the missing
explanations.)
In revision 6670, you added the 'davautocheck.sh' script. Again, the
log message documents the script, but there is no documentation in the
tree (not even in the script itself!). Even if you intend it to live
only on the branch, it's best to give some explanation of what it is
for. (By the way, if you plan to transfer it to trunk eventually,
consider putting it in subversion/tests/.)
Regarding this (from rev 6671's log message):
> The replacements have been done using `perl -pi -e' command, so I
> have to apologize if the lines got longer than 80 characters in
> some files.
No problem for the branch, but note that the long lines should be easy
to find & fix: just run 'svn diff' and look for overly long lines :-).
Consider doing this in the branch before you merge to trunk.
Thanks for the "/* FIXME: Neon SSL API change */" comments -- and I'm
sure David Waite agrees :-).
In libsvn_ra_dav/util.c, you define:
+/* Finds a given element in the table of elements. If element is not found
+ * tries to find and return `unknown' element. If that is not found, returns
+ * NULL pointer. Uses a single loop to save CPU cycles. */
+static const svn_ra_dav__xml_elm_t *lookup_elem(
+ const svn_ra_dav__xml_elm_t *table,
+ const char *nspace,
+ const char *name)
Even for internal static functions, the documentation string should
mention the arguments by name and state precisely how they are used,
and state the return value formally (it's really "ELEM_unknown", not
just "unknown"). The fact that it uses a single loop to save CPU
cycles is not part of the function's interface -- a comment like that
belongs inside the function, above the for-loop, not in the function's
documentation.
Similar documentation tips apply to 'shim_startelm' and all the other
new wrappers. If they are wrappers, state what they wrap, and
formally describe the functionality they add. You don't need to be
verbose, just complete: supply all the information a reader needs to
understand what the function does. For example, in the shim_*
functions, the single most important fact is what type the 'userdata'
argument is interpreted as.
So I guess the whole review boils down to: documentation,
documentation, documentation :-). But even as it is, the changes are
pretty clear. Looking forward to the merge...
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Neon 0.24 support
Posted by kf...@collab.net.
"Sergey A. Lipnevich" <se...@optimaltec.com> writes:
> I've applied Neon 0.24-related changes to the branch in several
> stages. It passes all the tests (over DAV) for Neon 0.23.9, and most
> of the tests with 0.24 (it fails 17 tests total in 8 test
> packages). While I continue banging on remaining issues, which are
> mostly to do with SVN properties support, I'd greatly appreciate an
> intermediate review so that I know I'm moving forward. Could somebody
> (MBK?) please review the neon-0.24 branch changes, at least briefly?
> Below is the list of failing tests, please let me know if you have any
> ideas on what might be a uniting error behind them all.
I will take a look at these tomorrow too, Sergey. (Though I don't
want to stop MBK or anyone else from doing so as well.)
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org