On Sun, Sep 06, 2009 at 09:48:03PM +0200, Daniel Näslund wrote:
> @@ -291,6 +291,21 @@
> return SVN_NO_ERROR;
> }
>
> +/** line-transformer callback to shave leading diff symbols. */
> +static svn_error_t *
> +remove_leading_char_filter(svn_stringbuf_t **buf,
I'd call this something other than a 'filter', but whatever :)
> @@ -358,8 +374,21 @@
> }
>
> c = line->data[0];
> - if (c == ' ' || c == '-' || c == '+')
> - hunk_seen = TRUE;
> + if (c == ' ' || c == '-')
> + {
> + hunk_seen = TRUE;
> + original_lines--;
What if original_lines wraps around?
I think you need an 'if (original_lines > 0)' here.
> + }
> + else if (c == '+')
> + {
> + hunk_seen = TRUE;
> + }
> + /* Should tolerate chopped leading spaces on empty lines. */
s/Should t/T/
> + else if (original_lines > 0 && ! eof && line->len == 0)
> + {
> + hunk_seen = TRUE;
> + original_lines--;
Here, you've guarded against wrap-around, as a side-effect of your
line checking logic. But you should also do so above.
As an aside, something else to think about: I believe that
hunk->original_length is currently taken verbatim from user input.
To avoid people playing games with us with very large or small numbers,
we may want to either set hunk->original_length to the number of original
lines we've parsed, or abort parsing entirely, if there's a mismatch.
> @@ -418,8 +451,17 @@
> modified_text = svn_stream_from_aprfile_range_readonly(f, FALSE,
> start, end,
> result_pool);
> +
> svn_stream_set_line_filter_callback(modified_text, modified_line_filter);
> +
> + svn_stream_set_line_transformer_callback(diff_text,
> + remove_leading_char_filter);
Is the diff text supposed to have chars stripped from it?
I don't think so. I don't even remember what it's used for...
Maybe we can just remove the diff_text (in another commit)?
>
> + svn_stream_set_line_transformer_callback(original_text,
> + remove_leading_char_filter);
> +
> + svn_stream_set_line_transformer_callback(modified_text,
> + remove_leading_char_filter);
> --- subversion/tests/libsvn_diff/parse-diff-test.c (revision 39156)
> +++ subversion/tests/libsvn_diff/parse-diff-test.c (arbetskopia)
> @@ -46,7 +46,7 @@
> "===================================================================" NL
> "--- A/D/gamma.orig" NL
> "+++ A/D/gamma" NL
> - "@@ -1 +1,2 @@" NL
> + "@@ -1,2 +1 @@" NL
> " This is the file 'gamma'." NL
> "-some less bytes to 'gamma'" NL
> "" NL
why this change?
> + svntest.actions.run_and_verify_patch(wc_dir, os.path.abspath(patch_file_path),
> + expected_output,
> + expected_disk,
> + expected_status,
> + expected_skip,
> + None, # expected err
> + 1, # check-props
> + 0) # dry-run
Also unrelated to your patch, but exposed by it: We need to clean up
run_and_verify_patch(), e.g. the check-props param is probably unused now.
> @@ -648,13 +648,9 @@
> {
> if (line->len >= 1)
> {
> - char c = line->data[0];
> -
> - SVN_ERR_ASSERT(c == ' ' || c == '+' || c == '-');
> - len = line->len - 1;
> - SVN_ERR(svn_io_file_write_full(file, line->data + 1, len, &len,
> + len = line->len;
Maybe get rid of the 'len = line->len;' and ..
> + SVN_ERR(svn_io_file_write_full(file, line->data, len, &len,
.. pass (..., line->len, &len, ...) here?
> iterpool));
> - SVN_ERR_ASSERT(len == line->len - 1);
> }
Looks very good overall, thanks!
Stefan
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2391691
Received on 2009-09-06 22:42:29 CEST