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

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