On Tue, May 4, 2010 at 12:45 PM, Stefan Sperling <stsp_at_elego.de> wrote:
> On Tue, May 04, 2010 at 09:45:44AM -0000, 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
> >
> > 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
Received on 2010-05-04 17:55:39 CEST