On Wed, Apr 15, 2009 at 21:48, Stefan Sperling <stsp_at_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
Received on 2009-04-16 14:29:25 CEST