[PATCH] Move multiple files, step 1, version 1

["Hyrum K. Wright" <hy...@byu.edu>]

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.

-Hyrum

[[[
Rev svn_client_move3 to allow it to take multiple source paths.  Modify
the client application to call the new version.

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.
]]]

Re: [PATCH] Move multiple files, step 1, version 1

["Hyrum K. Wright" <hy...@byu.edu>]

Julian Foad wrote:
>>> 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?
>>
>>
>> Wouldn't it be possible to just check and see if the destination is a
>> target when there is more than one source?  If that case were not met,
>> we could error.
> 
> (I assume you mean "directory" instead of "target".)

Yes.  My hands sometimes have a mind of their own. :)

> No, I don't think we can just check, because I think the "multiple
> files" mode needs to work in the same way regardless whether the number
> of files is one or more than one, and the way it needs to work (the
> destination always being a directory in which to put the sources) is
> incompatible with the current semantics for one file.

Ah, the light just turned on. :)  This could probably be accomplished
without a rev of the current API, just an addition.  We would still do
some preliminary checking in the client application to see which API to
call (e.g., are we renaming a file, or are we moving just one file to a
directory.)

Such an addition to the API would also make it possible to eventually
implement "true renames", instead of the current "delete and add"
functionality.

-Hyrum

Re: [PATCH] Move multiple files, step 1, version 1

[Julian Foad <ju...@btopenworld.com>]

Hyrum K. Wright wrote:
> 
> I think that is will be easier to first implement move, which only has
> two cases (WC->WC and URL->URL), and then go back and implement copy,
> which has four.

OK.

>>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?
> 
> Wouldn't it be possible to just check and see if the destination is a
> target when there is more than one source?  If that case were not met,
> we could error.

(I assume you mean "directory" instead of "target".)

No, I don't think we can just check, because I think the "multiple files" mode 
needs to work in the same way regardless whether the number of files is one or 
more than one, and the way it needs to work (the destination always being a 
directory in which to put the sources) is incompatible with the current 
semantics for one file.

- Julian

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] Move multiple files, step 1, version 1

["Hyrum K. Wright" <hy...@byu.edu>]

Julian Foad wrote:
> 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.

Good point, I'll hold off on exactly what needs to be done with the API
until I've worked a bit more on the implementation.

> 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.

I think that is will be easier to first implement move, which only has
two cases (WC->WC and URL->URL), and then go back and implement copy,
which has four.

> 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?

Wouldn't it be possible to just check and see if the destination is a
target when there is more than one source?  If that case were not met,
we could error.

> 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.

I think that I'll be able to continue just fine.  I look forward to your
findings about the API definition.

-Hyrum

Re: [PATCH] Move multiple files, step 1, version 1

[Julian Foad <ju...@btopenworld.com>]

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