I'm a little bit confused... After this commit, it seems that some tests
are failing:
[[[
W: Unexpected output
W: EXPECTED STDERR (regexp, match_all=False):
W: | svn:.*is not a local path
W: ACTUAL STDERR:
W: | svn: E200009:
'D:\svn\svn-trunk5\out\build\x64-Debug\Testing\subversion\tests\cmdline\svn-test-work\working_copies\input_validation_tests-20\foo'
does not exist
W: CWD:
D:\svn\svn-trunk5\out\build\x64-Debug\Testing\subversion\tests\cmdline
W: EXCEPTION: SVNUnmatchedError
Traceback (most recent call last):
File "D:\svn\svn-trunk5\subversion\tests\cmdline\svntest\main.py", line
1986, in run
rc = self.pred.run(sandbox)
^^^^^^^^^^^^^^^^^^^^^^
File "D:\svn\svn-trunk5\subversion\tests\cmdline\svntest\testcase.py",
line 178, in run
result = self.func(sandbox)
^^^^^^^^^^^^^^^^^^
File
"D:\svn\svn-trunk5\subversion\tests\cmdline\input_validation_tests.py",
line 226, in invalid_patch_targets
run_and_verify_svn_in_wc(sbox, "svn:.*is not a local path", 'patch',
File
"D:\svn\svn-trunk5\subversion\tests\cmdline\input_validation_tests.py",
line 55, in run_and_verify_svn_in_wc
svntest.actions.run_and_verify_svn([], expected_stderr,
File "D:\svn\svn-trunk5\subversion\tests\cmdline\svntest\actions.py",
line 339, in run_and_verify_svn
return run_and_verify_svn2(expected_stdout, expected_stderr,
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "D:\svn\svn-trunk5\subversion\tests\cmdline\svntest\actions.py",
line 379, in run_and_verify_svn2
verify.verify_outputs("Unexpected output", out, err,
File "D:\svn\svn-trunk5\subversion\tests\cmdline\svntest\verify.py", line
532, in verify_outputs
compare_and_display_lines(message, label, expected, actual, raisable)
File "D:\svn\svn-trunk5\subversion\tests\cmdline\svntest\verify.py", line
505, in compare_and_display_lines
raise raisable
svntest.main.SVNUnmatchedError
FAIL: input_validation_tests.py 20: non-working copy paths for 'patch'
]]]
The reason for this is I've accidently changed the sequence in which
parameters are verifying. Now the `svn patch` command verifies that the
patch file is openable before validating a working copy. However, I think
that the new behaviour is more correct because it seems like the patch file
is something we should look into first. Like this is a more important part
of the command. Even in the cmdline it goes first.
So is it okay to do this little change in behaviour and adjust the tests a
little bit? Or this doesn't follow backward compatibility guidelines?
On Thu, May 8, 2025 at 4:19 PM <rin...@apache.org> wrote:
> Author: rinrab
> Date: Thu May 8 14:19:32 2025
> New Revision: 1925463
>
> URL: http://svn.apache.org/viewvc?rev=1925463&view=rev
> Log:
> Add svn_client_patch2 function that accesses the patch file from an
> APR file instead of using a local path.
>
> * subversion/include/svn_client.h
> (svn_client_patch2): New function.
> (svn_client_patch): Deprecate.
> * subversion/libsvn_client/deprecated.c
> (svn_client_patch): Add and implement through svn_client_patch2().
> * subversion/libsvn_client/patch.c
> (apply_patches): Accept a file instead of a path and create a
> svn_diff_patch_parser_t instead of opening a svn_patch_file_t.
> (svn_client_patch): Update implementation to svn_client_patch2.
> * subversion/svn/patch-cmd.c
> (svn_cl__patch): Check and open the patch file manually, pass to
> the new function, and close it then.
> * subversion/tests/libsvn_client/client-test.c
> (test_patch): Update patch api used.
>
> See also a related discussion with myself on dev@ where this was
> initiated:
> https://lists.apache.org/thread/p260hdrt5wx0tv6xs9l5nt3pvbzvnrv4
>
> Modified:
> subversion/trunk/subversion/include/svn_client.h
> subversion/trunk/subversion/libsvn_client/deprecated.c
> subversion/trunk/subversion/libsvn_client/patch.c
> subversion/trunk/subversion/svn/patch-cmd.c
> subversion/trunk/subversion/tests/libsvn_client/client-test.c
>
> Modified: subversion/trunk/subversion/include/svn_client.h
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/include/svn_client.h?rev=1925463&r1=1925462&r2=1925463&view=diff
>
> ==============================================================================
> --- subversion/trunk/subversion/include/svn_client.h (original)
> +++ subversion/trunk/subversion/include/svn_client.h Thu May 8 14:19:32
> 2025
> @@ -7718,8 +7718,9 @@ typedef svn_error_t *(*svn_client_patch_
> apr_pool_t *scratch_pool);
>
> /**
> - * Apply a unidiff patch that's located at absolute path
> - * @a patch_abspath to the working copy directory at @a wc_dir_abspath.
> + * Apply a unidiff patch, described in @a apr_file which should have
> + * @c APR_READ and @c APR_BUFFERED capabilities to the working copy
> + * directory at @a wc_dir_abspath.
> *
> * This function makes a best-effort attempt at applying the patch.
> * It might skip patch targets which cannot be patched (e.g. targets
> @@ -7756,7 +7757,26 @@ typedef svn_error_t *(*svn_client_patch_
> *
> * Use @a scratch_pool for temporary allocations.
> *
> - * @since New in 1.7.
> + * @since New in 1.15.
> + */
> +svn_error_t *
> +svn_client_patch2(apr_file_t *apr_file,
> + const char *wc_dir_abspath,
> + svn_boolean_t dry_run,
> + int strip_count,
> + svn_boolean_t reverse,
> + svn_boolean_t ignore_whitespace,
> + svn_boolean_t remove_tempfiles,
> + svn_client_patch_func_t patch_func,
> + void *patch_baton,
> + svn_client_ctx_t *ctx,
> + apr_pool_t *scratch_pool);
> +
> +/**
> + * Similar to svn_client_patch2(), but accessing the patch by an absolute
> + * path.
> + *
> + * @deprecated Provided for backward compatibility with the 1.7 API.
> */
> svn_error_t *
> svn_client_patch(const char *patch_abspath,
>
> Modified: subversion/trunk/subversion/libsvn_client/deprecated.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_client/deprecated.c?rev=1925463&r1=1925462&r2=1925463&view=diff
>
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_client/deprecated.c (original)
> +++ subversion/trunk/subversion/libsvn_client/deprecated.c Thu May 8
> 14:19:32 2025
> @@ -3290,3 +3290,46 @@ svn_client_upgrade(const char *path,
> return svn_error_trace(svn_client_upgrade2(NULL, path, NULL, ctx,
> NULL, scratch_pool));
> }
> +
> +svn_error_t *
> +svn_client_patch(const char *patch_abspath,
> + const char *wc_dir_abspath,
> + svn_boolean_t dry_run,
> + int strip_count,
> + svn_boolean_t reverse,
> + svn_boolean_t ignore_whitespace,
> + svn_boolean_t remove_tempfiles,
> + svn_client_patch_func_t patch_func,
> + void *patch_baton,
> + svn_client_ctx_t *ctx,
> + apr_pool_t *scratch_pool)
> +{
> + svn_node_kind_t kind;
> + apr_file_t *apr_file;
> +
> + SVN_ERR(svn_io_check_path(patch_abspath, &kind, scratch_pool));
> + if (kind == svn_node_none)
> + return svn_error_createf(SVN_ERR_ILLEGAL_TARGET, NULL,
> + _("'%s' does not exist"),
> + svn_dirent_local_style(patch_abspath,
> + scratch_pool));
> + if (kind != svn_node_file)
> + return svn_error_createf(SVN_ERR_ILLEGAL_TARGET, NULL,
> + _("'%s' is not a file"),
> + svn_dirent_local_style(patch_abspath,
> + scratch_pool));
> +
> + SVN_ERR(svn_io_file_open(&apr_file, patch_abspath,
> + APR_READ | APR_BUFFERED, APR_OS_DEFAULT,
> + scratch_pool));
> +
> + SVN_ERR(svn_client_patch2(apr_file, wc_dir_abspath,
> + dry_run, strip_count, reverse,
> + ignore_whitespace, remove_tempfiles,
> + patch_func, patch_baton,
> + ctx, scratch_pool));
> +
> + SVN_ERR(svn_io_file_close(apr_file, scratch_pool));
> +
> + return SVN_NO_ERROR;
> +}
>
> Modified: subversion/trunk/subversion/libsvn_client/patch.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_client/patch.c?rev=1925463&r1=1925462&r2=1925463&view=diff
>
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_client/patch.c (original)
> +++ subversion/trunk/subversion/libsvn_client/patch.c Thu May 8 14:19:32
> 2025
> @@ -3613,8 +3613,8 @@ check_ancestor_delete(const char *delete
>
> /* This function is the main entry point into the patch code. */
> static svn_error_t *
> -apply_patches(/* The path to the patch file. */
> - const char *patch_abspath,
> +apply_patches(/* The patch file. */
> + apr_file_t *apr_file,
> /* The abspath to the working copy the patch should be
> applied to. */
> const char *root_abspath,
> /* Indicates whether we're doing a dry run. */
> @@ -3636,11 +3636,10 @@ apply_patches(/* The path to the patch f
> {
> svn_patch_t *patch;
> apr_pool_t *iterpool;
> - svn_patch_file_t *patch_file;
> + svn_diff_patch_parser_t *patch_parser;
> apr_array_header_t *targets_info;
>
> - /* Try to open the patch file. */
> - SVN_ERR(svn_diff_open_patch_file(&patch_file, patch_abspath,
> scratch_pool));
> + patch_parser = svn_diff_patch_parser_create(apr_file, scratch_pool);
>
> /* Apply patches. */
> targets_info = apr_array_make(scratch_pool, 0,
> @@ -3653,9 +3652,9 @@ apply_patches(/* The path to the patch f
> if (ctx->cancel_func)
> SVN_ERR(ctx->cancel_func(ctx->cancel_baton));
>
> - SVN_ERR(svn_diff_parse_next_patch(&patch, patch_file,
> - reverse, ignore_whitespace,
> - iterpool, iterpool));
> + SVN_ERR(svn_diff_patch_parser_next(&patch, patch_parser,
> + reverse, ignore_whitespace,
> + iterpool, iterpool));
> if (patch)
> {
> patch_target_t *target;
> @@ -3720,24 +3719,23 @@ apply_patches(/* The path to the patch f
> }
> while (patch);
>
> - SVN_ERR(svn_diff_close_patch_file(patch_file, iterpool));
> svn_pool_destroy(iterpool);
>
> return SVN_NO_ERROR;
> }
>
> svn_error_t *
> -svn_client_patch(const char *patch_abspath,
> - const char *wc_dir_abspath,
> - svn_boolean_t dry_run,
> - int strip_count,
> - svn_boolean_t reverse,
> - svn_boolean_t ignore_whitespace,
> - svn_boolean_t remove_tempfiles,
> - svn_client_patch_func_t patch_func,
> - void *patch_baton,
> - svn_client_ctx_t *ctx,
> - apr_pool_t *scratch_pool)
> +svn_client_patch2(apr_file_t *apr_file,
> + const char *wc_dir_abspath,
> + svn_boolean_t dry_run,
> + int strip_count,
> + svn_boolean_t reverse,
> + svn_boolean_t ignore_whitespace,
> + svn_boolean_t remove_tempfiles,
> + svn_client_patch_func_t patch_func,
> + void *patch_baton,
> + svn_client_ctx_t *ctx,
> + apr_pool_t *scratch_pool)
> {
> svn_node_kind_t kind;
>
> @@ -3751,18 +3749,6 @@ svn_client_patch(const char *patch_abspa
> svn_dirent_local_style(wc_dir_abspath,
> scratch_pool));
>
> - SVN_ERR(svn_io_check_path(patch_abspath, &kind, scratch_pool));
> - if (kind == svn_node_none)
> - return svn_error_createf(SVN_ERR_ILLEGAL_TARGET, NULL,
> - _("'%s' does not exist"),
> - svn_dirent_local_style(patch_abspath,
> - scratch_pool));
> - if (kind != svn_node_file)
> - return svn_error_createf(SVN_ERR_ILLEGAL_TARGET, NULL,
> - _("'%s' is not a file"),
> - svn_dirent_local_style(patch_abspath,
> - scratch_pool));
> -
> SVN_ERR(svn_io_check_path(wc_dir_abspath, &kind, scratch_pool));
> if (kind == svn_node_none)
> return svn_error_createf(SVN_ERR_ILLEGAL_TARGET, NULL,
> @@ -3776,7 +3762,7 @@ svn_client_patch(const char *patch_abspa
> scratch_pool));
>
> SVN_WC__CALL_WITH_WRITE_LOCK(
> - apply_patches(patch_abspath, wc_dir_abspath, dry_run, strip_count,
> + apply_patches(apr_file, wc_dir_abspath, dry_run, strip_count,
> reverse, ignore_whitespace, remove_tempfiles,
> patch_func, patch_baton, ctx, scratch_pool),
> ctx->wc_ctx, wc_dir_abspath, FALSE /* lock_anchor */, scratch_pool);
>
> Modified: subversion/trunk/subversion/svn/patch-cmd.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/svn/patch-cmd.c?rev=1925463&r1=1925462&r2=1925463&view=diff
>
> ==============================================================================
> --- subversion/trunk/subversion/svn/patch-cmd.c (original)
> +++ subversion/trunk/subversion/svn/patch-cmd.c Thu May 8 14:19:32 2025
> @@ -51,8 +51,10 @@ svn_cl__patch(apr_getopt_t *os,
> apr_array_header_t *targets;
> const char *abs_patch_path;
> const char *patch_path;
> + apr_file_t *patch_file;
> const char *abs_target_path;
> const char *target_path;
> + svn_node_kind_t kind;
>
> opt_state = ((svn_cl__cmd_baton_t *)baton)->opt_state;
> ctx = ((svn_cl__cmd_baton_t *)baton)->ctx;
> @@ -74,6 +76,19 @@ svn_cl__patch(apr_getopt_t *os,
>
> SVN_ERR(svn_dirent_get_absolute(&abs_patch_path, patch_path, pool));
>
> + SVN_ERR(svn_io_check_path(abs_patch_path, &kind, pool));
> + if (kind == svn_node_none)
> + return svn_error_createf(SVN_ERR_ILLEGAL_TARGET, NULL,
> + _("'%s' does not exist"),
> + svn_dirent_local_style(abs_patch_path,
> pool));
> + if (kind != svn_node_file)
> + return svn_error_createf(SVN_ERR_ILLEGAL_TARGET, NULL,
> + _("'%s' is not a file"),
> + svn_dirent_local_style(abs_patch_path,
> pool));
> +
> + SVN_ERR(svn_io_file_open(&patch_file, abs_patch_path,
> + APR_READ | APR_BUFFERED, APR_OS_DEFAULT,
> pool));
> +
> if (targets->nelts == 1)
> target_path = ""; /* "" is the canonical form of "." */
> else
> @@ -84,12 +99,13 @@ svn_cl__patch(apr_getopt_t *os,
> }
> SVN_ERR(svn_dirent_get_absolute(&abs_target_path, target_path, pool));
>
> - SVN_ERR(svn_client_patch(abs_patch_path, abs_target_path,
> - opt_state->dry_run, opt_state->strip,
> - opt_state->reverse_diff,
> - opt_state->ignore_whitespace,
> - TRUE, NULL, NULL, ctx, pool));
> + SVN_ERR(svn_client_patch2(patch_file, abs_target_path,
> + opt_state->dry_run, opt_state->strip,
> + opt_state->reverse_diff,
> + opt_state->ignore_whitespace,
> + TRUE, NULL, NULL, ctx, pool));
>
> + SVN_ERR(svn_io_file_close(patch_file, pool));
>
> if (! opt_state->quiet)
> SVN_ERR(svn_cl__notifier_print_conflict_stats(ctx->notify_baton2,
> pool));
>
> Modified: subversion/trunk/subversion/tests/libsvn_client/client-test.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/libsvn_client/client-test.c?rev=1925463&r1=1925462&r2=1925463&view=diff
>
> ==============================================================================
> --- subversion/trunk/subversion/tests/libsvn_client/client-test.c
> (original)
> +++ subversion/trunk/subversion/tests/libsvn_client/client-test.c Thu May
> 8 14:19:32 2025
> @@ -348,6 +348,8 @@ test_patch(const svn_test_opts_t *opts,
> const char *reject_tempfile_path;
> const char *key;
> int i;
> + apr_off_t off;
> +
> #define NL APR_EOL_STR
> #define UNIDIFF_LINES 7
> const char *unidiff_patch[UNIDIFF_LINES] = {
> @@ -401,8 +403,8 @@ test_patch(const svn_test_opts_t *opts,
> pool, svn_test_data_path("test-patch", pool),
> "test-patch.diff", SVN_VA_NULL);
> SVN_ERR(svn_io_file_open(&patch_file, patch_file_path,
> - (APR_READ | APR_WRITE | APR_CREATE |
> APR_TRUNCATE),
> - APR_OS_DEFAULT, pool));
> + (APR_READ | APR_WRITE | APR_CREATE |
> APR_TRUNCATE
> + | APR_BUFFERED), APR_OS_DEFAULT, pool));
> for (i = 0; i < UNIDIFF_LINES; i++)
> {
> apr_size_t len = strlen(unidiff_patch[i]);
> @@ -415,9 +417,12 @@ test_patch(const svn_test_opts_t *opts,
> pcb.patched_tempfiles = apr_hash_make(pool);
> pcb.reject_tempfiles = apr_hash_make(pool);
> pcb.state_pool = pool;
> - SVN_ERR(svn_client_patch(patch_file_path, wc_path, FALSE, 0, FALSE,
> - FALSE, FALSE, patch_collection_func, &pcb,
> - ctx, pool));
> +
> + off = 0;
> + SVN_ERR(svn_io_file_seek(patch_file, APR_SET, &off, pool));
> + SVN_ERR(svn_client_patch2(patch_file, wc_path, FALSE, 0, FALSE,
> + FALSE, FALSE, patch_collection_func, &pcb,
> + ctx, pool));
> SVN_ERR(svn_io_file_close(patch_file, pool));
>
> SVN_TEST_ASSERT(apr_hash_count(pcb.patched_tempfiles) == 1);
>
>
>
--
Timofei Zhakov
