You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Julian Foad <ju...@wandisco.com> on 2010/05/04 10:36:49 UTC
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
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 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