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...@apache.org> on 2017/12/07 17:03:04 UTC
svn_client_status6() and svn_client_patch_func_t doc strings
Queries/suggestions on svn_client_status6() and svn_client_patch_func_t...
[[[
Index: subversion/include/svn_client.h
===================================================================
--- subversion/include/svn_client.h (revision 1817399)
+++ subversion/include/svn_client.h (working copy)
@@ -2514,9 +2514,9 @@ typedef svn_error_t *(*svn_client_status
* *result_rev is not meaningful unless @a check_out_of_date is
* set).
*
- * - If @a check_working_copy is not set, do not scan the working
- * copy for local modifications. This parameter will be ignored
- * unless @a check_out_of_date is set. When set, the status
+ * - If @a check_working_copy is false, do not scan the working
+ * copy for local modifications. This parameter will be assumed true
+ * unless @a check_out_of_date is set. When false, the status
* report will not contain any information about local changes in
* the working copy; this includes local deletions and
* replacements.
@@ -7456,18 +7456,24 @@ svn_client_min_max_revisions(svn_revnum_
*/
/**
- * The callback invoked by svn_client_patch() before attempting to patch
- * the target file at @a canon_path_from_patchfile (the path as parsed from
- * the patch file, but in canonicalized form). The callback can set
- * @a *filtered to @c TRUE to prevent the file from being patched, or else
+ * The callback invoked by svn_client_patch() when patching each target file.
+ *
+ * Called after putting the patch result and any reject in temporary files,
+ * before moving those files to the real location to complete the patching.
+ *
+ * The callback can set @a *filtered to @c TRUE to prevent moving the
+ * temporary files to the real location to complete the patching, or else
* must set it to @c FALSE.
*
+ * @a canon_path_from_patchfile is the path as parsed from the patch file,
+ * but in canonicalized form.
+ *
* The callback is also provided with @a patch_abspath, the path of a
* temporary file containing the patched result, and with @a reject_abspath,
* the path to a temporary file containing the diff text of any hunks
* which were rejected during patching.
*
- * Because the callback is invoked before the patching attempt is made,
+ * ### ? Because the callback is invoked before the patching attempt is made,
* there is no guarantee that the target file will actually be patched
* successfully. Client implementations must pay attention to notification
* feedback provided by svn_client_patch() to find out which paths were
]]]
Thoughts?
- Julian
Re: svn_client_status6() and svn_client_patch_func_t doc strings
Posted by Troy Curtis Jr <tr...@gmail.com>.
On Fri, Dec 8, 2017, 3:49 AM Julian Foad <ju...@apache.org> wrote:
> Troy Curtis Jr wrote:
> > Julian Foad wrote:
> > [[[
> > Index: subversion/include/svn_client.h
> > ===================================================================
> > --- subversion/include/svn_client.h (revision 1817399)
> > +++ subversion/include/svn_client.h ( will
>
...
> > - * Because the callback is invoked before the patching attempt is
> made,
> > + * ### ? Because the callback is invoked before the patching
> > attempt is made,
> > * there is no guarantee that the target file will actually be
> patched
> > * successfully. Client implementations must pay attention to
> > notification
> > * feedback provided by svn_client_patch() to find out which paths
> > were
> > ]]]
> >
> >
> > How about:
> >
> > The callback is invoked before the patching attempt is made and
> > therefore there is
> > no guarantee that the target file will actually be patched successfully.
>
> Sorry, I didn't explain why I queried this paragraph. To me, "the
> patching attempt" began before the callback, which is the main thing I
> was clarifying in the first hunk above, so this paragraph is inaccurate.
> It is not clear to me whether the client really needs to pay attention
> to notifications, or if the callback can just look at whether any
> non-empty "reject" file was created, to know whether there was a
> problem. If there was no problem at this stage, then the patching is
> more or less guaranteed to be successful -- there are very few reasons
> why the final "move into place" step might fail.
>
Ah, I should have known it would not not be that simple!
I haven't looked at the underlying code, but based on your description it
seems like the warning is still reasonable. To me it more clearly indicates
the purpose of the callback: to hook in and modify the patching process.
There may not be many current reasons it could fail, but subversion apis
are very long lived, and additional cases may be found later on to bail
after this point.
You are right though, the patching process has already started, so perhaps
it should say something more like "When this callback is invoked, the
patching process is not yet complete, and could still fail for other
reasons."
Even if it is currently unlikely to fail, it limits the intended role for
this particular callback, leaving future changes to the implementation more
freedom to work within established bounds.
Troy
>
> - Julian
>
Re: svn_client_status6() and svn_client_patch_func_t doc strings
Posted by Julian Foad <ju...@apache.org>.
Troy Curtis Jr wrote:
> Julian Foad wrote:
> [[[
> Index: subversion/include/svn_client.h
> ===================================================================
> --- subversion/include/svn_client.h (revision 1817399)
> +++ subversion/include/svn_client.h (working copy)
> @@ -2514,9 +2514,9 @@ typedef svn_error_t *(*svn_client_status
> * *result_rev is not meaningful unless @a check_out_of_date is
> * set).
> *
> - * - If @a check_working_copy is not set, do not scan the working
> - * copy for local modifications. This parameter will be ignored
> - * unless @a check_out_of_date is set. When set, the status
> + * - If @a check_working_copy is false, do not scan the working
> + * copy for local modifications. This parameter will be
> assumed true
> + * unless @a check_out_of_date is set. When false, the status
> * report will not contain any information about local changes in
> * the working copy; this includes local deletions and
> * replacements.
> @@ -7456,18 +7456,24 @@ svn_client_min_max_revisions(svn_revnum_
> */
>
> /**
> - * The callback invoked by svn_client_patch() before attempting to
> patch
> - * the target file at @a canon_path_from_patchfile (the path as
> parsed from
> - * the patch file, but in canonicalized form). The callback can set
> - * @a *filtered to @c TRUE to prevent the file from being patched,
> or else
> + * The callback invoked by svn_client_patch() when patching each
> target file.
> + *
> + * Called after putting the patch result and any reject in
> temporary files,
> + * before moving those files to the real location to complete the
> patching.
> + *
> + * The callback can set @a *filtered to @c TRUE to prevent moving the
> + * temporary files to the real location to complete the patching,
> or else
> * must set it to @c FALSE.
> *
> + * @a canon_path_from_patchfile is the path as parsed from the
> patch file,
> + * but in canonicalized form.
> + *
>
> Definitely much clearer language!
>
> - * Because the callback is invoked before the patching attempt is made,
> + * ### ? Because the callback is invoked before the patching
> attempt is made,
> * there is no guarantee that the target file will actually be patched
> * successfully. Client implementations must pay attention to
> notification
> * feedback provided by svn_client_patch() to find out which paths
> were
> ]]]
>
>
> How about:
>
> The callback is invoked before the patching attempt is made and
> therefore there is
> no guarantee that the target file will actually be patched successfully.
Sorry, I didn't explain why I queried this paragraph. To me, "the
patching attempt" began before the callback, which is the main thing I
was clarifying in the first hunk above, so this paragraph is inaccurate.
It is not clear to me whether the client really needs to pay attention
to notifications, or if the callback can just look at whether any
non-empty "reject" file was created, to know whether there was a
problem. If there was no problem at this stage, then the patching is
more or less guaranteed to be successful -- there are very few reasons
why the final "move into place" step might fail.
- Julian
Re: svn_client_status6() and svn_client_patch_func_t doc strings
Posted by Troy Curtis Jr <tr...@gmail.com>.
On Thu, Dec 7, 2017 at 11:03 AM Julian Foad <ju...@apache.org> wrote:
> Queries/suggestions on svn_client_status6() and svn_client_patch_func_t...
>
> [[[
> Index: subversion/include/svn_client.h
> ===================================================================
> --- subversion/include/svn_client.h (revision 1817399)
> +++ subversion/include/svn_client.h (working copy)
> @@ -2514,9 +2514,9 @@ typedef svn_error_t *(*svn_client_status
> * *result_rev is not meaningful unless @a check_out_of_date is
> * set).
> *
> - * - If @a check_working_copy is not set, do not scan the working
> - * copy for local modifications. This parameter will be ignored
> - * unless @a check_out_of_date is set. When set, the status
> + * - If @a check_working_copy is false, do not scan the working
> + * copy for local modifications. This parameter will be assumed true
> + * unless @a check_out_of_date is set. When false, the status
> * report will not contain any information about local changes in
> * the working copy; this includes local deletions and
> * replacements.
> @@ -7456,18 +7456,24 @@ svn_client_min_max_revisions(svn_revnum_
> */
>
> /**
> - * The callback invoked by svn_client_patch() before attempting to patch
> - * the target file at @a canon_path_from_patchfile (the path as parsed
> from
> - * the patch file, but in canonicalized form). The callback can set
> - * @a *filtered to @c TRUE to prevent the file from being patched, or else
> + * The callback invoked by svn_client_patch() when patching each target
> file.
> + *
> + * Called after putting the patch result and any reject in temporary
> files,
> + * before moving those files to the real location to complete the
> patching.
> + *
> + * The callback can set @a *filtered to @c TRUE to prevent moving the
> + * temporary files to the real location to complete the patching, or else
> * must set it to @c FALSE.
> *
> + * @a canon_path_from_patchfile is the path as parsed from the patch file,
> + * but in canonicalized form.
> + *
> * The callback is also provided with @a patch_abspath, the path of a
> * temporary file containing the patched result, and with @a
> reject_abspath,
> * the path to a temporary file containing the diff text of any hunks
> * which were rejected during patching.
> *
>
Definitely much clearer language!
> - * Because the callback is invoked before the patching attempt is made,
> + * ### ? Because the callback is invoked before the patching attempt is
> made,
> * there is no guarantee that the target file will actually be patched
> * successfully. Client implementations must pay attention to notification
> * feedback provided by svn_client_patch() to find out which paths were
> ]]]
>
>
How about:
The callback is invoked before the patching attempt is made and therefore
there is
no guarantee that the target file will actually be patched successfully.
Thoughts?
>
> - Julian
>
Troy