svn_client_status6() and svn_client_patch_func_t doc strings

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

svn_client_status6() and svn_client_patch_func_t doc strings

Julian Foad-5
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
Reply | Threaded
Open this post in threaded view
|

Re: svn_client_status6() and svn_client_patch_func_t doc strings

Troy Curtis Jr


On Thu, Dec 7, 2017 at 11:03 AM Julian Foad <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: svn_client_status6() and svn_client_patch_func_t doc strings

Julian Foad-5
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
Reply | Threaded
Open this post in threaded view
|

Re: svn_client_status6() and svn_client_patch_func_t doc strings

Troy Curtis Jr


On Fri, Dec 8, 2017, 3:49 AM Julian Foad <[hidden email]> 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