Greg Stein <gstein@lyra.org> writes:
> > - baton = apr_pcalloc(pool, sizeof(*baton));
> > - baton->file = file;
> > + baton = apr_pcalloc(file->cc->ras->pool, sizeof(*baton));
>
> Woah! That is a session-long pool. This change means that everything will
> now get accumulated into one pool, whereas we used to use an iterative pool.
> This should be changed to use the file_pool passed to open/add_file.
>
> (which also implies a need to store that pool in the resource_baton_t)
>
> Also, I note that the POOL argument is not documented for apply_textdelta.
> Specifically, that it will be destroyed after the "null window" is passed to
> the window handler.
Good point, thanks! Will fix.
> > static svn_error_t * commit_close_file(void *file_baton,
> > + const char *text_checksum,
> > apr_pool_t *pool)
> > {
> >...
> > resource_baton_t *file = file_baton;
> > + commit_ctx_t *cc = file->cc;
> > + const char *url = file->rsrc->wr_url;
> > + ne_request *req;
> > + int code;
> > + svn_error_t *err;
> > +
> > + if (file->put_baton)
> > + {
> > + put_baton_t *pb = file->put_baton;
> > +
>
> The variables at the outermost block should move inside, as appropriate.
I'll buy that :-).
> >...
> > + /* run the request and get the resulting status code (and svn_error_t) */
> > + err = svn_ra_dav__request_dispatch(&code, req, cc->ras->sess,
> > + "PUT", url,
> > + 201 /* Created */,
> > + 204 /* No Content */,
> > + cc->ras->pool);
>
> That should just be "pool".
>
> I'm also guessing that you might be able to fully deref the session, rather
> than just to "cc" and further deref'ing several times.
Okay.
> > +++ trunk/subversion/libsvn_ra_dav/fetch.c Fri Apr 4 15:22:22 2003
> >...
> > @@ -736,8 +735,7 @@
> > simple_fetch_file() params related to fetching version URLs (for
> > fetching deltas) */
> > err = simple_fetch_file(sess, bc_url, NULL, TRUE, compression, file_baton,
> > - NULL, (checksum ? checksum->data : NULL),
> > - editor, NULL, NULL, pool);
> > + NULL, editor, NULL, NULL, pool);
> > if (err)
> > {
> > /* ### do we really need to bother with closing the file_baton? */
> > @@ -751,7 +749,7 @@
> > err = store_vsn_url(rsrc, file_baton, editor->change_file_prop, pool);
> >
> > error:
> > - err2 = (*editor->close_file)(file_baton, pool);
> > + err2 = (*editor->close_file)(file_baton, checksum->data, pool);
> > return err ? err : err2;
>
> That error path could choke if checksum==NULL. The previous code had a test
> for that.
Oh! My goodness; in fact, when I had some debugging printf's in
there, *they* also checked for null. Will fix :-).
> > +++ trunk/subversion/libsvn_repos/checkout.c Fri Apr 4 15:22:22 2003
> > @@ -22,6 +22,8 @@
> > #include "svn_path.h"
> > #include "svn_pools.h"
> >
> > +#include <apr_md5.h>
> > +#include "svn_md5.h"
>
> Normally, the APR headers are before the SVN headers.
Check.
> >...
> > @@ -453,6 +454,38 @@
> >
> >
> > static svn_error_t *
> > +close_file (void *file_baton,
> > + const char *text_checksum,
> > + apr_pool_t *pool)
> > +{
> > + struct file_baton *fb = file_baton;
> > +
> > + if (text_checksum)
> > + {
> > + unsigned char digest[MD5_DIGESTSIZE];
> > + const char *hex_digest;
> > +
> > + SVN_ERR (svn_fs_file_md5_checksum
> > + (digest, fb->edit_baton->txn_root, fb->path, fb->pool));
> > + hex_digest = svn_md5_digest_to_cstring (digest, fb->pool);
>
> These operations should use the pool passed to close_file().
>
> Hmm. That pool is doc'd in svn_delta.h either.
Yes, will do.
(Note there may be some `implied' docs for the pool arguments to
editor functions, will see.)
> > +++ trunk/subversion/libsvn_wc/diff.c Fri Apr 4 15:22:22 2003
> >...
> > @@ -1053,9 +1052,13 @@
> >
> > /* An editor function. When the file is closed we have a temporary
> > * file containing a pristine version of the repository file. This can
> > - * be compared against the working copy. */
> > + * be compared against the working copy.
> > + *
> > + * Ignore TEXT_CHECKSUM.
> > + */
>
> Why ignore it? Please explain in the comment, or leave a ### to have this
> fixed later on.
>
> (this file is coming from the repository, right? so a checksum error would
> be nice to have)
Yup. "###" is the way to go for now, I think.
> >...
> > +++ trunk/subversion/libsvn_wc/status_editor.c Fri Apr 4 15:22:22 2003
> >...
> > @@ -507,6 +506,7 @@
> >
> > static svn_error_t *
> > close_file (void *file_baton,
> > + const char *text_checksum,
> > apr_pool_t *pool)
> > {
>
> Does the same comment apply here?
I'll check.
> >...
> > +++ trunk/subversion/libsvn_wc/update_editor.c Fri Apr 4 15:22:22 2003
> >...
> > @@ -1814,6 +1821,7 @@
> > /* Mostly a wrapper around svn_wc_install_file. */
> > static svn_error_t *
> > close_file (void *file_baton,
> > + const char *text_checksum,
> > apr_pool_t *pool)
> > {
> > struct file_baton *fb = file_baton;
> > @@ -1824,7 +1832,23 @@
> >
> > /* window-handler assembles new pristine text in .svn/tmp/text-base/ */
> > if (fb->text_changed)
> > - new_text_path = svn_wc__text_base_path (fb->path, TRUE, fb->pool);
> > + {
> > + new_text_path = svn_wc__text_base_path (fb->path, TRUE, fb->pool);
>
> May as well have changed this to just "pool" while you were here.
>
> (it does look like the whole function needs to be switched away from
> fb->pool; in fact, the very notion of fb->pool should probably die)
Will do.
Nice review, and timely too! Thanks, Greg.
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Apr 5 01:06:12 2003