You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Luke Blanshard <lu...@blanshard.us> on 2003/05/03 14:02:54 UTC
Re: svn commit: rev 5790 - trunk/subversion/libsvn_wc
Is there ever a time when you want remove_file_if_present to fail on
Windows because the permissions are wrong? In other words, why not
simply rewrite remove_file_if_present to set the file read-write before
removing it, instead of repeating the same logic everywhere? Better
yet, only do this extra step on systems that actually require it -- if
you can get APR to encapsulate this.
cmpilato@tigris.org wrote:
>Author: cmpilato
>Date: Sat May 3 00:49:23 2003
>New Revision: 5790
>
>Modified:
> trunk/subversion/libsvn_wc/adm_ops.c
>Log:
>* subversion/libsvn_wc/adm_ops.c
> (svn_wc_remove_from_revision_control): Fixup for revision 5787.
> If the file you're setting attributes on isn't there, don't sweat it.
> Should have taken the queue from all those remove_file_***if_present***
> calls...sheesh...
>
>Modified: trunk/subversion/libsvn_wc/adm_ops.c
>==============================================================================
>--- trunk/subversion/libsvn_wc/adm_ops.c (original)
>+++ trunk/subversion/libsvn_wc/adm_ops.c Sat May 3 00:49:23 2003
>@@ -1648,25 +1648,25 @@
>
> /* Text base. */
> svn_thang = svn_wc__text_base_path (full_path, 0, subpool);
>- SVN_ERR (svn_io_set_file_read_write (svn_thang, FALSE, subpool));
>+ SVN_ERR (svn_io_set_file_read_write (svn_thang, TRUE, subpool));
> SVN_ERR (remove_file_if_present (svn_thang, subpool));
>
> /* Working prop file. */
> SVN_ERR (svn_wc__prop_path (&svn_thang, full_path, adm_access, FALSE,
> subpool));
>- SVN_ERR (svn_io_set_file_read_write (svn_thang, FALSE, subpool));
>+ SVN_ERR (svn_io_set_file_read_write (svn_thang, TRUE, subpool));
> SVN_ERR (remove_file_if_present (svn_thang, subpool));
>
> /* Prop base file. */
> SVN_ERR (svn_wc__prop_base_path (&svn_thang, full_path, adm_access,
> FALSE, subpool));
>- SVN_ERR (svn_io_set_file_read_write (svn_thang, FALSE, subpool));
>+ SVN_ERR (svn_io_set_file_read_write (svn_thang, TRUE, subpool));
> SVN_ERR (remove_file_if_present (svn_thang, subpool));
>
> /* wc-prop file. */
> SVN_ERR (svn_wc__wcprop_path (&svn_thang, full_path, adm_access, FALSE,
> subpool));
>- SVN_ERR (svn_io_set_file_read_write (svn_thang, FALSE, subpool));
>+ SVN_ERR (svn_io_set_file_read_write (svn_thang, TRUE, subpool));
> SVN_ERR (remove_file_if_present (svn_thang, subpool));
> }
>
>
>---------------------------------------------------------------------
>To unsubscribe, e-mail: svn-unsubscribe@subversion.tigris.org
>For additional commands, e-mail: svn-help@subversion.tigris.org
>
>
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 5790 - trunk/subversion/libsvn_wc
Posted by kf...@collab.net.
Branko Čibej <br...@xbc.nu> writes:
> Here's what I'd do: First, I'd instrument svn_io_remove_file to check if
> the file is read-only, and complain (to a log file?) if it's not. Then
> I'd run the test suite, fixing the calls that produce a warning until
> svn_io_remove_file is quiet.
Trying it now, good idea.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 5790 - trunk/subversion/libsvn_wc
Posted by Branko Čibej <br...@xbc.nu>.
kfogel@collab.net wrote:
>brane@xbc.nu writes:
>
>
>>It's also impossible to test ATM, for two reasons: a) the Windows
>>build doesn't, which means that nobody's running tests on Windows;
>>b) our test coverage isn't good enough.
>>
>>One thing I never saw was performance test output pointing its
>>finger of blame at that chmod. Has anyone seen any difference in the
>>speed of the client? If not, then I can't understand why you're
>>wasting time on this, breaking things left and right, without a
>>single bit of hard data to go on.
>>
>>
>
>"Left and right" is a bit much :-).
>
Yeah. I tend to exaggerate a bit every now and then. :-)
> I did run the test suite on Unix;
>I wasn't aware that Windows had different semantics w.r.t. file
>removal & perms than Unix has.
>
>My primary motivation wasn't for performance reasons, it was because I
>felt that our underlying (svn_io_) file removal mechanism shouldn't be
>doing a chmod by default, because that might someday blow away
>something that shouldn't be blown away. Instead, it should be the
>caller's responsibility to know that something is ready to be removed.
>
>Another solution is to have a `force' flag, as you mentioned, though I
>still feel that callers working in the .svn/ area ought to *know*
>whether something needs to be chmod'd or not.
>
>I'm sorry I haven't had a chance to deal with issue #1279 yet. I've
>felt the urgency somewhat lessened becuase the Win32 build is broken
>(?), and been trying to avoid getting distracted from cvs2svn, which
>we really need to get finished. I can revert the relevant portion of
>revision 5663, to get a quick fix if that's needed right away, but
>IMHO a real solution requires identifying the problematic callers and
>either passing a `force' flag or having them manually set the target
>read/write.
>
>What do you think is the best course right now?
>
I suggested adding another function (either to adm_files.c or io.c) that
deals specifically with files in .svn. Mike is right; if a caller
doesn't know the file to be deleted is in the .svn area, then something
is wrong with the caller. Unfortunately, that still means every call to
svn_io_remove_file (and rename, too) has to be checked and fixed.
For a minute there I wondered if we shouldn't just stop making the admin
files read-only, then I rememberd it's not just the text bases (which we
have checksums for), but also the prop files, which aren't checksummed.
Here's what I'd do: First, I'd instrument svn_io_remove_file to check if
the file is read-only, and complain (to a log file?) if it's not. Then
I'd run the test suite, fixing the calls that produce a warning until
svn_io_remove_file is quiet.
--
Brane Čibej <br...@xbc.nu> http://www.xbc.nu/brane/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 5790 - trunk/subversion/libsvn_wc
Posted by cm...@collab.net.
kfogel@collab.net writes:
> brane@xbc.nu writes:
> > It's also impossible to test ATM, for two reasons: a) the Windows
> > build doesn't, which means that nobody's running tests on Windows;
> > b) our test coverage isn't good enough.
> >
> > One thing I never saw was performance test output pointing its
> > finger of blame at that chmod. Has anyone seen any difference in the
> > speed of the client? If not, then I can't understand why you're
> > wasting time on this, breaking things left and right, without a
> > single bit of hard data to go on.
>
> "Left and right" is a bit much :-). I did run the test suite on
> Unix; I wasn't aware that Windows had different semantics
> w.r.t. file removal & perms than Unix has.
Actually, that aspect was a surprise to me as well.
> My primary motivation wasn't for performance reasons, it was because
> I felt that our underlying (svn_io_) file removal mechanism
> shouldn't be doing a chmod by default, because that might someday
> blow away something that shouldn't be blown away. Instead, it
> should be the caller's responsibility to know that something is
> ready to be removed.
FWIW, I agree. Our working copy library is plenty complex, but
shouldn't be so much that it can't keep track of the permissions of
the files it exists to manage. And svn_io_remove_file most certainly
should not be just blanketly ensuring that it can perform the
requested removal.
The 'force' flag idea is, in my opinion, a crutch for sloppy coding.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 5790 - trunk/subversion/libsvn_wc
Posted by kf...@collab.net.
brane@xbc.nu writes:
> It's also impossible to test ATM, for two reasons: a) the Windows
> build doesn't, which means that nobody's running tests on Windows;
> b) our test coverage isn't good enough.
>
> One thing I never saw was performance test output pointing its
> finger of blame at that chmod. Has anyone seen any difference in the
> speed of the client? If not, then I can't understand why you're
> wasting time on this, breaking things left and right, without a
> single bit of hard data to go on.
"Left and right" is a bit much :-). I did run the test suite on Unix;
I wasn't aware that Windows had different semantics w.r.t. file
removal & perms than Unix has.
My primary motivation wasn't for performance reasons, it was because I
felt that our underlying (svn_io_) file removal mechanism shouldn't be
doing a chmod by default, because that might someday blow away
something that shouldn't be blown away. Instead, it should be the
caller's responsibility to know that something is ready to be removed.
Another solution is to have a `force' flag, as you mentioned, though I
still feel that callers working in the .svn/ area ought to *know*
whether something needs to be chmod'd or not.
I'm sorry I haven't had a chance to deal with issue #1279 yet. I've
felt the urgency somewhat lessened becuase the Win32 build is broken
(?), and been trying to avoid getting distracted from cvs2svn, which
we really need to get finished. I can revert the relevant portion of
revision 5663, to get a quick fix if that's needed right away, but
IMHO a real solution requires identifying the problematic callers and
either passing a `force' flag or having them manually set the target
read/write.
What do you think is the best course right now?
> Sorry if I seem to have blown my top a bit, but this fiddling with a
> chmod looks just as well thought out as any of the myriad
> hare-brained schemes to "speed up the client" that we've so gleefuly
> shot down in the past.
If it were just that, I would agree (and no, I don't have any hard
data that it was an actual performance bottleneck, though it did clog
the output of strace quite a bit).
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 5790 - trunk/subversion/libsvn_wc
Posted by br...@xbc.nu.
Quoting kfogel@collab.net:
> Luke Blanshard <lu...@blanshard.us> writes:
> > Is there ever a time when you want remove_file_if_present to fail on
> > Windows because the permissions are wrong? In other words, why not
> > simply rewrite remove_file_if_present to set the file read-write
> > before removing it, instead of repeating the same logic everywhere?
> > Better yet, only do this extra step on systems that actually require
> > it -- if you can get APR to encapsulate this.
>
> We used to have similar logic in svn_io_removed_file() too, but it
> turned out it was masking some inconsistent assumptions in the calling
> code -- a lot of the time, the file was already writeable, and we were
> wasting time setting it so.
>
> After that, we made a policy that the low-level removal functions
> would always assume that their targets were writeable, and callers
> would be responsible for ensuring this.
Was this policy discussed on the list? I must've missed that thread...
> While this decision hasn't
> made its way to all the code yet (see issue 1279), I think it's a good
> way to go...
It's also impossible to test ATM, for two reasons: a) the Windows build doesn't,
which means that nobody's running tests on Windows; b) our test coverage isn't
good enough.
One thing I never saw was performance test output pointing its finger of blame
at that chmod. Has anyone seen any difference in the speed of the client? If
not, then I can't understand why you're wasting time on this, breaking things
left and right, without a single bit of hard data to go on.
BTW, that chmod is only necessary on Windows, never on Unix. So if you want to
speed things up for Unix users, revert all these changes and put an "#ifdef
SVN_WIN32" around the chmod in svn_io_remove_file. Or write an
"svn_adm_remove_file" and use that wherever we're deleting files in the .svn
area. Or just stop making those files read-only (huh, another waste of time,
since 99% of the time users won't fiddle with those files).
Sorry if I seem to have blown my top a bit, but this fiddling with a chmod looks
just as well thought out as any of the myriad hare-brained schemes to "speed up
the client" that we've so gleefuly shot down in the past.
Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 5790 - trunk/subversion/libsvn_wc
Posted by kf...@collab.net.
Luke Blanshard <lu...@blanshard.us> writes:
> Is there ever a time when you want remove_file_if_present to fail on
> Windows because the permissions are wrong? In other words, why not
> simply rewrite remove_file_if_present to set the file read-write
> before removing it, instead of repeating the same logic everywhere?
> Better yet, only do this extra step on systems that actually require
> it -- if you can get APR to encapsulate this.
We used to have similar logic in svn_io_removed_file() too, but it
turned out it was masking some inconsistent assumptions in the calling
code -- a lot of the time, the file was already writeable, and we were
wasting time setting it so.
After that, we made a policy that the low-level removal functions
would always assume that their targets were writeable, and callers
would be responsible for ensuring this. While this decision hasn't
made its way to all the code yet (see issue 1279), I think it's a good
way to go...
-K
> >Author: cmpilato
> >Date: Sat May 3 00:49:23 2003
> >New Revision: 5790
> >
> >Modified:
> > trunk/subversion/libsvn_wc/adm_ops.c
> >Log:
> >* subversion/libsvn_wc/adm_ops.c
> > (svn_wc_remove_from_revision_control): Fixup for revision 5787.
> > If the file you're setting attributes on isn't there, don't sweat it.
> > Should have taken the queue from all those remove_file_***if_present***
> > calls...sheesh...
> >
> >Modified: trunk/subversion/libsvn_wc/adm_ops.c
> >==============================================================================
> >--- trunk/subversion/libsvn_wc/adm_ops.c (original)
> >+++ trunk/subversion/libsvn_wc/adm_ops.c Sat May 3 00:49:23 2003
> >@@ -1648,25 +1648,25 @@
> > /* Text base. */
> > svn_thang = svn_wc__text_base_path (full_path, 0, subpool);
> >- SVN_ERR (svn_io_set_file_read_write (svn_thang, FALSE, subpool));
> >+ SVN_ERR (svn_io_set_file_read_write (svn_thang, TRUE, subpool));
> > SVN_ERR (remove_file_if_present (svn_thang, subpool));
> > /* Working prop file. */
> > SVN_ERR (svn_wc__prop_path (&svn_thang, full_path, adm_access, FALSE,
> > subpool));
> >- SVN_ERR (svn_io_set_file_read_write (svn_thang, FALSE, subpool));
> >+ SVN_ERR (svn_io_set_file_read_write (svn_thang, TRUE, subpool));
> > SVN_ERR (remove_file_if_present (svn_thang, subpool));
> > /* Prop base file. */
> > SVN_ERR (svn_wc__prop_base_path (&svn_thang, full_path, adm_access,
> > FALSE, subpool));
> >- SVN_ERR (svn_io_set_file_read_write (svn_thang, FALSE, subpool));
> >+ SVN_ERR (svn_io_set_file_read_write (svn_thang, TRUE, subpool));
> > SVN_ERR (remove_file_if_present (svn_thang, subpool));
> > /* wc-prop file. */
> > SVN_ERR (svn_wc__wcprop_path (&svn_thang, full_path, adm_access, FALSE,
> > subpool));
> >- SVN_ERR (svn_io_set_file_read_write (svn_thang, FALSE, subpool));
> >+ SVN_ERR (svn_io_set_file_read_write (svn_thang, TRUE, subpool));
> > SVN_ERR (remove_file_if_present (svn_thang, subpool));
> > }
> > ---------------------------------------------------------------------
> >To unsubscribe, e-mail: svn-unsubscribe@subversion.tigris.org
> >For additional commands, e-mail: svn-help@subversion.tigris.org
> >
> >
> >
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org