Re: [PATCH] #3460 -v2

Daniel Näslund wrote:
> Removed some debug statements.
>
> On Wed, Jan 27, 2010 at 08:59:02PM +0100, Daniel Näslund wrote:
> [[[
> Fix #3460 - svn patch is not fuzzy when applying unidiffs.
>
[...]

Hi Daniel. Just one quick kind of comment from me: can you make sure all
the functions have doc strings, at least documenting any new parameters
that you are adding. Even when it seems "obvious".

> @@ -596,10 +599,11 @@
> * Do temporary allocations in POOL. */
> static svn_error_t *
> match_hunk(svn_boolean_t *matched, patch_target_t *target,
> - const svn_hunk_t *hunk, apr_pool_t *pool)
> + const svn_hunk_t *hunk, int fuzz, apr_pool_t *pool)

> @@ -670,7 +686,7 @@
> static svn_error_t *
> scan_for_match(svn_linenum_t *matched_line, patch_target_t *target,
> const svn_hunk_t *hunk, svn_boolean_t match_first,
> - svn_linenum_t upper_line, apr_pool_t *pool)
> + svn_linenum_t upper_line, int fuzz, apr_pool_t *pool)

> @@ -727,7 +743,7 @@
> * Do temporary allocations in POOL. */
> static svn_error_t *
> get_hunk_info(hunk_info_t **hi, patch_target_t *target,
> - const svn_hunk_t *hunk, apr_pool_t *result_pool,
> + const svn_hunk_t *hunk, int fuzz, apr_pool_t *result_pool,
> apr_pool_t *scratch_pool)

> @@ -833,13 +851,17 @@
> /* Copy HUNK_TEXT into TARGET, line by line, such that the line filter
> * and transformation callbacks set on HUNK_TEXT by the diff parsing
> * code in libsvn_diff will trigger. ABSPATH is the absolute path to the
> - * file underlying TARGET. */
> + * file underlying TARGET. Do not copy the lines that is within FUZZ steps
> + * from the beginning or end of hunk unless NR_OF_LINES is set to 0. Then we
> + * copy each line of HUNK_TEXT. */
> static svn_error_t *
> copy_hunk_text(svn_stream_t *hunk_text, svn_stream_t *target,
> - const char *abspath, apr_pool_t *scratch_pool)
> + const char *abspath, int fuzz, int nr_of_lines,
> + apr_pool_t *scratch_pool)

Here you mention both of the new parameters but you need to document
what NR_OF_LINES does if it is not zero.

> @@ -895,15 +931,19 @@
> return SVN_NO_ERROR;
> }
>
> -/* Apply a hunk described by hunk info HI to a patch TARGET.
> +/* Apply a hunk described by hunk info HI to a patch TARGET. If we have FUZZ
> + * use the lines from the target for those lines instead of the hunk lines.
> * Do all allocations in POOL. */
> static svn_error_t *
> -apply_one_hunk(patch_target_t *target, hunk_info_t *hi, apr_pool_t *pool)
> +apply_one_hunk(patch_target_t *target, hunk_info_t *hi,
> + apr_pool_t *pool)

Here it looks like there should be a parameter called FUZZ but there
isn't.

I'm going to trust that if the doc strings are good then the whole patch
will be good :-)

Thanks.
- Julian

-- 
Regards,
Julian Foad
Senior Subversion Committer
WANdisco, Inc.
Office  : +44 (0)114 3039985 (Ext 711)
http://www.wandisco.com
http://blog.wandisco.com/david/
Free Webinar: Make Subversion Agile:
http://www.wandisco.com/webinar/svn_agile
Replay Webinar / Get Slides: Subversion in 2010 and Beyond:
http://www.slideshare.net/wandisco/subversion-in-2010-and-beyond
Follow us on Twitter
http://twitter.com/wandisco
Open Source Subversion Community:
http://subversion.wandisco.com
This email and any attachments may contain private, confidential and
privileged material for the sole use of the intended recipient. If you
are not the intended recipient, please immediately delete this email
and any attachments.