You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Paul Burba <pa...@softlanding.com> on 2006/06/05 17:11:23 UTC
[PATCH] Moving Unversioned Files
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?
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?
Paul B.
_____________________________________________________________________________
Scanned for SoftLanding Systems, Inc. and SoftLanding Europe Plc by IBM Email Security Management Services powered by MessageLabs.
_____________________________________________________________________________
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Moving Unversioned Files
Posted by Paul Burba <pa...@softlanding.com>.
rooneg@gmail.com wrote on 06/05/2006 01:20:19 PM:
> On 6/5/06, Paul Burba <pa...@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.
Done r19963. I made one mv_unversioned_file test that tries to move an
unversioned file without --force and then tries to move another
unversioned file with --force. That is all the other two tests did.
> > 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...
No, I'm just being too trusting/stupid - I'll leave the code as-is.
Paul B.
_____________________________________________________________________________
Scanned for SoftLanding Systems, Inc. and SoftLanding Europe Plc by IBM Email Security Management Services powered by MessageLabs.
_____________________________________________________________________________
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Moving Unversioned Files
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 6/5/06, Paul Burba <pa...@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