On Wed, Sep 19, 2012 at 02:35:07AM +0100, Julian Foad wrote:
> > Modified: subversion/trunk/subversion/include/svn_io.h
> > ==============================================================================
> > --- subversion/trunk/subversion/include/svn_io.h (original)
> > +++ subversion/trunk/subversion/include/svn_io.h Fri Jul 6 10:45:21 2012
> >
> > +/* Read a line of text from a file, up to a specified length.
>
> This is much like a variant of svn_io_read_length_line(). Can we move its declaration and definition adjacent to that function, and cross-reference their doc strings?
>
> Can they share implementation? They look similar. svn_io_read_length_line() was optimized in r1381800, but this function was not.
>
> This is also a bit like svn_stream_readline(). It has in common with svn_stream_readline() that the result is returned in a stringbuf. However, svn_io_read_length_line() writes into a plain memory area.
>
> All three of these functions differ in how they handle EOLs.
Oh, I didn't notice svn_io_read_length_line() before!
Thanks for pointing it out.
The EOL handling in svn_io_read_length_line() is different from what
current users of svn_io_file_readline() need. It only checks for '\n',
which won't work for e.g. svn patch since it needs to deal with mixed
newlines in the same file (such as svn diff generates on Windows...).
Maybe svn_io_read_length_line() should use svn_eol__detect_eol()
internally. This would be ABI compatible, it's just that mixed
newlines in input would be split different if the file being
read was opened in binary mode.
> The different combinations of output style and EOL handling tend to make the overall API get confusing. Can we do something to reduce that amount of difference?
Well, the released public APIs cannot be changed, and they're not
a good replacement for svn_io_file_readline() either. The callers
need to know what kind of newline was read because they need to
retain the newline character in the output they're computing.
> One simple short-term solution to all the above would be to just make this new API private for the time being.
svn_io_file_readline() is used from the command line client, in the
new file merge tool. I think this function could be useful to other
clients trying to implement similar functionality.
I'd prefer to group these functions together in the header file,
and just document their differences.
> > + * If end-of-file is reached or @a max_len bytes have been read, and @a eof
> > + * is not NULL, then set @a *eof to @c TRUE.
>
> Confusing that *eof can be set when it's not end-of-file. Can we set *eof only when it's end-of-file?
The idea here is to provide the caller with the illusion that a
file is shorter than it actually is. This is used by 'svn patch'
to split a patch file into logical segments (one for each hunk
in the patch), which can then be read as if they were individual
files. So to the patch code *eof really means "no more lines left
in this hunk". Maybe this abstraction could be handled by the caller,
but that would require some adaptation and thorough testing.
The current patch implementation is very much geared towards this
abstraction and relies on it.
Received on 2012-09-19 09:42:54 CEST