Hyrum K. Wright wrote:
> This patch comprises the first step in moving multiple files, namely
> revving the client library API, and modifying the client application to
> call this new function. More patches are on their way, but I figure it
> is better to submit small changes at a time in separate patches.
It is a good idea to send these patches for review in small stages. However,
we should not actually commit this patch as it is, because the result would be
an API whose doc-string is inconsistent with its implementation. (I suppose
you could make the API say "Exactly one source must be supplied", and then this
would be a self-contained, logical, self-consistent change, but this isn't a
sufficient reason on its own for revving the API.)
When a good part of the underlying implementation for it is ready, then we can
commit this, as the added functionality will be sufficient reason for revving
the API. Probably, by that time, you will have found some reason to change
this part slightly.
> [[[
> Rev svn_client_move3 to allow it to take multiple source paths. Modify
> the client application to call the new version.
I don't want "copy" to remain only able to handle a single file while "move"
gets the ability to handle multiple files. I feel it would be neater to make
the corresponding changes to "copy" at the same time as "move". However, that
could involve twice as much re-work after reviews, so perhaps it makes sense to
submit a patch for just "move" and then, when it is approved, update "copy" in
the same way.
> Note: The command line application still does argument count checking,
> so at this point there should not be more than one path in the src_paths
> array.
>
> * subversion/include/svn_client.h
> (svn_client_move4): Rev API to accept an array of source paths instead
> of just one. Adjust comments appropriately.
>
> * subversion/libsvn_client/copy.c
> (svn_client_move4): New function.
> (svn_client_move3): Instead of setting up the move directly, call
> svn_client_move4.
>
> * subversion/clients/cmdline/move-cmd.c
> (svn_cl__move): Build an array of the source paths. Call the new API.
> ]]]
>
>
> ------------------------------------------------------------------------
>
> Index: subversion/include/svn_client.h
> ===================================================================
> --- subversion/include/svn_client.h (revision 17395)
> +++ subversion/include/svn_client.h (working copy)
> @@ -1716,12 +1716,12 @@
>
>
> /**
> - * Move @a src_path to @a dst_path.
> + * Move all @a src_paths to @a dst_path.
> *
> - * @a src_path must be a file or directory under version control, or the
> - * URL of a versioned item in the repository.
> + * @a src_paths must all be files or directories under version control, or
> + * URLs of versioned items in the repository.
The English language isn't very good for saying things precisely. It's not
quite clear that the sources must all be of the same type. Just add "all be"
at the beginning of the second line:
* @a src_paths must all be files or directories under version control, or
* all be URLs of versioned items in the repository.
> *
> - * If @a src_path is a repository URL:
> + * If @a src_paths are repository URLs:
> *
> * - @a dst_path must also be a repository URL (existent or not).
Presumably, for multiple source paths, the destination must be a directory into
which the sources will be moved, whereas for a single item it can be the new
name for the item. (This part of the doc string, about "dst_path", is already
ambiguous; I've just posted a patch to clarify it.)
I think we might well need to use two separate functions, one for moving (and
possibly renaming) a single item, and one for moving multiple (one or more)
items without renaming them. Otherwise, if only one source item is specified,
how do we know whether the destination path or url represents the new _name_
for the item or the new _directory_ in which to place it?
Basically your code looks OK. As I said, it's not appropriate to commit it
yet, and the API probably needs some work. Please (all readers) have a look at
the thread "API proposal - issue 2188 - svn_client_copy/move" where I recently
proposed a new single-file copy/move API. Some aspects of the proposal may be
too complex to implement all at once; I'm not sure. Anyway I think the way
forward is to formulate a multi-file copy/move API based on the thinking in
that proposal, so that they have most semantics in common.
I'll look into the API definition; Hyrum, you can continue working on the
lower-level parts that will be needed, unless you feel that splitting the API
later into two different functions would invalidate much of your work.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Nov 17 02:16:30 2005