> I understand the spirit of this change, but are you sure you did it
> correctly?
>
> For example:
>
> > Modified: trunk/subversion/libsvn_subr/subst.c
> >
>
==============================================================================
> > --- trunk/subversion/libsvn_subr/subst.c (original)
> > +++ trunk/subversion/libsvn_subr/subst.c Thu Dec 11 13:16:39 2003
> > @@ -735,33 +735,35 @@
> > eol_str, repair, keywords, expand);
> >
> > if (err)
> > - {
> > - /* ignore closure errors if we're bailing. */
> > - svn_error_clear (svn_stream_close (src_stream));
> > - svn_error_clear (svn_stream_close (dst_stream));
> > - if (s)
> > - apr_file_close (s);
> > - if (d)
> > - apr_file_close (d);
> > -
> > - svn_error_clear (svn_io_remove_file (dst_tmp, pool));
> > - return
> > - svn_error_createf (err->apr_err, err,
> > - "File translation failed when copying '%s'
> to '%s'",
> > - src, dst);
> > - }
> > + goto error;
>
> The specific error message disappears here, but does not reappear
> anywhere else. It has been lost.
Yes, but the svn_io wrapper in io.c added an error message which describes
what actually went wrong. If there was not enough disc space to do the
translation, that information would have been lost before this patch.
> Also, the old code tested s and d before closing them; the new code
> does not (I followed the code path down to do_io_file_wrapper_cleanup
> in io.c, it just assumes that the file is not null). Is this safe?
In case there is an error SVN_ERR () prevents the code path from ever
reaching the point where d or s is closed. Even if the pointer were NULL
(hypothetical case!), having a successfull open would necessarily mean that it is
closeable (and close should be able to handle that condition).
Looking at the code in apr_file_open, it cannot complete successfully
without setting d or s to a non-NULL value. (Checked it in case the above does not
ease you.)
> -Karl, beginning to think Greg Stein is right about being stricter on
> all trunk commits starting now...
I'll start sending my patches to the list. (which I did for this one BTW.)
And I'll start waiting for votes. (which I didn't do longer than 24h -
presumably too short as you are all so busy)
bye,
Erik.
--
+++ GMX - die erste Adresse für Mail, Message, More +++
Neu: Preissenkung für MMS und FreeMMS! http://www.gmx.net
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Dec 11 22:02:21 2003