Re: svn commit: rev 7468 - in trunk/subversion: libsvn_ra_dav mod_dav_svn

[Greg Stein <gs...@lyra.org>]

On Tue, Oct 21, 2003 at 04:05:29PM -0500, kfogel@tigris.org wrote:
>...
> +++ trunk/subversion/libsvn_ra_dav/fetch.c	Tue Oct 21 16:05:28 2003
> @@ -2042,6 +2042,9 @@
>    const char *vcc;
>    int http_status;
>  
> +  SVN_ERR (svn_ra_dav__get_vcc
> +           (&vcc, rb->ras->sess, rb->ras->url, rb->ras->pool));
>...
> -  err = svn_ra_dav__get_vcc(&vcc, rb->ras->sess, rb->ras->url, rb->ras->pool);
> -  if (err)
> -    {
> -      (void) apr_file_close(rb->tmpfile);
> -      return err;
> -    }

Previously, a failure in getting the VCC would close the file. You altered
the behavior here -- it doesn't close that file before throwing an error.

While it might not be a problem, in practice (it will eventually be closed
by a pool clear/destory), I don't think the postcondition of this function
should be altered.

If you *do* want to keep the SVN_ERR() because (I believe) it will always
throw the error "far enough" to toss the pool and the file, then I'd
suggest altering the code just after that line. That code closes the file
before returning. IOW, before your change, the consistent approach was
"close before return". "close by pool" is probably valid, but the two
error/return conditions should be sync'd.

Cheers,
-g

-- 
Greg Stein, http://www.lyra.org/

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: svn commit: rev 7468 - in trunk/subversion: libsvn_ra_dav mod_dav_svn

[kf...@collab.net]

"C. Michael Pilato" <cm...@collab.net> writes:
> Oh, poo.  I'll take a large degree of blame for that, since making
> this part of the change was my idea.  I saw that we were moving this
> call to a place prior to our use of the tmpfile, forgetting that while
> we weren't yet writing to the tmpfile, it had still already been
> created.

The blame is shared equally -- we didn't look far enough to see that
the file already exists, whether written to or not.

Hmm, two commits, two problems.  Not a good record for yesterday.


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: svn commit: rev 7468 - in trunk/subversion: libsvn_ra_dav mod_dav_svn

["C. Michael Pilato" <cm...@collab.net>]

Greg Stein <gs...@lyra.org> writes:

> On Tue, Oct 21, 2003 at 04:05:29PM -0500, kfogel@tigris.org wrote:
> >...
> > +++ trunk/subversion/libsvn_ra_dav/fetch.c	Tue Oct 21 16:05:28 2003
> > @@ -2042,6 +2042,9 @@
> >    const char *vcc;
> >    int http_status;
> >  
> > +  SVN_ERR (svn_ra_dav__get_vcc
> > +           (&vcc, rb->ras->sess, rb->ras->url, rb->ras->pool));
> >...
> > -  err = svn_ra_dav__get_vcc(&vcc, rb->ras->sess, rb->ras->url, rb->ras->pool);
> > -  if (err)
> > -    {
> > -      (void) apr_file_close(rb->tmpfile);
> > -      return err;
> > -    }
> 
> Previously, a failure in getting the VCC would close the file. You altered
> the behavior here -- it doesn't close that file before throwing an error.

Oh, poo.  I'll take a large degree of blame for that, since making
this part of the change was my idea.  I saw that we were moving this
call to a place prior to our use of the tmpfile, forgetting that while
we weren't yet writing to the tmpfile, it had still already been
created.

Will fix.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org