I did several fixes in trunk in relation to the patch implementation and also improved the failed tests a little bit by adding more assertions. I'll appreciate your review on those commits.
Also I made a patch where I introduced the svn_client_patch_stream() function, but its current only consumer is the test I've added. Please find the patch attached to the email. Basically I had to duplicate several argument checks, but if we want to keep the old behaviour of svn_client_patch(), no common helper can be factored out. However, we may factor out separate utilities which will verify one group each, so we can combine them. But I didn't initially like it, and IMO it's fine to duplicate those checks. Any thoughts? If it seems nice to everyone, I'll commit it. On Fri, May 9, 2025 at 4:29 PM Timofei Zhakov <t...@chemodax.net> wrote: > [...] > >> *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. >> > > It should not be leaked because it will be free'd on pool cleanup and even > the file will be closed. As far as I can tell Subversion relies on this > behaviour quite frequently. > > -- > Timofei Zhakov > -- Timofei Zhakov
Index: subversion/include/svn_client.h =================================================================== --- subversion/include/svn_client.h (revision 1925476) +++ subversion/include/svn_client.h (working copy) @@ -7771,6 +7771,25 @@ svn_client_ctx_t *ctx, apr_pool_t *scratch_pool); +/** + * Similar to svn_client_patch(), but the patch is read from a file handle, + * described in @a patch_file. + * + * @since New in 1.15. + */ +svn_error_t * +svn_client_patch_stream(apr_file_t *patch_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); + /** @} */ /** @} end group: Client working copy management */ Index: subversion/libsvn_client/patch.c =================================================================== --- subversion/libsvn_client/patch.c (revision 1925479) +++ subversion/libsvn_client/patch.c (working copy) @@ -3786,3 +3786,52 @@ return SVN_NO_ERROR; } + +svn_error_t * +svn_client_patch_stream(apr_file_t *patch_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; + svn_diff_patch_parser_t *patch_parser; + + if (strip_count < 0) + return svn_error_create(SVN_ERR_INCORRECT_PARAMS, NULL, + _("strip count must be positive")); + + if (svn_path_is_url(wc_dir_abspath)) + return svn_error_createf(SVN_ERR_ILLEGAL_TARGET, NULL, + _("'%s' is not a local path"), + svn_dirent_local_style(wc_dir_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, + _("'%s' does not exist"), + svn_dirent_local_style(wc_dir_abspath, + scratch_pool)); + if (kind != svn_node_dir) + return svn_error_createf(SVN_ERR_ILLEGAL_TARGET, NULL, + _("'%s' is not a directory"), + svn_dirent_local_style(wc_dir_abspath, + scratch_pool)); + + patch_parser = svn_diff_patch_parser_create(patch_file, scratch_pool); + + SVN_WC__CALL_WITH_WRITE_LOCK( + apply_patches(patch_parser, 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); + + return SVN_NO_ERROR; +} Index: subversion/tests/libsvn_client/client-test.c =================================================================== --- subversion/tests/libsvn_client/client-test.c (revision 1925476) +++ subversion/tests/libsvn_client/client-test.c (working copy) @@ -418,7 +418,6 @@ SVN_ERR(svn_client_patch(patch_file_path, 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); key = "A/D/gamma"; @@ -433,6 +432,27 @@ SVN_ERR(check_patch_result(reject_tempfile_path, expected_gamma_reject, APR_EOL_STR, EXPECTED_GAMMA_REJECT_LINES, pool)); + /* svn_client_patch_stream() test */ + apr_hash_clear(pcb.patched_tempfiles); + apr_hash_clear(pcb.reject_tempfiles); + + SVN_ERR(svn_client_patch_stream(patch_file, wc_path, FALSE, 0, FALSE, + FALSE, FALSE, patch_collection_func, &pcb, + ctx, pool)); + + SVN_TEST_ASSERT(apr_hash_count(pcb.patched_tempfiles) == 1); + patched_tempfile_path = apr_hash_get(pcb.patched_tempfiles, key, + APR_HASH_KEY_STRING); + SVN_ERR(check_patch_result(patched_tempfile_path, expected_gamma, "\n", + EXPECTED_GAMMA_LINES, pool)); + SVN_TEST_ASSERT(apr_hash_count(pcb.reject_tempfiles) == 1); + reject_tempfile_path = apr_hash_get(pcb.reject_tempfiles, key, + APR_HASH_KEY_STRING); + SVN_ERR(check_patch_result(reject_tempfile_path, expected_gamma_reject, + APR_EOL_STR, EXPECTED_GAMMA_REJECT_LINES, pool)); + + SVN_ERR(svn_io_file_close(patch_file, pool)); + return SVN_NO_ERROR; }