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;
 }
 

Reply via email to