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\cmdlineW: EXCEPTION: SVNUnmatchedError Traceback (most recent call last):File "D:\svn\svn-trunk5\subversion\tests\cmdline\svntest\main.py", line 1986, in runrc = self.pred.run(sandbox) ^^^^^^^^^^^^^^^^^^^^^^File "D:\svn\svn-trunk5\subversion\tests\cmdline\svntest\testcase.py", line 178, in runresult = self.func(sandbox) ^^^^^^^^^^^^^^^^^^File "D:\svn\svn-trunk5\subversion\tests\cmdline\input_validation_tests.py", line 226, in invalid_patch_targetsrun_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_wcsvntest.actions.run_and_verify_svn([], expected_stderr,File "D:\svn\svn-trunk5\subversion\tests\cmdline\svntest\actions.py", line 339, in run_and_verify_svnreturn 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_svn2verify.verify_outputs("Unexpected output", out, err,File "D:\svn\svn-trunk5\subversion\tests\cmdline\svntest\verify.py", line 532, in verify_outputscompare_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_linesraise 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
(I hate it when the mailing list headers make reply-to-list do the wrong
thing...)
- Re: svn commit: r1925463 - in /subversion/trunk/subversion:... Timofei Zhakov
- Re: svn commit: r1925463 - in /subversion/trunk/subver... Branko Čibej
- Re: svn commit: r1925463 - in /subversion/trunk/su... Branko Čibej
- Re: svn commit: r1925463 - in /subversion/trun... Timofei Zhakov
- Re: svn commit: r1925463 - in /subversion/trunk/subver... Timofei Zhakov
- Re: svn commit: r1925463 - in /subversion/trunk/su... Timofei Zhakov
