On 8/18/06, Paul Burba <paulb@softlanding.com> wrote:
> "Ivan Zhakov" <chemodax@gmail.com> wrote on 08/14/2006 05:24:33 AM:
>
> > On 8/10/06, Paul Burba <paulb@softlanding.com> wrote:
> > > Paul Burba <paulb@softlanding.com> wrote on 08/09/2006 02:22:01 PM:
> > >
> > > Ivan,
> > >
> > > I took a second look at doing this in libsvn_wc rather than
> libsvn_client
> > > as you suggested. It doesn't take much to copy added files, but I'm
> > > getting nowhere fast when copying added directories. Is having
> similar
> > > svn add testing logic in libsvn_wc and libsvn_client really that
> > > "dangerous" as you say, why exactly?
> > >
> > I mean that it dangerous because introduce undocumented behavior
> > dependency between two layers (libsvn_client and libsvn_wc), which is
> > not right thing.
> > Just imagine: we have rewritten libsvn_wc using central database and
> > there is no problems to copy added files in this situation, but it
> > will require to change libsvn_client. This is violation of
> > incapsulation between layers.
>
> Hi Ivan,
>
> I understand not making code in libsvn_client dependent on implementation
> details in libsvn_wc, but I don't see that my original patch was doing
> that, it only made calls to libsvn_wc's public API. Short of libsvn_wc
> API changes, how was my patch dependent on libsvn_wc?
>
> Regardless, you might want to wait before answering that, it's a moot
> point! In my attempts to ensure unversioned paths also got copied, I
> encountered other problems which drove me once more to providing a
> solution entirely within libsvn_wc. It wasn't as difficult as I first
> thought...but what is once you've done it :-)
Ok, let's drop this moot topic and save our time :)
>
> This new patch provides a helper function for svn_wc_copy2() to go along
> with copy_file_administratively() and copy_file_administratively(). The
> new function, copy_added_path_administratively(), recursively walks the
> source working copy tree from the top, copying files and
> (non-administrative) dirs one at a time* and adding them via svn_wc_add2()
> if they are versioned in the source of the copy. As versioned dirs are
> copied, the destination path (locked) svn_wc_adm_access_t is expanded down
> one directory at a time.
>
> * Except when an unversioned dir is encountered, then it just recursively
> copies the whole thing.
>
> "Ivan Zhakov" <chemodax@gmail.com> wrote on 08/08/2006 02:59:16 PM:
> > Now unversioned files in versioned commited directory copied when you
> > copy directory. I mean:
> > $ mkdir dir
> > $ svn add dir
> > $ svn ci -m ""
> > $ touch dir\foo
> > $ svn st dir
> > ? dir\foo
> >
> > $ svn cp dir dir2
> > $ svn st dir2
> > ? dir2\foo
> >
> > But your patch seems copy only versioned files, i.e.
> > $ mkdir dir
> > $ svn add dir
> > $ svn ci -m ""
> > $ touch dir\foo
> > $ svn st dir
> > A dir
> > ? dir\foo
> >
> > $ svn cp dir dir2
> > $ svn st dir2
> > A dir2
> >
> > So there is inconsistency. I consider it's bad.
>
> As mentioned above, the new patch copies/moves all unversioned files/dirs.
> The copy_move_added_paths test now checks that this is done correctly.
>
> Take a look if you have some time, thanks,
Good work, Paul. Your patch looks good, but I have some thoughts:
1. Did you try split copy_added_path_administratively to
copy_added_file_administratively/copy_added_dir_administratively()?
Because copy_added_path_administratively() have separated
implementation for each case. I think it will be reasonable to split
it.
2. Might good idea to add src_is_versioned parameter to
copy_added_path_administratively (or
copy_added_file_administratively/copy_added_dir_administratively),
because caller always know this and there is no reason to duplicate
stat calls.
3. There is duplication of code at:
+ else if (this_entry.filetype != APR_UNKFILE)
+ {
+ /* Copy the file and add it to version control if the
+ source of the copy is versioned. */
+ SVN_ERR(svn_io_copy_file(src_fullpath, dst_fullpath, TRUE,
+ subpool));
This code is similiar to what is done at top of
copy_added_path_administratively. I think if you implement (1) and (2)
you can just call copy_added_file_administratively here.
4. Also at the same place:
+ else if (this_entry.filetype != APR_UNKFILE)
+ {
+ /* Copy the file and add it to version control if the
+ source of the copy is versioned. */
+ SVN_ERR(svn_io_copy_file(src_fullpath, dst_fullpath, TRUE,
+ subpool));
+ SVN_ERR(svn_wc_entry(&entry,
+ svn_path_join(src_path,
+ this_entry.name,
+ pool),
+ src_child_dir_access, TRUE, pool));
I didn't see reasons to construct full source path again, because you
have it in variable src_fullpath.
5.
+ else /* Yes, it exited cleanly, so close the dir. */
+ {
+ apr_status_t apr_err;
+
+ svn_error_clear(err);
+ apr_err = apr_dir_close(dir);
+ if (apr_err)
+ return svn_error_wrap_apr(apr_err,
+ _("Can't close directory '%s'"),
+ svn_path_local_style(src_path,
+ subpool));
+ }
Btw, why we have not svn_io_dir_close() ??
>
> Paul B.
>
> [[[
> Support copy and move of paths scheduled for addition.
>
> Follow-up to r20811.
>
> Suggested by: zhakov
>
> * subversion/libsvn_wc/copy.c
> (copy_added_path_administratively): New recursive helper function which
> copies added paths, including unversioned paths within added
> directories.
> (svn_wc_copy2): Use new helper function when copying added paths.
> * subversion/tests/cmdline/copy_tests.py
> (copy_move_added_paths, copy_added_paths_to_URL,): New tests.
> (test_list): Run new tests.
> ]]]
>
>
>
>
--
Ivan Zhakov
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Aug 17 23:40:42 2006