You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Greg Stein <gs...@gmail.com> on 2009/04/16 12:29:02 UTC
Re: svn commit: r37278 - trunk/subversion/libsvn_client
On Wed, Apr 15, 2009 at 21:48, Stefan Sperling <st...@elego.de> wrote:
>...
> +++ trunk/subversion/libsvn_client/patch.c Wed Apr 15 12:48:04 2009 (r37278)
>...
> @@ -1738,6 +1739,11 @@ extract_svnpatch(const char *original_pa
> return SVN_NO_ERROR;
> }
>
> +/* ### forward-declaration */
> +static svn_error_t *
> +apply_textdiffs(const char *patch_path, svn_client_ctx_t *ctx,
> + apr_pool_t *pool);
Will this go away at some point? And could you put this at the TOP of
the file, rather than hidden in the middle of the code?
>...
> +/* --- Text-diff application routines --- */
> +
> +typedef struct patch_target_t {
Does the structure actually need a name? Since it is typedef'd, that
is kinda redundant.
Could you also try to keep typedefs near the top of the file, rather
than intermixed with all the code?
> + /* The target file, read-only, seekable. */
> + apr_file_t *file;
> +
> + /* The result stream, write-only, not seekable.
> + * This is where we write the patched result to. */
> + svn_stream_t *result;
> +
> + /* Path to the temporary file underlying the result stream. */
> + const char *result_path;
> +
> + /* The line last read from the target file. */
> + svn_filesize_t current_line;
While I recognize that a file may have 2^63 bytes, files that large
are NOT going to be text files with more than an int's worth of line
numbers. "filesize" is a strange name for "line number".
I will also note that svn's diff algorithms keep a small structure of
data *per line*. If we ever run into files with more than 2 billion
lines, then we're going to be in HUGE trouble.
(and no, I wouldn't be very opposed to Yet Another Typedef :-P ... say
svn_lineno_t, maybe).
>>..
> + /* EOL-marker used by target file. */
> + const char *eol_str;
> +
> + /* EOL-marker used by patch file. */
> + const char *patch_eol_str;
This structure describes the "target", so why is source information in here?
>...
> +copy_lines_to_target(patch_target_t *target, svn_filesize_t line,
> + svn_boolean_t *eof, apr_pool_t *pool)
> +{
> + svn_stream_t *s;
> + apr_pool_t *iterpool;
> +
> + s = svn_stream_from_aprfile2(target->file, TRUE, pool);
> + *eof = FALSE;
> +
> + iterpool = svn_pool_create(pool);
> + while (target->current_line < line && ! *eof)
> + {
> + svn_stringbuf_t *buf;
> + apr_size_t len;
> +
> + svn_pool_clear(iterpool);
> +
> + SVN_ERR(svn_stream_readline(s, &buf, target->eol_str, eof, pool));
> + svn_stringbuf_appendcstr(buf, target->eol_str);
> + target->current_line++;
> +
> + len = buf->len;
> + SVN_ERR(svn_stream_write(target->result, buf->data, &len));
> + if (len < buf->len)
By definition, a short write cannot occur without an error.
>...
> + svn_stream_close(s);
> +
> + return SVN_NO_ERROR;
Woah! Can't ignore the error on that stream_close. I'd suggest returning it.
> +/* Read at most NLINES from the TARGET file, returning lines read in *LINES,
> + * separated by the EOL sequence specified in TARGET->patch_eol_str.
> + * Allocate *LINES in result_pool.
> + * Use SCRATCH_POOL for all other allocations. */
> +static svn_error_t *
> +read_lines_from_target(svn_string_t **lines, svn_filesize_t nlines,
> + patch_target_t *target, apr_pool_t *scratch_pool,
> + apr_pool_t *result_pool)
Please correct the ordering of result/scratch_pool.
And what is keeping NLINES from being 1,000,000 ? And reading an
ENTIRE file into memory? Unless there is a "just can't be done,
otherwise" reason (like the diff code), then we cannot read
filesize-proportionate contents into memory.
>...
> + svn_stream_close(s);
Gotta return, or clear, that error return.
> +
> + target->current_line += i;
> + *lines = svn_string_create_from_buf(buf, result_pool);
Why do you read the lines into iterpool, then copy them to
scratch_pool, then copy them *again* to result_pool?
>...
> +/* Apply a HUNK to a patch TARGET. Do all allocations in POOL. */
> +static svn_error_t *
> +apply_one_hunk(svn_hunk_t *hunk, patch_target_t *target, apr_pool_t *pool)
> +{
> + svn_filesize_t line;
> + svn_boolean_t eof;
> + svn_string_t *latest_text;
> + svn_diff_t *diff;
> + svn_diff_file_options_t *opts;
> +
> + /* Determine the line the hunk should be applied at. */
> + line = determine_hunk_line(hunk, target);
> +
> + if (target->current_line > line)
> + /* If we already passed the line that the hunk should be applied to,
> + * the hunks in the patch file are out of order. */
> + /* TODO: Warn, create reject file? */
> + return SVN_NO_ERROR;
I'll repeat a comment from a previous review: please use ### for to-do markers.
Also: if there is more than one line after an "if", then please wrap
it in braces (yes, even tho it is a comment...)
>...
> + SVN_ERR(svn_stream_close(target->result));
> + SVN_ERR(svn_io_file_close(target->file, pool));
Do you ever seek on target->file? If not, then let the stream own the
file (and don't even bother to remember the apr_file_t).
>...
Cheers,
-g
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1746464
Re: svn commit: r37278 - trunk/subversion/libsvn_client
Posted by Stefan Sperling <st...@elego.de>.
On Fri, Apr 17, 2009 at 10:39:41PM +0100, Stefan Sperling wrote:
> > > +/* ### forward-declaration */
> > > +static svn_error_t *
> > > +apply_textdiffs(const char *patch_path, svn_client_ctx_t *ctx,
> > > + apr_pool_t *pool);
> >
> > Will this go away at some point? And could you put this at the TOP of
> > the file, rather than hidden in the middle of the code?
>
> It can only go away if we move the svn_client_patch() function to
> the end of the file.
>
> Right now, it's nice to have the svn-protocol-specific stuff at
> the top, and the unidiff-specific stuff at the bottom.
>
> I expect the svn-protocol-specific stuff to be changed a lot at some
> point, when we clean it up then.
Should have said "we can clean it up then".
Stefan
Re: svn commit: r37278 - trunk/subversion/libsvn_client
Posted by Greg Stein <gs...@gmail.com>.
Shweet... I don't have any specific line-by-line responses, and will
wait for your next revision. I'm happy to have pointed out spots of
concern, and see where ya go...
[and yes, it looks like a number of items I asked about are "in flux",
so no worries...]
On Fri, Apr 17, 2009 at 23:39, Stefan Sperling <st...@elego.de> wrote:
> On Thu, Apr 16, 2009 at 02:29:02PM +0200, Greg Stein wrote:
>> On Wed, Apr 15, 2009 at 21:48, Stefan Sperling <st...@elego.de> wrote:
>> >...
>> > +++ trunk/subversion/libsvn_client/patch.c Wed Apr 15 12:48:04 2009 (r37278)
>> >...
>> > @@ -1738,6 +1739,11 @@ extract_svnpatch(const char *original_pa
>> > return SVN_NO_ERROR;
>> > }
>> >
>> > +/* ### forward-declaration */
>> > +static svn_error_t *
>> > +apply_textdiffs(const char *patch_path, svn_client_ctx_t *ctx,
>> > + apr_pool_t *pool);
>>
>> Will this go away at some point? And could you put this at the TOP of
>> the file, rather than hidden in the middle of the code?
>
> It can only go away if we move the svn_client_patch() function to
> the end of the file.
>
> Right now, it's nice to have the svn-protocol-specific stuff at
> the top, and the unidiff-specific stuff at the bottom.
>
> I expect the svn-protocol-specific stuff to be changed a lot at some
> point, when we clean it up then.
>
>> >...
>> > +/* --- Text-diff application routines --- */
>> > +
>> > +typedef struct patch_target_t {
>>
>> Does the structure actually need a name? Since it is typedef'd, that
>> is kinda redundant.
>
> I'll remove it.
>
>> Could you also try to keep typedefs near the top of the file, rather
>> than intermixed with all the code?
>
> Well, same thing again. I'd like that struct to be close to functions
> that use it. I consider the unidiff stuff separate from the rest of
> that file. I'd even go as far as moving it into a file of its own.
> It evolved from debugging code I had added to libsvn_client/patch.c,
> that's the only reason it's there right now.
>
>> While I recognize that a file may have 2^63 bytes, files that large
>> are NOT going to be text files with more than an int's worth of line
>> numbers. "filesize" is a strange name for "line number".
>
> Yes. I was assuming that diff used line offsets until I got to writing
> code that applys diffs. The unidiff-format spec that I read didn't
> say that all numbers where in lines.
>
> I will convert those to unsigned long.
>
>> I will also note that svn's diff algorithms keep a small structure of
>> data *per line*. If we ever run into files with more than 2 billion
>> lines, then we're going to be in HUGE trouble.
>
> heh
>
>> (and no, I wouldn't be very opposed to Yet Another Typedef :-P ... say
>> svn_lineno_t, maybe).
>
> Might make the code more readable, yes. I'll add it to svn_types.h.
>
>> >>..
>> > + /* EOL-marker used by target file. */
>> > + const char *eol_str;
>> > +
>> > + /* EOL-marker used by patch file. */
>> > + const char *patch_eol_str;
>>
>> This structure describes the "target", so why is source information in here?
>
> This might go away eventually. I'm not entirely sure it's going to be
> needed. I will look at this again when implementing eol-style
> normalisation for patching.
>
>> >...
>> > +copy_lines_to_target(patch_target_t *target, svn_filesize_t line,
>> > + svn_boolean_t *eof, apr_pool_t *pool)
>> > +{
>> > + svn_stream_t *s;
>> > + apr_pool_t *iterpool;
>> > +
>> > + s = svn_stream_from_aprfile2(target->file, TRUE, pool);
>> > + *eof = FALSE;
>> > +
>> > + iterpool = svn_pool_create(pool);
>> > + while (target->current_line < line && ! *eof)
>> > + {
>> > + svn_stringbuf_t *buf;
>> > + apr_size_t len;
>> > +
>> > + svn_pool_clear(iterpool);
>> > +
>> > + SVN_ERR(svn_stream_readline(s, &buf, target->eol_str, eof, pool));
>> > + svn_stringbuf_appendcstr(buf, target->eol_str);
>> > + target->current_line++;
>> > +
>> > + len = buf->len;
>> > + SVN_ERR(svn_stream_write(target->result, buf->data, &len));
>> > + if (len < buf->len)
>>
>> By definition, a short write cannot occur without an error.
>
> Great, so I can drop that check.
>
>> >...
>> > + svn_stream_close(s);
>> > +
>> > + return SVN_NO_ERROR;
>>
>> Woah! Can't ignore the error on that stream_close. I'd suggest returning it.
>
> Ooops :)
> Sorry I didn't do that on purpose. Good catch.
>
>> > +/* Read at most NLINES from the TARGET file, returning lines read in *LINES,
>> > + * separated by the EOL sequence specified in TARGET->patch_eol_str.
>> > + * Allocate *LINES in result_pool.
>> > + * Use SCRATCH_POOL for all other allocations. */
>> > +static svn_error_t *
>> > +read_lines_from_target(svn_string_t **lines, svn_filesize_t nlines,
>> > + patch_target_t *target, apr_pool_t *scratch_pool,
>> > + apr_pool_t *result_pool)
>>
>> Please correct the ordering of result/scratch_pool.
>
> Already done (r37303).
>
>> And what is keeping NLINES from being 1,000,000 ? And reading an
>> ENTIRE file into memory? Unless there is a "just can't be done,
>> otherwise" reason (like the diff code), then we cannot read
>> filesize-proportionate contents into memory.
>
> Ooops!
>
> You're right, a hunk might be very big, or even an entire file when it
> is being added (all lines in the diff start with +).
>
> Good thing we have diff APIs for strings as well as files.
> I'll try to put hunks into temporary files instead in-memory strings
> if the number of bytes of the hunk is above an upper bound. We'll just
> need to define a threshold. Does 64k sound reasonable or is that not
> enough for you? :)
>
>> >...
>> > + svn_stream_close(s);
>>
>> Gotta return, or clear, that error return.
>
> Yes, will do.
>
>>
>> > +
>> > + target->current_line += i;
>> > + *lines = svn_string_create_from_buf(buf, result_pool);
>>
>> Why do you read the lines into iterpool, then copy them to
>> scratch_pool, then copy them *again* to result_pool?
>
> Because I applied our pool usage pragmas without thinking.
>
>> >...
>> > +/* Apply a HUNK to a patch TARGET. Do all allocations in POOL. */
>> > +static svn_error_t *
>> > +apply_one_hunk(svn_hunk_t *hunk, patch_target_t *target, apr_pool_t *pool)
>> > +{
>> > + svn_filesize_t line;
>> > + svn_boolean_t eof;
>> > + svn_string_t *latest_text;
>> > + svn_diff_t *diff;
>> > + svn_diff_file_options_t *opts;
>> > +
>> > + /* Determine the line the hunk should be applied at. */
>> > + line = determine_hunk_line(hunk, target);
>> > +
>> > + if (target->current_line > line)
>> > + /* If we already passed the line that the hunk should be applied to,
>> > + * the hunks in the patch file are out of order. */
>> > + /* TODO: Warn, create reject file? */
>> > + return SVN_NO_ERROR;
>>
>> I'll repeat a comment from a previous review: please use ### for to-do markers.
>
> If that helps you, I can add them.
>
>> Also: if there is more than one line after an "if", then please wrap
>> it in braces (yes, even tho it is a comment...)
>
> If that makes you happy I can do it.
>
> I might add that the above two points are not mentioned in HACKING
> so I'll consider them your personal preference and not the top-most
> priority tasks here -- you found bigger much problems I'd like to
> address first.
>
>> >...
>> > + SVN_ERR(svn_stream_close(target->result));
>> > + SVN_ERR(svn_io_file_close(target->file, pool));
>>
>> Do you ever seek on target->file? If not, then let the stream own the
>> file (and don't even bother to remember the apr_file_t).
>
> We might seek in it later. I'm not sure yet.
>
> Thanks,
> Stefan
>
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1776422
Re: svn commit: r37278 - trunk/subversion/libsvn_client
Posted by Stefan Sperling <st...@elego.de>.
On Thu, Apr 16, 2009 at 02:29:02PM +0200, Greg Stein wrote:
> On Wed, Apr 15, 2009 at 21:48, Stefan Sperling <st...@elego.de> wrote:
> >...
> > +++ trunk/subversion/libsvn_client/patch.c Wed Apr 15 12:48:04 2009 (r37278)
> >...
> > @@ -1738,6 +1739,11 @@ extract_svnpatch(const char *original_pa
> > return SVN_NO_ERROR;
> > }
> >
> > +/* ### forward-declaration */
> > +static svn_error_t *
> > +apply_textdiffs(const char *patch_path, svn_client_ctx_t *ctx,
> > + apr_pool_t *pool);
>
> Will this go away at some point? And could you put this at the TOP of
> the file, rather than hidden in the middle of the code?
It can only go away if we move the svn_client_patch() function to
the end of the file.
Right now, it's nice to have the svn-protocol-specific stuff at
the top, and the unidiff-specific stuff at the bottom.
I expect the svn-protocol-specific stuff to be changed a lot at some
point, when we clean it up then.
> >...
> > +/* --- Text-diff application routines --- */
> > +
> > +typedef struct patch_target_t {
>
> Does the structure actually need a name? Since it is typedef'd, that
> is kinda redundant.
I'll remove it.
> Could you also try to keep typedefs near the top of the file, rather
> than intermixed with all the code?
Well, same thing again. I'd like that struct to be close to functions
that use it. I consider the unidiff stuff separate from the rest of
that file. I'd even go as far as moving it into a file of its own.
It evolved from debugging code I had added to libsvn_client/patch.c,
that's the only reason it's there right now.
> While I recognize that a file may have 2^63 bytes, files that large
> are NOT going to be text files with more than an int's worth of line
> numbers. "filesize" is a strange name for "line number".
Yes. I was assuming that diff used line offsets until I got to writing
code that applys diffs. The unidiff-format spec that I read didn't
say that all numbers where in lines.
I will convert those to unsigned long.
> I will also note that svn's diff algorithms keep a small structure of
> data *per line*. If we ever run into files with more than 2 billion
> lines, then we're going to be in HUGE trouble.
heh
> (and no, I wouldn't be very opposed to Yet Another Typedef :-P ... say
> svn_lineno_t, maybe).
Might make the code more readable, yes. I'll add it to svn_types.h.
> >>..
> > + /* EOL-marker used by target file. */
> > + const char *eol_str;
> > +
> > + /* EOL-marker used by patch file. */
> > + const char *patch_eol_str;
>
> This structure describes the "target", so why is source information in here?
This might go away eventually. I'm not entirely sure it's going to be
needed. I will look at this again when implementing eol-style
normalisation for patching.
> >...
> > +copy_lines_to_target(patch_target_t *target, svn_filesize_t line,
> > + svn_boolean_t *eof, apr_pool_t *pool)
> > +{
> > + svn_stream_t *s;
> > + apr_pool_t *iterpool;
> > +
> > + s = svn_stream_from_aprfile2(target->file, TRUE, pool);
> > + *eof = FALSE;
> > +
> > + iterpool = svn_pool_create(pool);
> > + while (target->current_line < line && ! *eof)
> > + {
> > + svn_stringbuf_t *buf;
> > + apr_size_t len;
> > +
> > + svn_pool_clear(iterpool);
> > +
> > + SVN_ERR(svn_stream_readline(s, &buf, target->eol_str, eof, pool));
> > + svn_stringbuf_appendcstr(buf, target->eol_str);
> > + target->current_line++;
> > +
> > + len = buf->len;
> > + SVN_ERR(svn_stream_write(target->result, buf->data, &len));
> > + if (len < buf->len)
>
> By definition, a short write cannot occur without an error.
Great, so I can drop that check.
> >...
> > + svn_stream_close(s);
> > +
> > + return SVN_NO_ERROR;
>
> Woah! Can't ignore the error on that stream_close. I'd suggest returning it.
Ooops :)
Sorry I didn't do that on purpose. Good catch.
> > +/* Read at most NLINES from the TARGET file, returning lines read in *LINES,
> > + * separated by the EOL sequence specified in TARGET->patch_eol_str.
> > + * Allocate *LINES in result_pool.
> > + * Use SCRATCH_POOL for all other allocations. */
> > +static svn_error_t *
> > +read_lines_from_target(svn_string_t **lines, svn_filesize_t nlines,
> > + patch_target_t *target, apr_pool_t *scratch_pool,
> > + apr_pool_t *result_pool)
>
> Please correct the ordering of result/scratch_pool.
Already done (r37303).
> And what is keeping NLINES from being 1,000,000 ? And reading an
> ENTIRE file into memory? Unless there is a "just can't be done,
> otherwise" reason (like the diff code), then we cannot read
> filesize-proportionate contents into memory.
Ooops!
You're right, a hunk might be very big, or even an entire file when it
is being added (all lines in the diff start with +).
Good thing we have diff APIs for strings as well as files.
I'll try to put hunks into temporary files instead in-memory strings
if the number of bytes of the hunk is above an upper bound. We'll just
need to define a threshold. Does 64k sound reasonable or is that not
enough for you? :)
> >...
> > + svn_stream_close(s);
>
> Gotta return, or clear, that error return.
Yes, will do.
>
> > +
> > + target->current_line += i;
> > + *lines = svn_string_create_from_buf(buf, result_pool);
>
> Why do you read the lines into iterpool, then copy them to
> scratch_pool, then copy them *again* to result_pool?
Because I applied our pool usage pragmas without thinking.
> >...
> > +/* Apply a HUNK to a patch TARGET. Do all allocations in POOL. */
> > +static svn_error_t *
> > +apply_one_hunk(svn_hunk_t *hunk, patch_target_t *target, apr_pool_t *pool)
> > +{
> > + svn_filesize_t line;
> > + svn_boolean_t eof;
> > + svn_string_t *latest_text;
> > + svn_diff_t *diff;
> > + svn_diff_file_options_t *opts;
> > +
> > + /* Determine the line the hunk should be applied at. */
> > + line = determine_hunk_line(hunk, target);
> > +
> > + if (target->current_line > line)
> > + /* If we already passed the line that the hunk should be applied to,
> > + * the hunks in the patch file are out of order. */
> > + /* TODO: Warn, create reject file? */
> > + return SVN_NO_ERROR;
>
> I'll repeat a comment from a previous review: please use ### for to-do markers.
If that helps you, I can add them.
> Also: if there is more than one line after an "if", then please wrap
> it in braces (yes, even tho it is a comment...)
If that makes you happy I can do it.
I might add that the above two points are not mentioned in HACKING
so I'll consider them your personal preference and not the top-most
priority tasks here -- you found bigger much problems I'd like to
address first.
> >...
> > + SVN_ERR(svn_stream_close(target->result));
> > + SVN_ERR(svn_io_file_close(target->file, pool));
>
> Do you ever seek on target->file? If not, then let the stream own the
> file (and don't even bother to remember the apr_file_t).
We might seek in it later. I'm not sure yet.
Thanks,
Stefan