[PATCH] (2nd attempt) Completed TODO: svn_stream_copy3()

[Edmund Wong <ed...@kdtc.net>]

Hi,

My bad.  Here's a better patch.  The previous patch
bombed due to invalid use of the *.  :)  Sorry.  Should've
compiled it before sending it.  My bad.

Log:

[[[
        Completed the TODO as stated in svn_io.h for
     svn_stream_copy3().

     * subversion/libsvn_subr/stream.c
       (svn_stream_copy3): Even on error exit, the two files
                           from and to are closed.

      Patch by: Edmund Wong <ed...@belfordhk.com>

]]]

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1590330

Re: [PATCH] (2nd attempt) Completed TODO: svn_stream_copy3()

[Edmund Wong <ed...@kdtc.net>]

Martin Furter wrote:
> On Wed, 8 Apr 2009, Edmund Wong wrote:
> 
> Often the variable is just named 'err', or 'err2'.
> 
> The following looks very complicated. Why not just something like this:
> 
>    err = somefunc();
>    if (err)
>      break;
> 
> As soon as there is an error you can leave the loop as has been done until 
> now.

I'll redo the patch and repost it.

Thanks for the review.  Very much appreciated.

Edmund

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1612661

Re: [PATCH] (2nd attempt) Completed TODO: svn_stream_copy3()

[Martin Furter <mf...@rola.ch>]

On Wed, 8 Apr 2009, Edmund Wong wrote:

> Hi,
>
> My bad.  Here's a better patch.  The previous patch
> bombed due to invalid use of the *.  :)  Sorry.  Should've
> compiled it before sending it.  My bad.
>
> Log:
>
> [[[
>        Completed the TODO as stated in svn_io.h for
>     svn_stream_copy3().
>
>     * subversion/libsvn_subr/stream.c
>       (svn_stream_copy3): Even on error exit, the two files
>                           from and to are closed.
>
>      Patch by: Edmund Wong <ed...@belfordhk.com>
>
> ]]]
>
> ------------------------------------------------------

> Index: subversion/libsvn_subr/stream.c
> ===================================================================
> --- subversion/libsvn_subr/stream.c	(revision 37093)
> +++ subversion/libsvn_subr/stream.c	(working copy)
> @@ -210,6 +210,8 @@
>                                apr_pool_t *scratch_pool)
>  {
>    char *buf = apr_palloc(scratch_pool, SVN__STREAM_CHUNK_SIZE);
> +  svn_error_t *svn_err__save_error; 
> +  svn_error_t *svn_err__save_error2;

Often the variable is just named 'err', or 'err2'.

The following looks very complicated. Why not just something like this:

   err = somefunc();
   if (err)
     break;

As soon as there is an error you can leave the loop as has been done until 
now.

>    /* Read and write chunks until we get a short read, indicating the
>       end of the stream.  (We can't get a short write without an
> @@ -218,18 +220,54 @@
>      {
>        apr_size_t len = SVN__STREAM_CHUNK_SIZE;
> 
> -      if (cancel_func)
> -        SVN_ERR(cancel_func(cancel_baton));
> -
> -      SVN_ERR(svn_stream_read(from, buf, &len));
> -      if (len > 0)
> -        SVN_ERR(svn_stream_write(to, buf, &len));
> -      if (len != SVN__STREAM_CHUNK_SIZE)
> -        break;
> +      if (cancel_func) 
> +        svn_err__save_error = cancel_func(cancel_baton);
> + 
> +      if (! svn_err__save_error)
> +        { 
> +         svn_err__save_error = svn_stream_read(from, buf, &len);
> +         if (! svn_err__save_error)
> +          {
> +            if (len > 0)
> +              {
> +               svn_err__save_error = svn_stream_write(to, buf, &len);
> +               if (! svn_err__save_error)
> +                 {
> +                   if (len != SVN__STREAM_CHUNK_SIZE)
> +                      break;
> +                 }
> +               else
> +                 {
> +                     break;
> +                 }
> +              }
> +          }
> +         else
> +          {
> +             break;
> +          }
> +        }
> +       else
> +        {
> +           break;
> +        }
>      }
> 
> -  return svn_error_compose_create(svn_stream_close(from),
> -                                  svn_stream_close(to));
> +    if (!svn_err__save_error)
> +     {
> +        svn_err__save_error = svn_error_compose_create(svn_stream_close(from),
> +                                                       svn_stream_close(to));
> +     } 
> +    else
> +     {
> +        svn_err__save_error2 = svn_error_compose_create(svn_stream_close(from),
> +                                                        svn_stream_close(to));
> +        svn_err__save_error  = svn_error_compose_create(svn_err__save_error2,
> +                                                        svn_err__save_error);
> +     }
> + 
> +
> +   return svn_err__save_error;

No if needed here, svn_error_compose_create takes care of that. You could 
do something like this:

   err2 = svn_error_compose_create(close(from), close(to);
   return svn_error_compose_create(err, err2);

That way the error from the loop is the first in the chain, followed by 
the error from closing 'from', then that from closing 'to'. If one of the 
errors is SVN_NO_ERROR it's just not added to the chain.
(Don't forget to initialize 'err' to 'SVN_NO_ERROR')

>  }
> 
>  svn_error_t *

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1606340