On 8. 5. 25 19:01, Timofei Zhakov wrote:
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?
Which specific error is returned is not, IMO, part of our compatibility
guarantees. However:
[...]
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
<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));
*This* is clearly wrong. If the call to svn_client_patch2 fails, we leak
the file descriptor. That doesn't affect the svn command-line much but
it does affect users of the library. Looking at the whole commit, it
appears that the same bug existed in apply_patches() before your change.
[...]
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
<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));
+
Here, you're duplicating code that you just implemented in the
svn_client_patch() wrapper. That doesn't make sense. Instead, you could
create a svn_client__patch() private helper function that's used both
here and by the svn_client_patch() compatibility wrapper. Duplicating
code is a bad idea, because ...
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));
... you also duplicated the file descriptor leak.
[...]
@@ -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);
Same here, although this is a test, so I wouldn't bother fixing it.
The more I look at this change, the more I wonder if svn_client_patch()
should be deprecated at all. It's clearly a useful bit of code, since we
can use it in our command-line implementation. It's also far easier to
use by third parties if they're not doing something complicated. There's
no reason to not have both svn_client_patch() and svn_client_patch2() in
the non-deprecated API.
-- Brane
