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
Timofei Zhakov <[email protected]>
| Newsgroups | gmane.comp.version-control.subversion.svn,gmane.comp.apache.incubator.ranger.devel |
|---|---|
| Message-ID | <CAPD1kXRiqUP=rkcMdQ8T0pi6EaNueg_zr6nihvm3vVV2akE92w__30033.2370957889$1746803144$gmane$org@mail.gmail.com> |
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
svn_client_patch_stream.patch.txt
(text/plain, 5.3 KB)
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;
}