You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Stefan Sperling <st...@elego.de> on 2008/07/26 16:03:46 UTC
[PATCH] remove unused parameter from
libsvn_client/copy.c:setup_copy()
Good news everyone,
libsvn_subr/copy.c:setup_copy() takes a parameter named 'force'
which was used in 1.4.x but is not used anymore in 1.5.x and
trunk.
svn_client_move5 still exposes this parameter in public API.
The API docs in trunk say the following and I think they're wrong
because, well, force is ignored:
* - If one of @a src_paths contains locally modified and/or unversioned
* items and @a force is not set, the move will fail. If @a force is set
* such items will be removed.
The above should probably be replaced by a description of the
current behaviour (which I have not had time to figure out yet).
Rev'ing svn_client_move5 for this is overkill.
I'd suggest adding a comment to the docs of svn_client_move5 stating
that if the function is rev'd again for any reason, the force parameter
could/should be dropped.
For starters, here's a patch that removes the force parameter
from setup_copy. OK to commit?
[[[
* subversion/libsvn_client/copy.c
(setup_copy): Drop unused force parameter.
(svn_client_copy4, svn_client_move5, svn_client_move): Adjust
calls to setup_copy accordingly.
]]]
Index: subversion/libsvn_client/copy.c
===================================================================
--- subversion/libsvn_client/copy.c (revision 32219)
+++ subversion/libsvn_client/copy.c (working copy)
@@ -1708,7 +1708,6 @@ setup_copy(svn_commit_info_t **commit_info_p,
const apr_array_header_t *sources,
const char *dst_path_in,
svn_boolean_t is_move,
- svn_boolean_t force,
svn_boolean_t make_parents,
const apr_hash_t *revprop_table,
svn_client_ctx_t *ctx,
@@ -1988,7 +1987,6 @@ svn_client_copy4(svn_commit_info_t **commit_info_p
err = setup_copy(&commit_info,
sources, dst_path,
FALSE /* is_move */,
- TRUE /* force, set to avoid deletion check */,
make_parents,
revprop_table,
ctx,
@@ -2015,7 +2013,6 @@ svn_client_copy4(svn_commit_info_t **commit_info_p
sources,
svn_path_join(dst_path, src_basename, subpool),
FALSE /* is_move */,
- TRUE /* force, set to avoid deletion check */,
make_parents,
revprop_table,
ctx,
@@ -2147,7 +2144,6 @@ svn_client_move5(svn_commit_info_t **commit_info_p
err = setup_copy(&commit_info, sources, dst_path,
TRUE /* is_move */,
- force,
make_parents,
revprop_table,
ctx,
@@ -2170,7 +2166,6 @@ svn_client_move5(svn_commit_info_t **commit_info_p
err = setup_copy(&commit_info, sources,
svn_path_join(dst_path, src_basename, pool),
TRUE /* is_move */,
- force,
make_parents,
revprop_table,
ctx,
@@ -2292,7 +2287,6 @@ svn_client_move(svn_client_commit_info_t **commit_
err = setup_copy(&commit_info,
sources, dst_path,
TRUE /* is_move */,
- force,
FALSE /* make_parents */,
NULL,
ctx,
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] remove unused parameter from
libsvn_client/copy.c:setup_copy()
Posted by Julian Foad <ju...@btopenworld.com>.
On Sun, 2008-07-27 at 12:20 +0200, Stefan Sperling wrote:
> On Sat, Jul 26, 2008 at 11:49:00PM +0100, Julian Foad wrote:
> > > For starters, here's a patch that removes the force parameter
> > > from setup_copy. OK to commit?
> >
> > Not sure.
> >
> > Strictly speaking, we should preserve the behaviour options for the
> > existing APIs. The previous behaviours were well documented and thus
> > something that third-party tools may well have been relying on. While we
> > could argue that we have just "improved" the behaviour, I am not sure
> > that we always pay enough attention to meeting our
> > backward-compatibility guarantees.
>
> You're right, the behaviour of the affected functions was
> retroactively altered.
>
> But does "behaviour-compatible" equal "binary-compatible"?
Awaiting other people's opinions. My own opinion on what we should
require can often be too strict.
> > To be on the safe side I would say we have introduced a regression in
> > svn_client_copy4/3/2/1() and should fix it. This would probably involve
> > a three-way option: options "fail" and "delete" for the old APIs, and
> > option "move" for the new svn_client_copy5() API.
>
> OK, so you want setup_copy() to be smart enough to tell from which
> API rev it was called, and behave as advertised by that API revision,
> instead of always behaving as advertised by the latest API revision.
Yes.
> Fine by me, but I'd like to hear other peoples' opinions on this, too.
Me too.
> > Can you see if it would be much work to do that?
>
> I don't know. I have not looked at the code in detail, I was just
> poking around because of a different problem and noticed that force
> was unused. I don't have time to look into this in detail right now.
No problem.
> So, to summarise, we're seeing two issues here:
>
> * The API docs for svn_client_copy5 are flat out wrong with respect
> to the force parameter.
> * The behaviour of svn_client_copy4/3/2/1 has been retroactively
> changed.
>
> Right?
Agreed, given that you mean "_move" not "_copy". And
* The behaviour of svn_client_copy4() should be documented with
respect to unversioned items and local mods. The "copy" API has never
mentioned this situation whereas the "move" API always has.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] remove unused parameter from
libsvn_client/copy.c:setup_copy()
Posted by Stefan Sperling <st...@elego.de>.
On Sat, Jul 26, 2008 at 11:49:00PM +0100, Julian Foad wrote:
> > For starters, here's a patch that removes the force parameter
> > from setup_copy. OK to commit?
>
> Not sure.
>
> Strictly speaking, we should preserve the behaviour options for the
> existing APIs. The previous behaviours were well documented and thus
> something that third-party tools may well have been relying on. While we
> could argue that we have just "improved" the behaviour, I am not sure
> that we always pay enough attention to meeting our
> backward-compatibility guarantees.
You're right, the behaviour of the affected functions was
retroactively altered.
But does "behaviour-compatible" equal "binary-compatible"?
> To be on the safe side I would say we have introduced a regression in
> svn_client_copy4/3/2/1() and should fix it. This would probably involve
> a three-way option: options "fail" and "delete" for the old APIs, and
> option "move" for the new svn_client_copy5() API.
OK, so you want setup_copy() to be smart enough to tell from which
API rev it was called, and behave as advertised by that API revision,
instead of always behaving as advertised by the latest API revision.
Fine by me, but I'd like to hear other peoples' opinions on this, too.
> Can you see if it would be much work to do that?
I don't know. I have not looked at the code in detail, I was just
poking around because of a different problem and noticed that force
was unused. I don't have time to look into this in detail right now.
So, to summarise, we're seeing two issues here:
* The API docs for svn_client_copy5 are flat out wrong with respect
to the force parameter.
* The behaviour of svn_client_copy4/3/2/1 has been retroactively
changed.
Right?
Stefan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] remove unused parameter from
libsvn_client/copy.c:setup_copy()
Posted by Julian Foad <ju...@btopenworld.com>.
On Sat, 2008-07-26 at 18:03 +0200, Stefan Sperling wrote:
> Good news everyone,
>
> libsvn_subr/copy.c:setup_copy() takes a parameter named 'force'
> which was used in 1.4.x but is not used anymore in 1.5.x and
> trunk.
Yay!
It looks like the behaviour change to stop using it was intentional, per
r21347, "Don't require --force to move dirs with unversioned files or
local mods."
> svn_client_move5 still exposes this parameter in public API.
> The API docs in trunk say the following and I think they're wrong
> because, well, force is ignored:
>
> * - If one of @a src_paths contains locally modified and/or unversioned
> * items and @a force is not set, the move will fail. If @a force is set
> * such items will be removed.
>
> The above should probably be replaced by a description of the
> current behaviour (which I have not had time to figure out yet).
I presume it is intended to move the unversioned files and the local
mods. It would be quite horrible if it were now to delete them.
> Rev'ing svn_client_move5 for this is overkill.
> I'd suggest adding a comment to the docs of svn_client_move5 stating
> that if the function is rev'd again for any reason, the force parameter
> could/should be dropped.
+1.
> For starters, here's a patch that removes the force parameter
> from setup_copy. OK to commit?
Not sure.
Strictly speaking, we should preserve the behaviour options for the
existing APIs. The previous behaviours were well documented and thus
something that third-party tools may well have been relying on. While we
could argue that we have just "improved" the behaviour, I am not sure
that we always pay enough attention to meeting our
backward-compatibility guarantees.
To be on the safe side I would say we have introduced a regression in
svn_client_copy4/3/2/1() and should fix it. This would probably involve
a three-way option: options "fail" and "delete" for the old APIs, and
option "move" for the new svn_client_copy5() API.
Can you see if it would be much work to do that?
Contrary opinions welcome.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org