Hi Greg,
>
> 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 quite agree that even though the tests passed, it means
that the code hasn't gotten to a stage or situation that
would croak badly.
>
>> 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".
Heh. I meant if it is read past, but I guess that's
meaningless.
>
>> - 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:
In hindsight, I can simply say this is one of those
"what the smeg was I thinking" moments. It's so
obvious that I wasn't really thinking clearly. Sorry.
>
> if (cancel_func)
> {
> err = cancel_func(cancel_baton);
> if (err)
> break;
> }
Is this allowed?
if (err = cancel_func(cancel_baton))
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.
I wasn't sure if at len== 0, it would break out of
the loop. Now I think of it, if len == 0, wouldn't
the stream_read() enter an error state?
>
>> - 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.
Yes. Stefan told me that. I still haven't gotten that
into my thick skull of mine.
Thanks! I'll get on to the patch asap.
Edmund
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1632076
Received on 2009-04-10 17:34:50 CEST