Re: svn commit: r1925463 - in /subversion/trunk/subversion: include/svn_client.h libsvn_client/deprecated.c libsvn_client/patch.c svn/patch-cmd.c tests/libsvn_client/client-test.c

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 <[email protected]> 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