You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Branko Čibej <br...@apache.org> on 2016/04/17 18:47:35 UTC
Deprecation of svn_client_resolve() leaves loose ends
I've been looking at and fixing new compile-time warnings on trunk, and
noticed that the svn_client_resolve() function was deprecated in
r1730495 in favour of three new functions that are specialised for text,
property or tree conflict resolution.
This change causes a deprecation warning to be emitted in
svn_cl__resolved(); and I'm fairly sure we shouldn't be calling
deprecated functions in the command-line client. (The warning probably
gets emitted from JavaHL too, but I haven't checked that.)
This causes me to wonder if deprecating svn_client_resolve() is really
such a good idea. It forces API users to jump through hoops to discover
the conflict type(s) and then calling up to three functions to mark the
conflict(s) resolved instead of just one; effectively changing a
one-liner to an unwieldy bunch of code.
I suggest we keep svn_client_resolve() on the books and just point API
users to the more specialised variants in the docs.
-- Brane
P.S.: It would also be nice if developers compiled stuff in maintainer
mode and fixed new warnings before committing ... just sayin'.
Re: Deprecation of svn_client_resolve() leaves loose ends
Posted by Branko Čibej <br...@apache.org>.
On 17.04.2016 18:47, Branko Čibej wrote:
> (The warning probably gets emitted from JavaHL too, but I haven't checked that.)
Indeed it does pop up in JavaHL.
-- Brane
Re: Deprecation of svn_client_resolve() leaves loose ends
Posted by Branko Čibej <br...@apache.org>.
On 18.04.2016 10:44, Stefan Sperling wrote:
> On Mon, Apr 18, 2016 at 10:40:53AM +0200, Stefan Sperling wrote:
>> On Mon, Apr 18, 2016 at 10:33:43AM +0200, Branko Čibej wrote:
>>> On 18.04.2016 10:28, Stefan Sperling wrote:
>>>> On Sun, Apr 17, 2016 at 06:47:35PM +0200, Branko Čibej wrote:
>>>>> I've been looking at and fixing new compile-time warnings on trunk, and
>>>>> noticed that the svn_client_resolve() function was deprecated in
>>>>> r1730495 in favour of three new functions that are specialised for text,
>>>>> property or tree conflict resolution.
>>>>>
>>>>> This change causes a deprecation warning to be emitted in
>>>>> svn_cl__resolved(); and I'm fairly sure we shouldn't be calling
>>>>> deprecated functions in the command-line client. (The warning probably
>>>>> gets emitted from JavaHL too, but I haven't checked that.)
>>>>>
>>>>> This causes me to wonder if deprecating svn_client_resolve() is really
>>>>> such a good idea. It forces API users to jump through hoops to discover
>>>>> the conflict type(s) and then calling up to three functions to mark the
>>>>> conflict(s) resolved instead of just one; effectively changing a
>>>>> one-liner to an unwieldy bunch of code.
>>>> I agree that, for non-interactive clients, calling 'svn_client_resolve()'
>>>> is easier. Also, the new API has no equivalent for the 'depth' parameter of
>>>> svn_client_resolve().
>>> I find this just a bit confusing ... How can typing "svn resolved -R' at
>>> the command prompt be considered non-interactive?
>> How about this, then?
> I think I've found an even better place to put this information:
>
> Index: subversion/include/svn_client.h
> ===================================================================
> --- subversion/include/svn_client.h (revision 1739704)
> +++ subversion/include/svn_client.h (working copy)
> @@ -4956,7 +4956,11 @@ svn_client_resolved(const char *path,
> * - svn_wc_conflict_choose_unspecified
> * invoke @a ctx->conflict_func2 with @a ctx->conflict_baton2 to obtain
> * a resolution decision for each conflict. This can be used to
> - * implement interactive conflict resolution.
> + * implement interactive conflict resolution but is NOT RECOMMENDED for
> + * new code. To perform conflict resolution based on interactive user
> + * input on a per-conflict basis, use svn_client_conflict_text_resolve(),
> + * svn_client_conflict_prop_resolve(), and
> + * svn_client_conflict_tree_resolve() instead of svn_client_resolve().
> *
> * #svn_wc_conflict_choose_theirs_conflict and
> * #svn_wc_conflict_choose_mine_conflict are not legal for binary
> @@ -4978,11 +4982,7 @@ svn_client_resolved(const char *path,
> * @a path in order to be able to resolve tree-conflicts on @a path.
> *
> * @since New in 1.5.
> - * @deprecated Provided for backward compatibility with the 1.9 API.
> - * Use svn_client_conflict_text_resolve() , svn_client_conflict_prop_resolve(),
> - * and svn_client_conflict_tree_resolve() instead.
> */
> -SVN_DEPRECATED
> svn_error_t *
> svn_client_resolve(const char *path,
> svn_depth_t depth,
Looks good, +1.
-- Brane
Re: Deprecation of svn_client_resolve() leaves loose ends
Posted by Stefan Sperling <st...@elego.de>.
On Mon, Apr 18, 2016 at 10:40:53AM +0200, Stefan Sperling wrote:
> On Mon, Apr 18, 2016 at 10:33:43AM +0200, Branko Čibej wrote:
> > On 18.04.2016 10:28, Stefan Sperling wrote:
> > > On Sun, Apr 17, 2016 at 06:47:35PM +0200, Branko Čibej wrote:
> > >> I've been looking at and fixing new compile-time warnings on trunk, and
> > >> noticed that the svn_client_resolve() function was deprecated in
> > >> r1730495 in favour of three new functions that are specialised for text,
> > >> property or tree conflict resolution.
> > >>
> > >> This change causes a deprecation warning to be emitted in
> > >> svn_cl__resolved(); and I'm fairly sure we shouldn't be calling
> > >> deprecated functions in the command-line client. (The warning probably
> > >> gets emitted from JavaHL too, but I haven't checked that.)
> > >>
> > >> This causes me to wonder if deprecating svn_client_resolve() is really
> > >> such a good idea. It forces API users to jump through hoops to discover
> > >> the conflict type(s) and then calling up to three functions to mark the
> > >> conflict(s) resolved instead of just one; effectively changing a
> > >> one-liner to an unwieldy bunch of code.
> > > I agree that, for non-interactive clients, calling 'svn_client_resolve()'
> > > is easier. Also, the new API has no equivalent for the 'depth' parameter of
> > > svn_client_resolve().
> >
> > I find this just a bit confusing ... How can typing "svn resolved -R' at
> > the command prompt be considered non-interactive?
>
> How about this, then?
I think I've found an even better place to put this information:
Index: subversion/include/svn_client.h
===================================================================
--- subversion/include/svn_client.h (revision 1739704)
+++ subversion/include/svn_client.h (working copy)
@@ -4956,7 +4956,11 @@ svn_client_resolved(const char *path,
* - svn_wc_conflict_choose_unspecified
* invoke @a ctx->conflict_func2 with @a ctx->conflict_baton2 to obtain
* a resolution decision for each conflict. This can be used to
- * implement interactive conflict resolution.
+ * implement interactive conflict resolution but is NOT RECOMMENDED for
+ * new code. To perform conflict resolution based on interactive user
+ * input on a per-conflict basis, use svn_client_conflict_text_resolve(),
+ * svn_client_conflict_prop_resolve(), and
+ * svn_client_conflict_tree_resolve() instead of svn_client_resolve().
*
* #svn_wc_conflict_choose_theirs_conflict and
* #svn_wc_conflict_choose_mine_conflict are not legal for binary
@@ -4978,11 +4982,7 @@ svn_client_resolved(const char *path,
* @a path in order to be able to resolve tree-conflicts on @a path.
*
* @since New in 1.5.
- * @deprecated Provided for backward compatibility with the 1.9 API.
- * Use svn_client_conflict_text_resolve() , svn_client_conflict_prop_resolve(),
- * and svn_client_conflict_tree_resolve() instead.
*/
-SVN_DEPRECATED
svn_error_t *
svn_client_resolve(const char *path,
svn_depth_t depth,
Re: Deprecation of svn_client_resolve() leaves loose ends
Posted by Stefan Sperling <st...@elego.de>.
On Mon, Apr 18, 2016 at 10:33:43AM +0200, Branko Čibej wrote:
> On 18.04.2016 10:28, Stefan Sperling wrote:
> > On Sun, Apr 17, 2016 at 06:47:35PM +0200, Branko Čibej wrote:
> >> I've been looking at and fixing new compile-time warnings on trunk, and
> >> noticed that the svn_client_resolve() function was deprecated in
> >> r1730495 in favour of three new functions that are specialised for text,
> >> property or tree conflict resolution.
> >>
> >> This change causes a deprecation warning to be emitted in
> >> svn_cl__resolved(); and I'm fairly sure we shouldn't be calling
> >> deprecated functions in the command-line client. (The warning probably
> >> gets emitted from JavaHL too, but I haven't checked that.)
> >>
> >> This causes me to wonder if deprecating svn_client_resolve() is really
> >> such a good idea. It forces API users to jump through hoops to discover
> >> the conflict type(s) and then calling up to three functions to mark the
> >> conflict(s) resolved instead of just one; effectively changing a
> >> one-liner to an unwieldy bunch of code.
> > I agree that, for non-interactive clients, calling 'svn_client_resolve()'
> > is easier. Also, the new API has no equivalent for the 'depth' parameter of
> > svn_client_resolve().
>
> I find this just a bit confusing ... How can typing "svn resolved -R' at
> the command prompt be considered non-interactive?
How about this, then?
Index: subversion/include/svn_client.h
===================================================================
--- subversion/include/svn_client.h (revision 1739704)
+++ subversion/include/svn_client.h (working copy)
@@ -4977,12 +4977,13 @@ svn_client_resolved(const char *path,
* Note that this operation will try to lock the parent directory of
* @a path in order to be able to resolve tree-conflicts on @a path.
*
+ * To perform conflict resolution based on interactive user input
+ * on a per-conflict basis, use svn_client_conflict_text_resolve(),
+ * svn_client_conflict_prop_resolve(), and svn_client_conflict_tree_resolve()
+ * instead.
+ *
* @since New in 1.5.
- * @deprecated Provided for backward compatibility with the 1.9 API.
- * Use svn_client_conflict_text_resolve() , svn_client_conflict_prop_resolve(),
- * and svn_client_conflict_tree_resolve() instead.
*/
-SVN_DEPRECATED
svn_error_t *
svn_client_resolve(const char *path,
svn_depth_t depth,
Re: Deprecation of svn_client_resolve() leaves loose ends
Posted by Branko Čibej <br...@apache.org>.
On 18.04.2016 10:28, Stefan Sperling wrote:
> On Sun, Apr 17, 2016 at 06:47:35PM +0200, Branko Čibej wrote:
>> I've been looking at and fixing new compile-time warnings on trunk, and
>> noticed that the svn_client_resolve() function was deprecated in
>> r1730495 in favour of three new functions that are specialised for text,
>> property or tree conflict resolution.
>>
>> This change causes a deprecation warning to be emitted in
>> svn_cl__resolved(); and I'm fairly sure we shouldn't be calling
>> deprecated functions in the command-line client. (The warning probably
>> gets emitted from JavaHL too, but I haven't checked that.)
>>
>> This causes me to wonder if deprecating svn_client_resolve() is really
>> such a good idea. It forces API users to jump through hoops to discover
>> the conflict type(s) and then calling up to three functions to mark the
>> conflict(s) resolved instead of just one; effectively changing a
>> one-liner to an unwieldy bunch of code.
> I agree that, for non-interactive clients, calling 'svn_client_resolve()'
> is easier. Also, the new API has no equivalent for the 'depth' parameter of
> svn_client_resolve().
I find this just a bit confusing ... How can typing "svn resolved -R' at
the command prompt be considered non-interactive?
-- Brane
Re: Deprecation of svn_client_resolve() leaves loose ends
Posted by Stefan Sperling <st...@elego.de>.
On Sun, Apr 17, 2016 at 06:47:35PM +0200, Branko Čibej wrote:
> I've been looking at and fixing new compile-time warnings on trunk, and
> noticed that the svn_client_resolve() function was deprecated in
> r1730495 in favour of three new functions that are specialised for text,
> property or tree conflict resolution.
>
> This change causes a deprecation warning to be emitted in
> svn_cl__resolved(); and I'm fairly sure we shouldn't be calling
> deprecated functions in the command-line client. (The warning probably
> gets emitted from JavaHL too, but I haven't checked that.)
>
> This causes me to wonder if deprecating svn_client_resolve() is really
> such a good idea. It forces API users to jump through hoops to discover
> the conflict type(s) and then calling up to three functions to mark the
> conflict(s) resolved instead of just one; effectively changing a
> one-liner to an unwieldy bunch of code.
I agree that, for non-interactive clients, calling 'svn_client_resolve()'
is easier. Also, the new API has no equivalent for the 'depth' parameter of
svn_client_resolve().
How about this?
Index: subversion/include/svn_client.h
===================================================================
--- subversion/include/svn_client.h (revision 1739704)
+++ subversion/include/svn_client.h (working copy)
@@ -4977,12 +4977,13 @@ svn_client_resolved(const char *path,
* Note that this operation will try to lock the parent directory of
* @a path in order to be able to resolve tree-conflicts on @a path.
*
+ * This function is recommended for non-interactive use only.
+ * If performing interactive conflict resolution based on user input, use
+ * svn_client_conflict_text_resolve() , svn_client_conflict_prop_resolve(),
+ * and svn_client_conflict_tree_resolve() instead.
+ *
* @since New in 1.5.
- * @deprecated Provided for backward compatibility with the 1.9 API.
- * Use svn_client_conflict_text_resolve() , svn_client_conflict_prop_resolve(),
- * and svn_client_conflict_tree_resolve() instead.
*/
-SVN_DEPRECATED
svn_error_t *
svn_client_resolve(const char *path,
svn_depth_t depth,
Re: Deprecation of svn_client_resolve() leaves loose ends
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Branko Čibej wrote on Sun, Apr 17, 2016 at 18:47:35 +0200:
> P.S.: It would also be nice if developers compiled stuff in maintainer
> mode and fixed new warnings before committing ... just sayin'.
That's exactly why we have the svn-warnings bot. Its build did trigger
the 'switch' warning you had just fixed elsewhere, but that didn't cause
the build status to become red/failure:
https://ci.apache.org/builders/svn-warnings/builds/1145/steps/Make/logs/stdio
So, I guess our custom "fail the build if stderr is non-empty" builder
setup code needs some attention:
https://svn.apache.org/repos/infra/infrastructure/buildbot/aegis/buildmaster/master1/projects/subversion.conf
[committers-only link, sorry]
Cheers,
Daniel
P.S. The existing bot uses gcc. We could add a clang warnings bot if we
wanted to, to improve coverage... (it just runs 'make all install'
without 'make test', so requires little resources on the buildslave)