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