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

[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