On 6/5/06, Paul Burba <paulb@softlanding.com> wrote:
> Hi All,
>
> Came across this tidbit this morning:
>
> copy_tests.py has two tests in it's test_list named "mv_unversioned_file".
> There are also two definitions for mv_unversioned_file, so only the
> second is used. I was going to simply rename the second instance and
> commit it, but then I looked into things a bit further...never look into
> things a bit further it just causes pain :-D
>
> Both tests try to move unversioned files and were added to test fixes that
> prevented seg faults. The only significant difference between the two is
> that the second test uses the --force option.
>
> The first mv_unversioned_file is from was back in the day, r1106. This
> revision also modified copy.c's copy_file_administratively() to check if
> the source of a move is unversioned.
>
> The second mv_unversioned_file is from r17242 & r17243 and was added as a
> test for issue #2436. r17243 checks if the source of a move is versioned
> in svn_wc_copy2().
>
> It appears that the check of the move source in svn_wc_copy2() makes the
> check in copy_file_administratively() redundant, since the check in the
> former is made before calling the latter and svn_wc_copy2() is the only
> caller of copy_file_administratively(). FWIW, the comment for
> copy_file_administratively() states clearly that we *shouldn't* have to
> check for this error:
>
> /* This function effectively creates and schedules a file for
> addition, but does extra administrative things to allow it to
> function as a 'copy'.
>
> ASSUMPTIONS:
>
> - src_path points to a file under version control
>
> Given the above I have two questions, should I:
>
> Q1) Just rename the second instance of mv_unversioned_file to
> mv_unversioned_file2 and call it a day? Or combine the two instances of
> mv_unversioned_file into one test?
If it's easy to combine them into one test, I'd do it. Saving on test
framework setup time is a good thing.
> Q2) Just leave copy.c alone, change the comment for
> copy_file_administratively, or remove the check for "versioned-ness" in
> copy_file_administratively():
>
> Index: libsvn_wc/copy.c
> ===================================================================
> --- copy.c (revision 19933)
> +++ copy.c (working copy)
> @@ -94,11 +94,6 @@
> in the repository. See comment at the bottom of this file for an
> explanation. */
> SVN_ERR(svn_wc_entry(&src_entry, src_path, src_access, FALSE, pool));
> - if (! src_entry)
> - return svn_error_createf
> - (SVN_ERR_UNVERSIONED_RESOURCE, NULL,
> - _("Cannot copy or move '%s': it's not under version control"),
> - svn_path_local_style(src_path, pool));
> if ((src_entry->schedule == svn_wc_schedule_add)
> || (! src_entry->url)
> || (src_entry->copied))
>
> Thoughts?
If we're already retrieving that entry and checking for it to be NULL
further up the stack, then I imagine we can get away with removing
that check, although I'm not sure if we really should. If we do, then
the doc comment for copy_file_administratively should be more strict,
right now it says that src_path must point to a file under version
control, which isn't quite the same as saying "it must point to
something we've already called svn_wc_entry on so that it'll be
certain to be in the entries cache, cause we're not going to check
that it's not NULL"...
Or maybe i'm just being paranoid...
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Jun 5 19:21:09 2006