Greg Stein wrote on Thu, Sep 22, 2011 at 06:12:58 -0400: > On Wed, Sep 21, 2011 at 22:53, Daniel Shahaf <[email protected]> wrote: > > [[[ > > Remove an edge case from svn_spillbuf__is_empty(). > > > > * subversion/include/private/svn_subr_private.h > > (svn_spillbuf_is_empty): Stop documenting this bug in the doc string. > > > > * subversion/libsvn_subr/spillbuf.c > > (read_data): Also check for EOF in the non-error code path. > > I don't think that apr_file_eof() can predictively determine EOF > unless/until you try and read off the end of the file. It is quite > conceivable to arrange a situation where we read precisely the number > of bytes in the file (ie. the remaining amount matches blocksize). > Looking apr_file_eof(), it would return that data without setting > thefile->eof_hit. >
I see what you mean. Two things here: - Adding the check does not make the situation any worse than it already is; it reduces the chance for a false negative, even though, as you say, it does not eliminate it. - I'm going to claim it's an APR bug :-). Your new API is specifically documented as not detecting EOF if the reads aligned with the file's size, but apr_file_eof() doesn't have that limitation documented. Therefore, either it should DTRT or it should grow that limitation too in its docstring. > >... > > +++ subversion/tests/libsvn_subr/spillbuf-test.c (working copy) > > @@ -224,18 +224,22 @@ test_spillbuf_interleaving(apr_pool_t *pool) > > SVN_ERR(svn_spillbuf__write(buf, "GHIJKL", 6, pool)); > > /* now: two blocks of 8 and 6 bytes, and 6 bytes spilled to a file */ > > By writing "GHIJKLMN" here, that would drop 8 bytes into the spill file... > > > > > + SVN_TEST_ASSERT(! svn_spillbuf__is_empty(buf)); > > SVN_ERR(svn_spillbuf__read(&readptr, &readlen, buf, pool)); > > SVN_TEST_ASSERT(readptr != NULL > > && readlen == 8 > > && memcmp(readptr, "qrstuvwx", 8) == 0); > > + SVN_TEST_ASSERT(! svn_spillbuf__is_empty(buf)); > > SVN_ERR(svn_spillbuf__read(&readptr, &readlen, buf, pool)); > > SVN_TEST_ASSERT(readptr != NULL > > && readlen == 6 > > && memcmp(readptr, "ABCDEF", 6) == 0); > > + SVN_TEST_ASSERT(! svn_spillbuf__is_empty(buf)); > > SVN_ERR(svn_spillbuf__read(&readptr, &readlen, buf, pool)); > > SVN_TEST_ASSERT(readptr != NULL > > && readlen == 6 > > && memcmp(readptr, "GHIJKL", 6) == 0); > > + SVN_TEST_ASSERT(svn_spillbuf__is_empty(buf)); > > ... and I would guess this read would return 8 bytes and not be marked as > empty. > For whatever reason the test still passes for me when I make that change. I agree with your analysis, though. > Cheers, > -g
