On 6/5/06, Paul Burba <firstname.lastname@example.org> 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'.
> - 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
> 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))
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...
To unsubscribe, e-mail: email@example.com
For additional commands, e-mail: firstname.lastname@example.org
Received on Mon Jun 5 19:21:09 2006