You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by hw...@apache.org on 2010/05/04 11:45:44 UTC
svn commit: r940786 - in /subversion/trunk/subversion: include/svn_client.h
libsvn_client/patch.c svn/patch-cmd.c tests/libsvn_client/client-test.c
Author: hwright
Date: Tue May 4 09:45:43 2010
New Revision: 940786
URL: http://svn.apache.org/viewvc?rev=940786&view=rev
Log:
Add a callback to the public patch API, to allow consumers to collect
information about the patch targets. This replaces the reject_tempfiles
and patched_tempfiles return parameters.
* subversion/tests/libsvn_client/client-test.c
(patch_collection_baton, patch_collection_func): New.
(test_patch): Use the baton to collect the tested information.
* subversion/svn/patch-cmd.c
(svn_cl__patch): Remove the tempfiles, and don't implement a patch callback.
* subversion/include/svn_client.h
(svn_client_patch_func_t): New.
(svn_client_patch): Remove the output hashes, and add a callback and baton.
* subversion/libsvn_client/patch.c
(init_patch_target): Pass through the REMOVE_TEMPFILES param.
(apply_one_patch): Adjust parameters, and call the callback, where
appropriate.
(apply_patches_baton_t): Adjust members to refer to the updated parameters.
(apply_patches): Pass through parameters to apply_one_patch().
(svn_client_patch): Set the updated baton parameters.
Modified:
subversion/trunk/subversion/include/svn_client.h
subversion/trunk/subversion/libsvn_client/patch.c
subversion/trunk/subversion/svn/patch-cmd.c
subversion/trunk/subversion/tests/libsvn_client/client-test.c
Modified: subversion/trunk/subversion/include/svn_client.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/include/svn_client.h?rev=940786&r1=940785&r2=940786&view=diff
==============================================================================
--- subversion/trunk/subversion/include/svn_client.h (original)
+++ subversion/trunk/subversion/include/svn_client.h Tue May 4 09:45:43 2010
@@ -4833,6 +4833,25 @@ svn_client_info(const char *path_or_url,
*/
/**
+ * The callback invoked by svn_client_patch(). For each patch target,
+ * call this function for @a local_abspath, and return the @a patch_abspath
+ * and @a reject_abspath. Neither @a patch_abspath or @a reject_abspath are
+ * guaranteed to exist (depending on the @a remove_tempfiles parameter for
+ * svn_client_patch() ).
+ *
+ * The const char * parameters may be allocated in @a scratch_pool which
+ * will be cleared after each invocation.
+ *
+ * @since New in 1.7.
+ */
+typedef svn_error_t *(*svn_client_patch_func_t)(
+ void *baton,
+ const char *local_abspath,
+ const char *patch_abspath,
+ const char *reject_abspath,
+ apr_pool_t *scratch_pool);
+
+/**
* Apply a unidiff patch that's located at absolute path
* @a abs_patch_path to the working copy at @a local_abspath.
*
@@ -4867,28 +4886,16 @@ svn_client_info(const char *path_or_url,
* the @a include_patterns are applied first, i.e. the @a exclude_patterns
* are applied to all targets which matched one of the @a include_patterns.
*
- * If @a patched_tempfiles is not NULL, return in @a *patched_tempfiles
- * a mapping {target path -> path to temporary file containing patched result}
- * for all patched targets which were neither skipped nor excluded via
- * @a include_patterns or @a exclude_patterns.
- * Note that if all hunks were rejected, the patched result will look just
- * like the target file, unmodified.
- * If @a reject_tempfiles is not NULL, return in @a *reject_tempfiles
- * a mapping {target path -> path to temporary file containing rejected hunks}
- * Both @a *patched_tempfiles and @a *reject_tempfiles are allocated in
- * @a result_pool, and the key (target path) used is the path as parsed
- * from the patch, but in canonicalized form. The value (path to temporary
- * file) is an absolute path, also in canonicalized form.
- * The temporary files are closed, and it is the caller's responsibility
- * to remove them when they are no longer needed.
- * Using @a patched_tempfiles and @a reject_tempfiles in combination with
- * @a dry_run = TRUE makes it possible to generate a preview of the result
- * of the patching process, e.g. for display purposes, without actually
- * modifying the working copy.
- *
* If @a ignore_whitespaces is TRUE, allow patches to be applied if they
* only differ from the target by whitespaces.
*
+ * If @a remove_tempfiles is TRUE, the temporary patch and reject files will
+ * be removed upon pool cleanup, otherwise, the caller should take ownership
+ * of these files.
+ *
+ * If @a patch_func is non-NULL, invoke @a patch_func with @a patch_baton
+ * for each patch target processed.
+ *
* If @a ctx->notify_func2 is non-NULL, invoke @a ctx->notify_func2 with
* @a ctx->notify_baton2 as patching progresses.
*
@@ -4907,9 +4914,10 @@ svn_client_patch(const char *abs_patch_p
svn_boolean_t reverse,
const apr_array_header_t *include_patterns,
const apr_array_header_t *exclude_patterns,
- apr_hash_t **patched_tempfiles,
- apr_hash_t **reject_tempfiles,
svn_boolean_t ignore_whitespaces,
+ svn_boolean_t remove_tempfiles,
+ svn_client_patch_func_t patch_func,
+ void *patch_baton,
svn_client_ctx_t *ctx,
apr_pool_t *result_pool,
apr_pool_t *scratch_pool);
Modified: subversion/trunk/subversion/libsvn_client/patch.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_client/patch.c?rev=940786&r1=940785&r2=940786&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_client/patch.c (original)
+++ subversion/trunk/subversion/libsvn_client/patch.c Tue May 4 09:45:43 2010
@@ -333,9 +333,7 @@ resolve_target_path(patch_target_t *targ
* Else, set *target to NULL.
* If a target does not match a glob in INCLUDE_PATTERNS, mark it as filtered.
* If a target matches a glob in EXCLUDE_PATTERNS, mark it as filtered.
- * If PATCHED_TEMPFILES or REJECT_TEMPFILES are not NULL, add the path
- * to temporary patched/reject files to them, keyed by the target's path
- * as parsed from the patch file (after canonicalization).
+ * REMOVE_TEMPFILES as in svn_client_patch().
* Use SCRATCH_POOL for all other allocations. */
static svn_error_t *
init_patch_target(patch_target_t **patch_target,
@@ -344,8 +342,7 @@ init_patch_target(patch_target_t **patch
svn_wc_context_t *wc_ctx, int strip_count,
const apr_array_header_t *include_patterns,
const apr_array_header_t *exclude_patterns,
- apr_hash_t *patched_tempfiles,
- apr_hash_t *reject_tempfiles,
+ svn_boolean_t remove_tempfiles,
apr_pool_t *result_pool, apr_pool_t *scratch_pool)
{
patch_target_t *target;
@@ -477,13 +474,10 @@ init_patch_target(patch_target_t **patch
/* Create a temporary file to write the patched result to. */
SVN_ERR(svn_stream_open_unique(&target->patched_raw,
&target->patched_path, NULL,
- patched_tempfiles ?
- svn_io_file_del_none :
- svn_io_file_del_on_pool_cleanup,
+ remove_tempfiles ?
+ svn_io_file_del_on_pool_cleanup :
+ svn_io_file_del_none,
result_pool, scratch_pool));
- if (patched_tempfiles)
- apr_hash_set(patched_tempfiles, target->canon_path_from_patchfile,
- APR_HASH_KEY_STRING, target->patched_path);
/* Expand keywords in the patched file.
* Repair newlines if svn:eol-style dictates a particular style. */
@@ -498,13 +492,10 @@ init_patch_target(patch_target_t **patch
* in reject files. */
SVN_ERR(svn_stream_open_unique(&target->reject,
&target->reject_path, NULL,
- reject_tempfiles ?
- svn_io_file_del_none :
- svn_io_file_del_on_pool_cleanup,
+ remove_tempfiles ?
+ svn_io_file_del_on_pool_cleanup :
+ svn_io_file_del_none,
result_pool, scratch_pool));
- if (reject_tempfiles)
- apr_hash_set(reject_tempfiles, target->canon_path_from_patchfile,
- APR_HASH_KEY_STRING, target->reject_path);
/* The reject stream needs a diff header. */
diff_header = apr_psprintf(scratch_pool, "--- %s%s+++ %s%s",
@@ -1143,9 +1134,7 @@ send_patch_notification(const patch_targ
* which should be stripped from target paths in the patch.
* If a target does not match a glob in INCLUDE_PATTERNS, mark it as filtered.
* If a target matches a glob in EXCLUDE_PATTERNS, mark it as filtered.
- * If PATCHED_TEMPFILES or REJECT_TEMPFILES are not NULL, add the path
- * to temporary patched/reject files to them, keyed by the target's path
- * as parsed from the patch file (after canonicalization).
+ * REMOVE_TEMPFILES, PATCH_FUNC, and PATCH_BATON as in svn_client_patch().
* IGNORE_WHITESPACE tells whether whitespace should be considered when
* doing the matching.
* Call cancel CANCEL_FUNC with baton CANCEL_BATON to trigger cancellation.
@@ -1156,9 +1145,10 @@ apply_one_patch(patch_target_t **patch_t
int strip_count,
const apr_array_header_t *include_patterns,
const apr_array_header_t *exclude_patterns,
- apr_hash_t *patched_tempfiles,
- apr_hash_t *reject_tempfiles,
svn_boolean_t ignore_whitespace,
+ svn_boolean_t remove_tempfiles,
+ svn_client_patch_func_t patch_func,
+ void *patch_baton,
svn_cancel_func_t cancel_func,
void *cancel_baton,
apr_pool_t *result_pool, apr_pool_t *scratch_pool)
@@ -1170,8 +1160,7 @@ apply_one_patch(patch_target_t **patch_t
SVN_ERR(init_patch_target(&target, patch, abs_wc_path, wc_ctx, strip_count,
include_patterns, exclude_patterns,
- patched_tempfiles, reject_tempfiles,
- result_pool, scratch_pool));
+ remove_tempfiles, result_pool, scratch_pool));
if (target->skipped || target->filtered)
{
@@ -1286,6 +1275,12 @@ apply_one_patch(patch_target_t **patch_t
}
*patch_target = target;
+
+ if (patch_func)
+ SVN_ERR(patch_func(patch_baton, target->canon_path_from_patchfile,
+ target->patched_path, target->reject_path,
+ scratch_pool));
+
return SVN_NO_ERROR;
}
@@ -1780,16 +1775,16 @@ typedef struct {
/* Files matching any of these patterns won't be patched. */
const apr_array_header_t *exclude_patterns;
- /* Mapping patch target path -> path to tempfile with patched result. */
- apr_hash_t *patched_tempfiles;
-
- /* Mapping patch target path -> path to tempfile with rejected hunks. */
- apr_hash_t *reject_tempfiles;
-
/* Indicates whether we should ignore whitespace when matching context
* lines */
svn_boolean_t ignore_whitespace;
+ /* As in svn_client_patch(). */
+ svn_boolean_t remove_tempfiles;
+
+ /* As in svn_client_patch(). */
+ svn_client_patch_func_t patch_func;
+ void *patch_baton;
/* The client context. */
svn_client_ctx_t *ctx;
@@ -1808,9 +1803,7 @@ apply_patches(void *baton,
apr_file_t *patch_file;
apr_array_header_t *targets;
int i;
- apply_patches_baton_t *btn;
-
- btn = (apply_patches_baton_t *)baton;
+ apply_patches_baton_t *btn = baton;
/* Try to open the patch file. */
SVN_ERR(svn_io_file_open(&patch_file, btn->abs_patch_path,
@@ -1845,8 +1838,9 @@ apply_patches(void *baton,
SVN_ERR(apply_one_patch(&target, patch, btn->abs_wc_path,
btn->ctx->wc_ctx, btn->strip_count,
btn->include_patterns, btn->exclude_patterns,
- btn->patched_tempfiles, btn->reject_tempfiles,
btn->ignore_whitespace,
+ btn->remove_tempfiles,
+ btn->patch_func, btn->patch_baton,
btn->ctx->cancel_func,
btn->ctx->cancel_baton,
result_pool, iterpool));
@@ -1896,9 +1890,10 @@ svn_client_patch(const char *abs_patch_p
svn_boolean_t reverse,
const apr_array_header_t *include_patterns,
const apr_array_header_t *exclude_patterns,
- apr_hash_t **patched_tempfiles,
- apr_hash_t **reject_tempfiles,
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 *result_pool,
apr_pool_t *scratch_pool)
@@ -1918,25 +1913,11 @@ svn_client_patch(const char *abs_patch_p
baton.include_patterns = include_patterns;
baton.exclude_patterns = exclude_patterns;
baton.ignore_whitespace = ignore_whitespace;
+ baton.remove_tempfiles = remove_tempfiles;
+ baton.patch_func = patch_func;
+ baton.patch_baton = patch_baton;
- if (patched_tempfiles)
- {
- (*patched_tempfiles) = apr_hash_make(result_pool);
- baton.patched_tempfiles = (*patched_tempfiles);
- }
- else
- baton.patched_tempfiles = NULL;
- if (reject_tempfiles)
- {
- (*reject_tempfiles) = apr_hash_make(result_pool);
- baton.reject_tempfiles = (*reject_tempfiles);
- }
- else
- baton.reject_tempfiles = NULL;
-
- SVN_ERR(svn_wc__call_with_write_lock(apply_patches, &baton,
+ return svn_error_return(svn_wc__call_with_write_lock(apply_patches, &baton,
ctx->wc_ctx, local_abspath,
result_pool, scratch_pool));
-
- return SVN_NO_ERROR;
}
Modified: subversion/trunk/subversion/svn/patch-cmd.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/svn/patch-cmd.c?rev=940786&r1=940785&r2=940786&view=diff
==============================================================================
--- subversion/trunk/subversion/svn/patch-cmd.c (original)
+++ subversion/trunk/subversion/svn/patch-cmd.c Tue May 4 09:45:43 2010
@@ -81,8 +81,8 @@ svn_cl__patch(apr_getopt_t *os,
opt_state->reverse_diff,
opt_state->include_patterns,
opt_state->exclude_patterns,
- NULL, NULL,
- opt_state->ignore_whitespaces, ctx, pool, pool));
+ opt_state->ignore_whitespaces,
+ TRUE, NULL, NULL, ctx, pool, pool));
if (! opt_state->quiet)
Modified: subversion/trunk/subversion/tests/libsvn_client/client-test.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/libsvn_client/client-test.c?rev=940786&r1=940785&r2=940786&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/libsvn_client/client-test.c (original)
+++ subversion/trunk/subversion/tests/libsvn_client/client-test.c Tue May 4 09:45:43 2010
@@ -246,6 +246,39 @@ check_patch_result(const char *path, con
return SVN_NO_ERROR;
}
+/* A baton for the patch collection function. */
+struct patch_collection_baton
+{
+ apr_hash_t *patched_tempfiles;
+ apr_hash_t *reject_tempfiles;
+ apr_pool_t *state_pool;
+};
+
+/* Collect all the patch information we're interested in. */
+static svn_error_t *
+patch_collection_func(void *baton,
+ const char *local_abspath,
+ const char *patch_abspath,
+ const char *reject_abspath,
+ apr_pool_t *scratch_pool)
+{
+ struct patch_collection_baton *pcb = baton;
+
+ if (patch_abspath)
+ apr_hash_set(pcb->patched_tempfiles,
+ apr_pstrdup(pcb->state_pool, local_abspath),
+ APR_HASH_KEY_STRING,
+ apr_pstrdup(pcb->state_pool, patch_abspath));
+
+ if (reject_abspath)
+ apr_hash_set(pcb->reject_tempfiles,
+ apr_pstrdup(pcb->state_pool, local_abspath),
+ APR_HASH_KEY_STRING,
+ apr_pstrdup(pcb->state_pool, reject_abspath));
+
+ return SVN_NO_ERROR;
+}
+
static svn_error_t *
test_patch(const svn_test_opts_t *opts,
apr_pool_t *pool)
@@ -254,8 +287,6 @@ test_patch(const svn_test_opts_t *opts,
svn_fs_t *fs;
svn_fs_txn_t *txn;
svn_fs_root_t *txn_root;
- apr_hash_t *patched_tempfiles;
- apr_hash_t *reject_tempfiles;
const char *repos_url;
const char *wc_path;
const char *cwd;
@@ -264,6 +295,7 @@ test_patch(const svn_test_opts_t *opts,
svn_opt_revision_t peg_rev;
svn_client_ctx_t *ctx;
apr_file_t *patch_file;
+ struct patch_collection_baton pcb;
const char *patch_file_path;
const char *patched_tempfile_path;
const char *reject_tempfile_path;
@@ -348,20 +380,23 @@ test_patch(const svn_test_opts_t *opts,
SVN_ERR(svn_io_file_flush_to_disk(patch_file, pool));
/* Apply the patch. */
+ 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,
- NULL, NULL, &patched_tempfiles, &reject_tempfiles,
- FALSE, ctx, pool, pool));
+ NULL, NULL, FALSE, TRUE, patch_collection_func,
+ &pcb, ctx, pool, pool));
SVN_ERR(svn_io_file_close(patch_file, pool));
- SVN_ERR_ASSERT(apr_hash_count(patched_tempfiles) == 1);
+ SVN_ERR_ASSERT(apr_hash_count(pcb.patched_tempfiles) == 1);
key = "A/D/gamma";
- patched_tempfile_path = apr_hash_get(patched_tempfiles, key,
+ 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_ERR_ASSERT(apr_hash_count(reject_tempfiles) == 1);
+ SVN_ERR_ASSERT(apr_hash_count(pcb.reject_tempfiles) == 1);
key = "A/D/gamma";
- reject_tempfile_path = apr_hash_get(reject_tempfiles, key,
+ 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));
Re: svn commit: r940786 - in /subversion/trunk/subversion:
include/svn_client.h libsvn_client/patch.c svn/patch-cmd.c
tests/libsvn_client/client-test.c
Posted by Julian Foad <ju...@wandisco.com>.
On Tue, 2010-05-04 at 17:55 +0200, Hyrum K. Wright wrote:
> On Tue, May 4, 2010 at 12:45 PM, Stefan Sperling <st...@elego.de> wrote:
>
> > On Tue, May 04, 2010 at 09:45:44AM -0000, hwright@apache.org wrote:
> > > Author: hwright
> > > Date: Tue May 4 09:45:43 2010
> > > New Revision: 940786
> > >
> > > URL: http://svn.apache.org/viewvc?rev=940786&view=rev
> > > Log:
> > > Add a callback to the public patch API, to allow consumers to collect
> > > information about the patch targets. This replaces the reject_tempfiles
> > > and patched_tempfiles return parameters.
> > >
> > > * subversion/tests/libsvn_client/client-test.c
> > > (patch_collection_baton, patch_collection_func): New.
> > > (test_patch): Use the baton to collect the tested information.
> > >
> > > * subversion/svn/patch-cmd.c
> > > (svn_cl__patch): Remove the tempfiles, and don't implement a patch
> > callback.
> > >
> > > * subversion/include/svn_client.h
> > > (svn_client_patch_func_t): New.
> > > (svn_client_patch): Remove the output hashes, and add a callback and
> > baton.
> > >
> > > * subversion/libsvn_client/patch.c
> > > (init_patch_target): Pass through the REMOVE_TEMPFILES param.
> > > (apply_one_patch): Adjust parameters, and call the callback, where
> > > appropriate.
> > > (apply_patches_baton_t): Adjust members to refer to the updated
> > parameters.
> > > (apply_patches): Pass through parameters to apply_one_patch().
> > > (svn_client_patch): Set the updated baton parameters.
> > >
> > > Modified:
> > > subversion/trunk/subversion/include/svn_client.h
> > > subversion/trunk/subversion/libsvn_client/patch.c
> > > subversion/trunk/subversion/svn/patch-cmd.c
> > > subversion/trunk/subversion/tests/libsvn_client/client-test.c
> > >
> > > Modified: subversion/trunk/subversion/include/svn_client.h
> > > URL:
> > http://svn.apache.org/viewvc/subversion/trunk/subversion/include/svn_client.h?rev=940786&r1=940785&r2=940786&view=diff
> > >
> > ==============================================================================
> > > --- subversion/trunk/subversion/include/svn_client.h (original)
> > > +++ subversion/trunk/subversion/include/svn_client.h Tue May 4 09:45:43
> > 2010
> > > @@ -4833,6 +4833,25 @@ svn_client_info(const char *path_or_url,
> > > */
> > >
> > > /**
> > > + * The callback invoked by svn_client_patch(). For each patch target,
> > > + * call this function for @a local_abspath, and return the @a
> > patch_abspath
> > > + * and @a reject_abspath. Neither @a patch_abspath or @a reject_abspath
> > are
> > > + * guaranteed to exist (depending on the @a remove_tempfiles parameter
> > for
> > > + * svn_client_patch() ).
> >
> >
> > What is the reason for the remove_tempfiles parameter?
> > Do you envision the callback to be used for additional tasks in the future?
> > Why not just drop the remove_tempfiles parameter and clean up the
> > tempfiles if the caller passes NULL for the callback?
>
>
> I feel uncomfortable overloading the callback parameter in that way. The
> value of the function pointer should not have operational side effects, such
> as leaving or not leaving tempfiles in the working copy. It makes more
> sense to me to make that explicit. If we currently require that
> remove_tempfiles should be FALSE when no callback is given, we can document
> it as such, and add an assertion in the function.
+1 on not overloading it. That ties in with my queries on the callback
doc string: part of the reason I was asking for lots of clarification is
because I got the feeling that something "magic" was happening that
wasn't being said - such as leaving temp files iff the callback function
pointer was non-null - but wasn't sure whether that was supposed to be
implied or not.
- Julian
Re: svn commit: r940786 - in /subversion/trunk/subversion:
include/svn_client.h libsvn_client/patch.c svn/patch-cmd.c
tests/libsvn_client/client-test.c
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
On Tue, May 4, 2010 at 12:45 PM, Stefan Sperling <st...@elego.de> wrote:
> On Tue, May 04, 2010 at 09:45:44AM -0000, hwright@apache.org wrote:
> > Author: hwright
> > Date: Tue May 4 09:45:43 2010
> > New Revision: 940786
> >
> > URL: http://svn.apache.org/viewvc?rev=940786&view=rev
> > Log:
> > Add a callback to the public patch API, to allow consumers to collect
> > information about the patch targets. This replaces the reject_tempfiles
> > and patched_tempfiles return parameters.
> >
> > * subversion/tests/libsvn_client/client-test.c
> > (patch_collection_baton, patch_collection_func): New.
> > (test_patch): Use the baton to collect the tested information.
> >
> > * subversion/svn/patch-cmd.c
> > (svn_cl__patch): Remove the tempfiles, and don't implement a patch
> callback.
> >
> > * subversion/include/svn_client.h
> > (svn_client_patch_func_t): New.
> > (svn_client_patch): Remove the output hashes, and add a callback and
> baton.
> >
> > * subversion/libsvn_client/patch.c
> > (init_patch_target): Pass through the REMOVE_TEMPFILES param.
> > (apply_one_patch): Adjust parameters, and call the callback, where
> > appropriate.
> > (apply_patches_baton_t): Adjust members to refer to the updated
> parameters.
> > (apply_patches): Pass through parameters to apply_one_patch().
> > (svn_client_patch): Set the updated baton parameters.
> >
> > Modified:
> > subversion/trunk/subversion/include/svn_client.h
> > subversion/trunk/subversion/libsvn_client/patch.c
> > subversion/trunk/subversion/svn/patch-cmd.c
> > subversion/trunk/subversion/tests/libsvn_client/client-test.c
> >
> > Modified: subversion/trunk/subversion/include/svn_client.h
> > URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/include/svn_client.h?rev=940786&r1=940785&r2=940786&view=diff
> >
> ==============================================================================
> > --- subversion/trunk/subversion/include/svn_client.h (original)
> > +++ subversion/trunk/subversion/include/svn_client.h Tue May 4 09:45:43
> 2010
> > @@ -4833,6 +4833,25 @@ svn_client_info(const char *path_or_url,
> > */
> >
> > /**
> > + * The callback invoked by svn_client_patch(). For each patch target,
> > + * call this function for @a local_abspath, and return the @a
> patch_abspath
> > + * and @a reject_abspath. Neither @a patch_abspath or @a reject_abspath
> are
> > + * guaranteed to exist (depending on the @a remove_tempfiles parameter
> for
> > + * svn_client_patch() ).
>
>
> What is the reason for the remove_tempfiles parameter?
> Do you envision the callback to be used for additional tasks in the future?
> Why not just drop the remove_tempfiles parameter and clean up the
> tempfiles if the caller passes NULL for the callback?
I feel uncomfortable overloading the callback parameter in that way. The
value of the function pointer should not have operational side effects, such
as leaving or not leaving tempfiles in the working copy. It makes more
sense to me to make that explicit. If we currently require that
remove_tempfiles should be FALSE when no callback is given, we can document
it as such, and add an assertion in the function.
-Hyrum
Re: svn commit: r940786 - in /subversion/trunk/subversion:
include/svn_client.h libsvn_client/patch.c svn/patch-cmd.c
tests/libsvn_client/client-test.c
Posted by Stefan Sperling <st...@elego.de>.
On Tue, May 04, 2010 at 09:45:44AM -0000, hwright@apache.org wrote:
> Author: hwright
> Date: Tue May 4 09:45:43 2010
> New Revision: 940786
>
> URL: http://svn.apache.org/viewvc?rev=940786&view=rev
> Log:
> Add a callback to the public patch API, to allow consumers to collect
> information about the patch targets. This replaces the reject_tempfiles
> and patched_tempfiles return parameters.
>
> * subversion/tests/libsvn_client/client-test.c
> (patch_collection_baton, patch_collection_func): New.
> (test_patch): Use the baton to collect the tested information.
>
> * subversion/svn/patch-cmd.c
> (svn_cl__patch): Remove the tempfiles, and don't implement a patch callback.
>
> * subversion/include/svn_client.h
> (svn_client_patch_func_t): New.
> (svn_client_patch): Remove the output hashes, and add a callback and baton.
>
> * subversion/libsvn_client/patch.c
> (init_patch_target): Pass through the REMOVE_TEMPFILES param.
> (apply_one_patch): Adjust parameters, and call the callback, where
> appropriate.
> (apply_patches_baton_t): Adjust members to refer to the updated parameters.
> (apply_patches): Pass through parameters to apply_one_patch().
> (svn_client_patch): Set the updated baton parameters.
>
> Modified:
> subversion/trunk/subversion/include/svn_client.h
> subversion/trunk/subversion/libsvn_client/patch.c
> subversion/trunk/subversion/svn/patch-cmd.c
> subversion/trunk/subversion/tests/libsvn_client/client-test.c
>
> Modified: subversion/trunk/subversion/include/svn_client.h
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/include/svn_client.h?rev=940786&r1=940785&r2=940786&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/include/svn_client.h (original)
> +++ subversion/trunk/subversion/include/svn_client.h Tue May 4 09:45:43 2010
> @@ -4833,6 +4833,25 @@ svn_client_info(const char *path_or_url,
> */
>
> /**
> + * The callback invoked by svn_client_patch(). For each patch target,
> + * call this function for @a local_abspath, and return the @a patch_abspath
> + * and @a reject_abspath. Neither @a patch_abspath or @a reject_abspath are
> + * guaranteed to exist (depending on the @a remove_tempfiles parameter for
> + * svn_client_patch() ).
What is the reason for the remove_tempfiles parameter?
Do you envision the callback to be used for additional tasks in the future?
Why not just drop the remove_tempfiles parameter and clean up the
tempfiles if the caller passes NULL for the callback?
Stefan
Re: svn commit: r940786 - in /subversion/trunk/subversion:
include/svn_client.h libsvn_client/patch.c svn/patch-cmd.c
tests/libsvn_client/client-test.c
Posted by Julian Foad <ju...@wandisco.com>.
On Tue, 2010-05-04 at 23:23 +0200, Hyrum K. Wright wrote:
> On Tue, May 4, 2010 at 12:36 PM, Julian Foad <ju...@wandisco.com>wrote:
> > Jumping in at this point, I can't grok this doc string. Can you write
> > it as a statement of what this function (a function of this type) is
> > required to do or promises to do or is expected to do? Try not to
> > assume the meanings of the parameters are totally obvious, and avoid
> > saying "return" when describing a parameter. ("const char *" is not
> > normally associated with "return" - are those parameters outputs?)
>
> A agree that the docstring is a bit confusing, but I'll make a couple of
> comments about it. This docstring actually is *more* explanatory than
> docstrings for similar callbacks elsewhere in svn_client.h. The problem is
> more rampant than just this one (though that does not excuse sloppiness in
> this case.)
>
> Asking what this function promises or is expected to do is itself a bit
> confusing. For instance, what does the blame receiver promise? Most of the
> callbacks in the client are informational. As this one is similar to the
> others, who are similarly documented, I assumed that aspect was apparent.
Hi Hyrum.
Sorry that my email sounded harsh - my long list of questions made it
look like I had a lot of problems with it, when in fact it probably just
needed a little clarification.
I don't expect any doc string to spell out the answers in full to all
the questions I asked. It should just say enough to enable the reader
to be confident of the answers. I had lots of questions because I
didn't know what's relevant, largely because I was confused due to
having an expectation that this callback played a part in *controlling*
the behaviour of the patching process. From the other emails I
understand that's not the case, it's a purely informational callback.
I think it's always fair to ask "What does this callback promise?" (as
well as "What should it expect?"). Maybe it promises not to modify the
target file. Maybe it doesn't promise anything. I don't know whether
the blame receiver promises anything. It's fine for it not to, and it's
fine not to mention any promises if it doesn't seem relevant.
> > For example, is it guaranteed to be called before or after the target is
> > patched? Is function able to examine the patch? Is it allowed to
> > modify the target file before patching? Should it or can it attempt to
> > apply the patch itself, or adjust the patch, or reject the patch, or
> > cause this patch to get applied to a different target? How can this
> > function, or how can the caller of svn_client_patch(), know whether
> > patch application has been successful?
[...]
> > Wasn't the idea that the caller could choose whether the patched result
> > should be written straight to the target file or to somewhere else? I'm
> > not sure but I thought specifying @a patched_tempfiles != NULL caused
> > that to happen. Is that right and if so does the callback support that
> > feature?
>
> I think Stefan addressed this already.
Yup.
[...]
> Thanks for the review. I committed an update to the docstrings in r941049.
> Unfortunately, I'm also late to the party when it comes to the internals of
> how the patch code works, so I'd suggest that further improvements should be
> made by those who have the most familiarity with the code.
Thanks, r941049 is better.
- Julian
Re: svn commit: r940786 - in /subversion/trunk/subversion:
include/svn_client.h libsvn_client/patch.c svn/patch-cmd.c
tests/libsvn_client/client-test.c
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
On Tue, May 4, 2010 at 12:36 PM, Julian Foad <ju...@wandisco.com>wrote:
> On Tue, 2010-05-04, hwright@apache.org wrote:
> > Author: hwright
> > Date: Tue May 4 09:45:43 2010
> > New Revision: 940786
> >
> > URL: http://svn.apache.org/viewvc?rev=940786&view=rev
> > Log:
> > Add a callback to the public patch API, to allow consumers to collect
> > information about the patch targets. This replaces the reject_tempfiles
> > and patched_tempfiles return parameters.
> >
> > * subversion/tests/libsvn_client/client-test.c
> > (patch_collection_baton, patch_collection_func): New.
> > (test_patch): Use the baton to collect the tested information.
> >
> > * subversion/svn/patch-cmd.c
> > (svn_cl__patch): Remove the tempfiles, and don't implement a patch
> callback.
> >
> > * subversion/include/svn_client.h
> > (svn_client_patch_func_t): New.
> > (svn_client_patch): Remove the output hashes, and add a callback and
> baton.
> >
> > * subversion/libsvn_client/patch.c
> > (init_patch_target): Pass through the REMOVE_TEMPFILES param.
> > (apply_one_patch): Adjust parameters, and call the callback, where
> > appropriate.
> > (apply_patches_baton_t): Adjust members to refer to the updated
> parameters.
> > (apply_patches): Pass through parameters to apply_one_patch().
> > (svn_client_patch): Set the updated baton parameters.
> >
> > Modified:
> > subversion/trunk/subversion/include/svn_client.h
> > subversion/trunk/subversion/libsvn_client/patch.c
> > subversion/trunk/subversion/svn/patch-cmd.c
> > subversion/trunk/subversion/tests/libsvn_client/client-test.c
>
> Just looking at the API change...
>
> > Modified: subversion/trunk/subversion/include/svn_client.h
> > URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/include/svn_client.h?rev=940786&r1=940785&r2=940786&view=diff
> >
> ==============================================================================
> > --- subversion/trunk/subversion/include/svn_client.h (original)
> > +++ subversion/trunk/subversion/include/svn_client.h Tue May 4 09:45:43
> 2010
> > @@ -4833,6 +4833,25 @@ svn_client_info(const char *path_or_url,
> > */
> >
> > /**
> > + * The callback invoked by svn_client_patch(). For each patch target,
> > + * call this function for @a local_abspath, and return the @a
> patch_abspath
> > + * and @a reject_abspath. Neither @a patch_abspath or @a reject_abspath
> are
> > + * guaranteed to exist (depending on the @a remove_tempfiles parameter
> for
> > + * svn_client_patch() ).
>
> Jumping in at this point, I can't grok this doc string. Can you write
> it as a statement of what this function (a function of this type) is
> required to do or promises to do or is expected to do? Try not to
> assume the meanings of the parameters are totally obvious, and avoid
> saying "return" when describing a parameter. ("const char *" is not
> normally associated with "return" - are those parameters outputs?)
>
A agree that the docstring is a bit confusing, but I'll make a couple of
comments about it. This docstring actually is *more* explanatory than
docstrings for similar callbacks elsewhere in svn_client.h. The problem is
more rampant than just this one (though that does not excuse sloppiness in
this case.)
Asking what this function promises or is expected to do is itself a bit
confusing. For instance, what does the blame receiver promise? Most of the
callbacks in the client are informational. As this one is similar to the
others, who are similarly documented, I assumed that aspect was apparent.
> For example, is it guaranteed to be called before or after the target is
> patched? Is function able to examine the patch? Is it allowed to
> modify the target file before patching? Should it or can it attempt to
> apply the patch itself, or adjust the patch, or reject the patch, or
> cause this patch to get applied to a different target? How can this
> function, or how can the caller of svn_client_patch(), know whether
> patch application has been successful?
>
> > + *
> > + * The const char * parameters may be allocated in @a scratch_pool which
> > + * will be cleared after each invocation.
> > + *
> > + * @since New in 1.7.
> > + */
> > +typedef svn_error_t *(*svn_client_patch_func_t)(
> > + void *baton,
> > + const char *local_abspath,
> > + const char *patch_abspath,
> > + const char *reject_abspath,
> > + apr_pool_t *scratch_pool);
> > +
> > +/**
> > * Apply a unidiff patch that's located at absolute path
> > * @a abs_patch_path to the working copy at @a local_abspath.
> > *
> > @@ -4867,28 +4886,16 @@ svn_client_info(const char *path_or_url,
> > * the @a include_patterns are applied first, i.e. the @a
> exclude_patterns
> > * are applied to all targets which matched one of the @a
> include_patterns.
> > *
> > - * If @a patched_tempfiles is not NULL, return in @a *patched_tempfiles
> > - * a mapping {target path -> path to temporary file containing patched
> result}
> > - * for all patched targets which were neither skipped nor excluded via
> > - * @a include_patterns or @a exclude_patterns.
> > - * Note that if all hunks were rejected, the patched result will look
> just
> > - * like the target file, unmodified.
> > - * If @a reject_tempfiles is not NULL, return in @a *reject_tempfiles
> > - * a mapping {target path -> path to temporary file containing rejected
> hunks}
> > - * Both @a *patched_tempfiles and @a *reject_tempfiles are allocated in
> > - * @a result_pool, and the key (target path) used is the path as parsed
> > - * from the patch, but in canonicalized form. The value (path to
> temporary
> > - * file) is an absolute path, also in canonicalized form.
> > - * The temporary files are closed, and it is the caller's responsibility
> > - * to remove them when they are no longer needed.
> > - * Using @a patched_tempfiles and @a reject_tempfiles in combination
> with
> > - * @a dry_run = TRUE makes it possible to generate a preview of the
> result
> > - * of the patching process, e.g. for display purposes, without actually
> > - * modifying the working copy.
> > - *
> > * If @a ignore_whitespaces is TRUE, allow patches to be applied if they
> > * only differ from the target by whitespaces.
> > *
> > + * If @a remove_tempfiles is TRUE, the temporary patch and reject files
> will
> > + * be removed upon pool cleanup, otherwise, the caller should take
> ownership
>
> Pool cleanup - which pool?
>
> > + * of these files.
> > + *
> > + * If @a patch_func is non-NULL, invoke @a patch_func with @a
> patch_baton
> > + * for each patch target processed.
>
> Wasn't the idea that the caller could choose whether the patched result
> should be written straight to the target file or to somewhere else? I'm
> not sure but I thought specifying @a patched_tempfiles != NULL caused
> that to happen. Is that right and if so does the callback support that
> feature?
>
I think Stefan addressed this already.
> * If @a ctx->notify_func2 is non-NULL, invoke @a ctx->notify_func2 with
> > * @a ctx->notify_baton2 as patching progresses.
> > *
> > @@ -4907,9 +4914,10 @@ svn_client_patch(const char *abs_patch_p
> > svn_boolean_t reverse,
> > const apr_array_header_t *include_patterns,
> > const apr_array_header_t *exclude_patterns,
> > - apr_hash_t **patched_tempfiles,
> > - apr_hash_t **reject_tempfiles,
> > svn_boolean_t ignore_whitespaces,
> > + svn_boolean_t remove_tempfiles,
> > + svn_client_patch_func_t patch_func,
> > + void *patch_baton,
> > svn_client_ctx_t *ctx,
> > apr_pool_t *result_pool,
> > apr_pool_t *scratch_pool);
>
Thanks for the review. I committed an update to the docstrings in r941049.
Unfortunately, I'm also late to the party when it comes to the internals of
how the patch code works, so I'd suggest that further improvements should be
made by those who have the most familiarity with the code.
-Hyrum
Re: svn commit: r940786 - in /subversion/trunk/subversion:
include/svn_client.h libsvn_client/patch.c svn/patch-cmd.c
tests/libsvn_client/client-test.c
Posted by Stefan Sperling <st...@elego.de>.
On Tue, May 04, 2010 at 11:36:49AM +0100, Julian Foad wrote:
> Wasn't the idea that the caller could choose whether the patched result
> should be written straight to the target file or to somewhere else?
No, that wasn't the idea.
The patched result is *always* written to tempfiles first. Always has been.
It is never written "straight to the target file" while being computed.
Rather, the target file is eventually overwritten with the tempfile,
resulting in the application of the patched result to the working copy.
(This is of course an implementation detail, but should be understood
when reasoning about the patch code itself. Callers don't need to care
how it is done.)
The caller can suppress the application of the patched result to the
working copy by passing dry_run == TRUE.
The idea behind the mechanism to retrieve the tempfiles is that
applications like TortoiseSVN can pass dry_run == TRUE and look at the
tempfiles to see what the patched result will look like.
Passing the request for access to the tempfiles alone does not imply
dry_run == TRUE, but the primary use case is the dry_run == TRUE case.
If dry_run == FALSE is passed, the working copy will be patched and the caller
will have access to the tempfiles which replaced files in the working
copy (and it's the caller's job to clean them up) -- not very exiting :)
Stefan
Re: svn commit: r940786 - in /subversion/trunk/subversion:
include/svn_client.h libsvn_client/patch.c svn/patch-cmd.c
tests/libsvn_client/client-test.c
Posted by Julian Foad <ju...@wandisco.com>.
On Tue, 2010-05-04, hwright@apache.org wrote:
> Author: hwright
> Date: Tue May 4 09:45:43 2010
> New Revision: 940786
>
> URL: http://svn.apache.org/viewvc?rev=940786&view=rev
> Log:
> Add a callback to the public patch API, to allow consumers to collect
> information about the patch targets. This replaces the reject_tempfiles
> and patched_tempfiles return parameters.
>
> * subversion/tests/libsvn_client/client-test.c
> (patch_collection_baton, patch_collection_func): New.
> (test_patch): Use the baton to collect the tested information.
>
> * subversion/svn/patch-cmd.c
> (svn_cl__patch): Remove the tempfiles, and don't implement a patch callback.
>
> * subversion/include/svn_client.h
> (svn_client_patch_func_t): New.
> (svn_client_patch): Remove the output hashes, and add a callback and baton.
>
> * subversion/libsvn_client/patch.c
> (init_patch_target): Pass through the REMOVE_TEMPFILES param.
> (apply_one_patch): Adjust parameters, and call the callback, where
> appropriate.
> (apply_patches_baton_t): Adjust members to refer to the updated parameters.
> (apply_patches): Pass through parameters to apply_one_patch().
> (svn_client_patch): Set the updated baton parameters.
>
> Modified:
> subversion/trunk/subversion/include/svn_client.h
> subversion/trunk/subversion/libsvn_client/patch.c
> subversion/trunk/subversion/svn/patch-cmd.c
> subversion/trunk/subversion/tests/libsvn_client/client-test.c
Just looking at the API change...
> Modified: subversion/trunk/subversion/include/svn_client.h
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/include/svn_client.h?rev=940786&r1=940785&r2=940786&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/include/svn_client.h (original)
> +++ subversion/trunk/subversion/include/svn_client.h Tue May 4 09:45:43 2010
> @@ -4833,6 +4833,25 @@ svn_client_info(const char *path_or_url,
> */
>
> /**
> + * The callback invoked by svn_client_patch(). For each patch target,
> + * call this function for @a local_abspath, and return the @a patch_abspath
> + * and @a reject_abspath. Neither @a patch_abspath or @a reject_abspath are
> + * guaranteed to exist (depending on the @a remove_tempfiles parameter for
> + * svn_client_patch() ).
Jumping in at this point, I can't grok this doc string. Can you write
it as a statement of what this function (a function of this type) is
required to do or promises to do or is expected to do? Try not to
assume the meanings of the parameters are totally obvious, and avoid
saying "return" when describing a parameter. ("const char *" is not
normally associated with "return" - are those parameters outputs?)
For example, is it guaranteed to be called before or after the target is
patched? Is function able to examine the patch? Is it allowed to
modify the target file before patching? Should it or can it attempt to
apply the patch itself, or adjust the patch, or reject the patch, or
cause this patch to get applied to a different target? How can this
function, or how can the caller of svn_client_patch(), know whether
patch application has been successful?
> + *
> + * The const char * parameters may be allocated in @a scratch_pool which
> + * will be cleared after each invocation.
> + *
> + * @since New in 1.7.
> + */
> +typedef svn_error_t *(*svn_client_patch_func_t)(
> + void *baton,
> + const char *local_abspath,
> + const char *patch_abspath,
> + const char *reject_abspath,
> + apr_pool_t *scratch_pool);
> +
> +/**
> * Apply a unidiff patch that's located at absolute path
> * @a abs_patch_path to the working copy at @a local_abspath.
> *
> @@ -4867,28 +4886,16 @@ svn_client_info(const char *path_or_url,
> * the @a include_patterns are applied first, i.e. the @a exclude_patterns
> * are applied to all targets which matched one of the @a include_patterns.
> *
> - * If @a patched_tempfiles is not NULL, return in @a *patched_tempfiles
> - * a mapping {target path -> path to temporary file containing patched result}
> - * for all patched targets which were neither skipped nor excluded via
> - * @a include_patterns or @a exclude_patterns.
> - * Note that if all hunks were rejected, the patched result will look just
> - * like the target file, unmodified.
> - * If @a reject_tempfiles is not NULL, return in @a *reject_tempfiles
> - * a mapping {target path -> path to temporary file containing rejected hunks}
> - * Both @a *patched_tempfiles and @a *reject_tempfiles are allocated in
> - * @a result_pool, and the key (target path) used is the path as parsed
> - * from the patch, but in canonicalized form. The value (path to temporary
> - * file) is an absolute path, also in canonicalized form.
> - * The temporary files are closed, and it is the caller's responsibility
> - * to remove them when they are no longer needed.
> - * Using @a patched_tempfiles and @a reject_tempfiles in combination with
> - * @a dry_run = TRUE makes it possible to generate a preview of the result
> - * of the patching process, e.g. for display purposes, without actually
> - * modifying the working copy.
> - *
> * If @a ignore_whitespaces is TRUE, allow patches to be applied if they
> * only differ from the target by whitespaces.
> *
> + * If @a remove_tempfiles is TRUE, the temporary patch and reject files will
> + * be removed upon pool cleanup, otherwise, the caller should take ownership
Pool cleanup - which pool?
> + * of these files.
> + *
> + * If @a patch_func is non-NULL, invoke @a patch_func with @a patch_baton
> + * for each patch target processed.
Wasn't the idea that the caller could choose whether the patched result
should be written straight to the target file or to somewhere else? I'm
not sure but I thought specifying @a patched_tempfiles != NULL caused
that to happen. Is that right and if so does the callback support that
feature?
> * If @a ctx->notify_func2 is non-NULL, invoke @a ctx->notify_func2 with
> * @a ctx->notify_baton2 as patching progresses.
> *
> @@ -4907,9 +4914,10 @@ svn_client_patch(const char *abs_patch_p
> svn_boolean_t reverse,
> const apr_array_header_t *include_patterns,
> const apr_array_header_t *exclude_patterns,
> - apr_hash_t **patched_tempfiles,
> - apr_hash_t **reject_tempfiles,
> svn_boolean_t ignore_whitespaces,
> + svn_boolean_t remove_tempfiles,
> + svn_client_patch_func_t patch_func,
> + void *patch_baton,
> svn_client_ctx_t *ctx,
> apr_pool_t *result_pool,
> apr_pool_t *scratch_pool);
- Julian
Re: svn commit: r940786 - in /subversion/trunk/subversion:
include/svn_client.h libsvn_client/patch.c svn/patch-cmd.c
tests/libsvn_client/client-test.c
Posted by Julian Foad <ju...@wandisco.com>.
On Tue, 2010-05-04, hwright@apache.org wrote:
> Author: hwright
> Date: Tue May 4 09:45:43 2010
> New Revision: 940786
>
> URL: http://svn.apache.org/viewvc?rev=940786&view=rev
> Log:
> Add a callback to the public patch API, to allow consumers to collect
> information about the patch targets. This replaces the reject_tempfiles
> and patched_tempfiles return parameters.
>
> * subversion/tests/libsvn_client/client-test.c
> (patch_collection_baton, patch_collection_func): New.
> (test_patch): Use the baton to collect the tested information.
>
> * subversion/svn/patch-cmd.c
> (svn_cl__patch): Remove the tempfiles, and don't implement a patch callback.
>
> * subversion/include/svn_client.h
> (svn_client_patch_func_t): New.
> (svn_client_patch): Remove the output hashes, and add a callback and baton.
>
> * subversion/libsvn_client/patch.c
> (init_patch_target): Pass through the REMOVE_TEMPFILES param.
> (apply_one_patch): Adjust parameters, and call the callback, where
> appropriate.
> (apply_patches_baton_t): Adjust members to refer to the updated parameters.
> (apply_patches): Pass through parameters to apply_one_patch().
> (svn_client_patch): Set the updated baton parameters.
>
> Modified:
> subversion/trunk/subversion/include/svn_client.h
> subversion/trunk/subversion/libsvn_client/patch.c
> subversion/trunk/subversion/svn/patch-cmd.c
> subversion/trunk/subversion/tests/libsvn_client/client-test.c
Just looking at the API change...
> Modified: subversion/trunk/subversion/include/svn_client.h
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/include/svn_client.h?rev=940786&r1=940785&r2=940786&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/include/svn_client.h (original)
> +++ subversion/trunk/subversion/include/svn_client.h Tue May 4 09:45:43 2010
> @@ -4833,6 +4833,25 @@ svn_client_info(const char *path_or_url,
> */
>
> /**
> + * The callback invoked by svn_client_patch(). For each patch target,
> + * call this function for @a local_abspath, and return the @a patch_abspath
> + * and @a reject_abspath. Neither @a patch_abspath or @a reject_abspath are
> + * guaranteed to exist (depending on the @a remove_tempfiles parameter for
> + * svn_client_patch() ).
Jumping in at this point, I can't grok this doc string. Can you write
it as a statement of what this function (a function of this type) is
required to do or promises to do or is expected to do? Try not to
assume the meanings of the parameters are totally obvious, and avoid
saying "return" when describing a parameter. ("const char *" is not
normally associated with "return" - are those parameters outputs?)
For example, is it guaranteed to be called before or after the target is
patched? Is function able to examine the patch? Is it allowed to
modify the target file before patching? Should it or can it attempt to
apply the patch itself, or adjust the patch, or reject the patch, or
cause this patch to get applied to a different target? How can this
function, or how can the caller of svn_client_patch(), know whether
patch application has been successful?
> + *
> + * The const char * parameters may be allocated in @a scratch_pool which
> + * will be cleared after each invocation.
> + *
> + * @since New in 1.7.
> + */
> +typedef svn_error_t *(*svn_client_patch_func_t)(
> + void *baton,
> + const char *local_abspath,
> + const char *patch_abspath,
> + const char *reject_abspath,
> + apr_pool_t *scratch_pool);
> +
> +/**
> * Apply a unidiff patch that's located at absolute path
> * @a abs_patch_path to the working copy at @a local_abspath.
> *
> @@ -4867,28 +4886,16 @@ svn_client_info(const char *path_or_url,
> * the @a include_patterns are applied first, i.e. the @a exclude_patterns
> * are applied to all targets which matched one of the @a include_patterns.
> *
> - * If @a patched_tempfiles is not NULL, return in @a *patched_tempfiles
> - * a mapping {target path -> path to temporary file containing patched result}
> - * for all patched targets which were neither skipped nor excluded via
> - * @a include_patterns or @a exclude_patterns.
> - * Note that if all hunks were rejected, the patched result will look just
> - * like the target file, unmodified.
> - * If @a reject_tempfiles is not NULL, return in @a *reject_tempfiles
> - * a mapping {target path -> path to temporary file containing rejected hunks}
> - * Both @a *patched_tempfiles and @a *reject_tempfiles are allocated in
> - * @a result_pool, and the key (target path) used is the path as parsed
> - * from the patch, but in canonicalized form. The value (path to temporary
> - * file) is an absolute path, also in canonicalized form.
> - * The temporary files are closed, and it is the caller's responsibility
> - * to remove them when they are no longer needed.
> - * Using @a patched_tempfiles and @a reject_tempfiles in combination with
> - * @a dry_run = TRUE makes it possible to generate a preview of the result
> - * of the patching process, e.g. for display purposes, without actually
> - * modifying the working copy.
> - *
> * If @a ignore_whitespaces is TRUE, allow patches to be applied if they
> * only differ from the target by whitespaces.
> *
> + * If @a remove_tempfiles is TRUE, the temporary patch and reject files will
> + * be removed upon pool cleanup, otherwise, the caller should take ownership
Pool cleanup - which pool?
> + * of these files.
> + *
> + * If @a patch_func is non-NULL, invoke @a patch_func with @a patch_baton
> + * for each patch target processed.
Wasn't the idea that the caller could choose whether the patched result
should be written straight to the target file or to somewhere else? I'm
not sure but I thought specifying @a patched_tempfiles != NULL caused
that to happen. Is that right and if so does the callback support that
feature?
> * If @a ctx->notify_func2 is non-NULL, invoke @a ctx->notify_func2 with
> * @a ctx->notify_baton2 as patching progresses.
> *
> @@ -4907,9 +4914,10 @@ svn_client_patch(const char *abs_patch_p
> svn_boolean_t reverse,
> const apr_array_header_t *include_patterns,
> const apr_array_header_t *exclude_patterns,
> - apr_hash_t **patched_tempfiles,
> - apr_hash_t **reject_tempfiles,
> svn_boolean_t ignore_whitespaces,
> + svn_boolean_t remove_tempfiles,
> + svn_client_patch_func_t patch_func,
> + void *patch_baton,
> svn_client_ctx_t *ctx,
> apr_pool_t *result_pool,
> apr_pool_t *scratch_pool);
- Julian