On Tue, May 4, 2010 at 12:36 PM, Julian Foad <julian.foad_at_wandisco.com>wrote:
> On Tue, 2010-05-04, hwright_at_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
Received on 2010-05-04 23:24:06 CEST