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...@btopenworld.com> on 2004/10/11 23:47:31 UTC
"move" shouldn't take a revision argument
Peter Hercek pointed out recently that "svn move" can take a revision argument. I see also that svn_client_move() does. That argument is meaningless. A WC move always acts on the working revision, and a repository move on the repository head revision. So...
"svn move" should stop accepting "-r".
"svn_client_move" should be deprecated and superceded by "svn_client_move2" which will be the same but without the revision parameter. The doc string of "svn_client_move" says:
* If @a src_path is a repository URL:
[...]
* - @a src_revision is used to choose the revision from which to copy
* the @a src_path.
[...]
* If @a src_path is a working copy path
[...]
* - @a src_revision, and @a ctx->log_msg_func/@a ctx->log_msg_baton are
* ignored.
but that is wrong and should be changed to match the implementation, which is (for any type of src_path):
* @a src_revision has no useful effect but must be of kind
* @c svn_opt_revision_unspecified or @c svn_opt_revision_head,
* otherwise error @c SVN_ERR_UNSUPPORTED_FEATURE is returned.
Does this all sound right?
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] "move" shouldn't take a revision argument
Posted by kf...@collab.net.
Julian Foad <ju...@btopenworld.com> writes:
> --- subversion/include/svn_client.h (revision 11328)
> +++ subversion/include/svn_client.h (working copy)
> @@ -1072,9 +1072,6 @@
> *
> * - @a dst_path must also be a repository URL (existent or not).
> *
> - * - @a src_revision is used to choose the revision from which to copy
> - * the @a src_path.
> - *
> * - the authentication baton in @a ctx and @a ctx->log_msg_func/@a
> * ctx->log_msg_baton are used to commit the move.
> *
> @@ -1085,8 +1082,7 @@
> *
> * - @a dst_path must also be a working copy path (existent or not).
> *
> - * - @a src_revision, and @a ctx->log_msg_func/@a ctx->log_msg_baton are
> - * ignored.
> + * - @a ctx->log_msg_func and @a ctx->log_msg_baton are ignored.
> *
> * - This is a scheduling operation. No changes will happen to the
> * repository until a commit occurs. This scheduling can be removed
> @@ -1110,6 +1106,22 @@
> * ### Is this really true? What about @c svn_wc_notify_commit_replaced? ###
> */
> svn_error_t *
> +svn_client_move2 (svn_client_commit_info_t **commit_info,
> + const char *src_path,
> + const char *dst_path,
> + svn_boolean_t force,
> + svn_client_ctx_t *ctx,
> + apr_pool_t *pool);
> +
I think you need to put
@since New in 1.2.
or something in svn_client_move2()'s doc string?
> +/**
> + * @deprecated Provided for backward compatibility with the 1.1.0 API.
> + *
> + * Similar to @c svn_client_move2, but an extra argument @a src_revision
> + * must be passed. This has no effect, but must be of kind
> + * @c svn_opt_revision_unspecified or @c svn_opt_revision_head,
> + * otherwise error @c SVN_ERR_UNSUPPORTED_FEATURE is returned.
> + */
> +svn_error_t *
> svn_client_move (svn_client_commit_info_t **commit_info,
> const char *src_path,
> const svn_opt_revision_t *src_revision,
Beautiful!
Everything else looks good to me. A quick tags-search confirms that
there are no other callers of svn_client_move() in the core code.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
[PATCH] "move" shouldn't take a revision argument
Posted by Julian Foad <ju...@btopenworld.com>.
Any comments on this patch to remove the unwanted "revision" argument from "svn move" and "svn_client_move"?
- Julian
Re: "move" shouldn't take a revision argument
Posted by Julian Foad <ju...@btopenworld.com>.
kfogel@collab.net wrote:
> Julian Foad <ju...@btopenworld.com> writes:
[...]
>> * @a src_revision has no useful effect but must be of kind
>> * @c svn_opt_revision_unspecified or @c svn_opt_revision_head,
>> * otherwise error @c SVN_ERR_UNSUPPORTED_FEATURE is returned.
>>
>>Does this all sound right?
>
> Yep, sounds sane to me.
I'll see about making a patch.
> (What does the revision param do in the code right now, though? Have
> you traced it through?)
I have traced it. It is not used at all; it is only checked to ensure that it is @c svn_opt_revision_unspecified or @c svn_opt_revision_head. Perhaps my suggested doc string is still not clear enough. I'll remove the word "useful" from the doc string and leave "has no effect but must be of kind ..."
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: "move" shouldn't take a revision argument
Posted by kf...@collab.net.
Julian Foad <ju...@btopenworld.com> writes:
> Peter Hercek pointed out recently that "svn move" can take a
> revision argument. I see also that svn_client_move() does. That
> argument is meaningless. A WC move always acts on the working
> revision, and a repository move on the repository head revision.
> So...
>
> "svn move" should stop accepting "-r".
>
> "svn_client_move" should be deprecated and superceded by
> "svn_client_move2" which will be the same but without the revision
> parameter. The doc string of "svn_client_move" says:
>
> * If @a src_path is a repository URL:
> [...]
> * - @a src_revision is used to choose the revision from which to copy
> * the @a src_path.
> [...]
> * If @a src_path is a working copy path
> [...]
> * - @a src_revision, and @a ctx->log_msg_func/@a ctx->log_msg_baton are
> * ignored.
>
> but that is wrong and should be changed to match the implementation,
> which is (for any type of src_path):
>
> * @a src_revision has no useful effect but must be of kind
> * @c svn_opt_revision_unspecified or @c svn_opt_revision_head,
> * otherwise error @c SVN_ERR_UNSUPPORTED_FEATURE is returned.
>
> Does this all sound right?
Yep, sounds sane to me.
(What does the revision param do in the code right now, though? Have
you traced it through?)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: "move" shouldn't take a revision argument
Posted by kf...@collab.net.
Ben Reser <be...@reser.org> writes:
> Ahh okay. So leave the current behavior in the command line client,
> mark it as depricated and remove it at 2.0.
I replied:
> Yep.
I originally proposed just ignoring the revision argument entirely
now, so that it wouldn't produce an error no matter what you passed.
On further thought, that's a terrible idea, and I never said it :-)
The current behavior is fine (and the error case *should* be an
error); we should just deprecate the -r option to mv in 2.0 as you
said.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: "move" shouldn't take a revision argument
Posted by kf...@collab.net.
Ben Reser <be...@reser.org> writes:
> Ahh okay. So leave the current behavior in the command line client,
> mark it as depricated and remove it at 2.0.
Yep.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: "move" shouldn't take a revision argument
Posted by Ben Reser <be...@reser.org>.
On Tue, Oct 12, 2004 at 10:56:42PM -0500, kfogel@collab.net wrote:
> Ben Reser <be...@reser.org> writes:
> > What are you talking about:
> >
> > $ svn mv -r 1234 HACKING HACKING1234
> > svn: Cannot specify revisions with move operations
> >
> > The API takes an option. But we don't support it in our command line
> > client. I can't imagine this change breaking anything that wasn't
> > already broken.
>
> Did you try "-r HEAD"?
Ahh okay. So leave the current behavior in the command line client,
mark it as depricated and remove it at 2.0.
--
Ben Reser <be...@reser.org>
http://ben.reser.org
"Conscience is the inner voice which warns us somebody may be looking."
- H.L. Mencken
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: "move" shouldn't take a revision argument
Posted by kf...@collab.net.
Ben Reser <be...@reser.org> writes:
> What are you talking about:
>
> $ svn mv -r 1234 HACKING HACKING1234
> svn: Cannot specify revisions with move operations
>
> The API takes an option. But we don't support it in our command line
> client. I can't imagine this change breaking anything that wasn't
> already broken.
Did you try "-r HEAD"?
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: "move" shouldn't take a revision argument
Posted by Ben Reser <be...@reser.org>.
On Tue, Oct 12, 2004 at 07:12:21PM +0200, Peter N. Lundblad wrote:
> On Mon, 11 Oct 2004, Ben Reser wrote:
>
> > On Tue, Oct 12, 2004 at 12:47:31AM +0100, Julian Foad wrote:
> > > "svn move" should stop accepting "-r".
> > >
> [...]
>
> > +1
> >
> Can we really remove an option without breaking compatibility?
What are you talking about:
$ svn mv -r 1234 HACKING HACKING1234
svn: Cannot specify revisions with move operations
The API takes an option. But we don't support it in our command line
client. I can't imagine this change breaking anything that wasn't
already broken.
--
Ben Reser <be...@reser.org>
http://ben.reser.org
"Conscience is the inner voice which warns us somebody may be looking."
- H.L. Mencken
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
[PATCH] "move" shouldn't take a revision argument
Posted by Julian Foad <ju...@btopenworld.com>.
Julian Foad wrote:
> I'll do something about deprecating it.
The attached patch deprecates svn_client_move because of its "src_revision"
argument. (A future patch should deprecate the command-line option "-r" with
"svn move".)
I don't think it is right for us to deprecate an API and then go on using it,
so I duplicate the "if (!HEAD) return ERROR;" logic in the caller to preserve
the current command-line behaviour until we can lose the "-r" option in version 2.
I'll commit this if there are no adverse comments.
- Julian
Re: "move" shouldn't take a revision argument
Posted by kf...@collab.net.
Julian Foad <ju...@btopenworld.com> writes:
> > The solution is fairly simple though: continue to accept the option,
> > and just ignore it, since that's (effectively) what we were doing
> > anyway. That is: accept the option, completely ignore it in
> > move-cmd.c, and file an issue to remove the option in 2.0.
>
> I would not like to change the error into a silently doing the wrong
> thing. For new users, that would be really horrible.
You're right, forget I said it.
> I'll do something about deprecating it.
Thanks!
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: "move" shouldn't take a revision argument
Posted by Julian Foad <ju...@btopenworld.com>.
kfogel@collab.net wrote:
>>>>Can we really remove an option without breaking compatibility?
[...]
> The solution is fairly simple though: continue to accept the option,
> and just ignore it, since that's (effectively) what we were doing
> anyway. That is: accept the option, completely ignore it in
> move-cmd.c, and file an issue to remove the option in 2.0.
I would not like to change the error into a silently doing the wrong
thing. For new users, that would be really horrible.
I'll do something about deprecating it.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: "move" shouldn't take a revision argument
Posted by kf...@collab.net.
Julian Foad <ju...@btopenworld.com> writes:
> >>Can we really remove an option without breaking compatibility?
> > Good point. Even though the option have no function, there might be
> > scripts using it, maybe by accident, and those will break if svn
> > doesn't accept it anymore. Maybe a deprecation warning for some
> > period of time would be the the right thing to do at the
> > moment. This will help weeding out the usage of the option with svn
> > move. Later it can be fully disabled.
>
> We can remove wrong or unintended behaviour at any time. If we
> think it is going to cause a significant amount of grief, then we
> should consider deprecating it first, but in this case I don't think
> it will cause a significant problem.
I agree that it probably won't cause many problems, but they're right
that removing the option *is* an interface change, and is not
acceptable according to the guidelines we've been using. If some
script out there currently passes '-rHEAD', then that script, which
formerly worked, would now break.
My apologies for not spotting this problem in my earlier review, btw.
The solution is fairly simple though: continue to accept the option,
and just ignore it, since that's (effectively) what we were doing
anyway. That is: accept the option, completely ignore it in
move-cmd.c, and file an issue to remove the option in 2.0.
Note that we don't have to preserve the error case behavior. If
someone has been passing '-rNUMBER', well, that never worked, so it's
not a disaster if it suddenly starts working now ("working" in the
sense of "does not error", not in the sense of "pays attention to
NUMBER", which of course it can never do).
Thoughts?
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: "move" shouldn't take a revision argument
Posted by Julian Foad <ju...@btopenworld.com>.
Øyvind A. Holm wrote:
> On 2004-10-12 19:12:21 Peter N. Lundblad wrote:
>>On Mon, 11 Oct 2004, Ben Reser wrote:
>>>On Tue, Oct 12, 2004 at 12:47:31AM +0100, Julian Foad wrote:
>>>>"svn move" should stop accepting "-r".
>>>
>>>+1
>>
>>Can we really remove an option without breaking compatibility?
>
> Good point. Even though the option have no function, there might be
> scripts using it, maybe by accident, and those will break if svn doesn't
> accept it anymore. Maybe a deprecation warning for some period of time
> would be the the right thing to do at the moment. This will help weeding
> out the usage of the option with svn move. Later it can be fully
> disabled.
We can remove wrong or unintended behaviour at any time. If we think it is going to cause a significant amount of grief, then we should consider deprecating it first, but in this case I don't think it will cause a significant problem.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: "move" shouldn't take a revision argument
Posted by "Øyvind A. Holm" <su...@sunbase.org>.
On 2004-10-12 19:12:21 Peter N. Lundblad wrote:
> On Mon, 11 Oct 2004, Ben Reser wrote:
> > On Tue, Oct 12, 2004 at 12:47:31AM +0100, Julian Foad wrote:
> > > "svn move" should stop accepting "-r".
> >
> > +1
>
> Can we really remove an option without breaking compatibility?
Good point. Even though the option have no function, there might be
scripts using it, maybe by accident, and those will break if svn doesn't
accept it anymore. Maybe a deprecation warning for some period of time
would be the the right thing to do at the moment. This will help weeding
out the usage of the option with svn move. Later it can be fully
disabled.
- Øyvind
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: "move" shouldn't take a revision argument
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Mon, 11 Oct 2004, Ben Reser wrote:
> On Tue, Oct 12, 2004 at 12:47:31AM +0100, Julian Foad wrote:
> > "svn move" should stop accepting "-r".
> >
[...]
> +1
>
Can we really remove an option without breaking compatibility?
Regards,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: "move" shouldn't take a revision argument
Posted by Ben Reser <be...@reser.org>.
On Tue, Oct 12, 2004 at 12:47:31AM +0100, Julian Foad wrote:
> Peter Hercek pointed out recently that "svn move" can take a revision
> argument. I see also that svn_client_move() does. That argument is
> meaningless. A WC move always acts on the working revision, and a
> repository move on the repository head revision. So...
>
> "svn move" should stop accepting "-r".
>
> "svn_client_move" should be deprecated and superceded by "svn_client_move2"
> which will be the same but without the revision parameter. The doc string
> of "svn_client_move" says:
>
> * If @a src_path is a repository URL:
> [...]
> * - @a src_revision is used to choose the revision from which to copy
> * the @a src_path.
> [...]
> * If @a src_path is a working copy path
> [...]
> * - @a src_revision, and @a ctx->log_msg_func/@a ctx->log_msg_baton are
> * ignored.
>
> but that is wrong and should be changed to match the implementation, which
> is (for any type of src_path):
>
> * @a src_revision has no useful effect but must be of kind
> * @c svn_opt_revision_unspecified or @c svn_opt_revision_head,
> * otherwise error @c SVN_ERR_UNSUPPORTED_FEATURE is returned.
>
> Does this all sound right?
+1
--
Ben Reser <be...@reser.org>
http://ben.reser.org
"Conscience is the inner voice which warns us somebody may be looking."
- H.L. Mencken
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org