Re: [PATCH] Completed TODO: svn_stream_copy3()

Hey Edmund,

Thanks for stretching your svn coding legs with this one. I've got
some detailed feedback for you below:

On Fri, Apr 10, 2009 at 09:55, Edmund Wong <ed_at_kdtc.net> wrote:
>...
> If len == 0, it doesn't break out of the loop; though
> it should break out the next time it tries to read.

I wouldn't exactly trust a stream that has returned len=0 to be readable.

Now... you said the test suite passed, so apparently none of our
streams have a problem with that. But I wouldn't want to rely on that
:-P ... in my feedback below, I've got a suggestion for this.

> I'll need to read up on the stream_read() function.
> What happens if it reads past?  It should throw
> an error, no?  And thusly break.

A stream cannot "read past".

>...
> +++ 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 *err = SVN_NO_ERROR;
> +  svn_error_t *err2 = SVN_NO_ERROR;
>
>   /* 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,46 @@
>     {
>       apr_size_t len = SVN__STREAM_CHUNK_SIZE;
>
> -      if (cancel_func)
> -        SVN_ERR(cancel_func(cancel_baton));
> +      if (cancel_func)
> +        err = cancel_func(cancel_baton);
> +
> +      if (err)
> +         break;

You cancel_func == NULL, then you can avoid the second if, since there
is no way err could get set:

  if (cancel_func)
    {
      err = cancel_func(cancel_baton);
      if (err)
        break;
    }

> +      err = svn_stream_read(from, buf, &len);
> +      if (err)
> +        break;
> +
> +      if (len > 0)
> +        {
> +           err = svn_stream_write(to, buf, &len);
>
> -      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 (len != SVN__STREAM_CHUNK_SIZE)
> +               break;

I'd suggest moving this outside of the len>0 block.

> +           if (err)
> +               break;
> +        }
> +      else
> +        {
> +          break;
> +        }

Skip this else branch, and just do the len test here.

>     }
> +
> +    if (err)
> +     {
> +         err2 = svn_error_compose_create(svn_stream_close(from),
> +                                         svn_stream_close(to));
> +     }
> +    else
> +     {
> +         err2 = SVN_NO_ERROR;
> +         err  = svn_error_compose_create(svn_stream_close(from),
> +                                         svn_stream_close(to));
> +     }

You could skip the if/else blocks altogether, and just do:

  err2 = svn_error_compose_create(...);

> -  return svn_error_compose_create(svn_stream_close(from),
> -                                  svn_stream_close(to));
> +   return svn_error_compose_create(err,err2);

If err==NULL, then it will return whatever err2 is... in this case,
any potential problem with closing the streams.

Style note: there should be a space after that comma.

Cheers,
-g

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1627618
Received on 2009-04-10 11:35:12 CEST