Uploaded image for project: 'Subversion'
  1. Subversion
  2. SVN-1294

rethink .svn/ area read-only files and dirs strategy



    • Type: Task
    • Status: Open
    • Priority: Major
    • Resolution: Unresolved
    • Affects Version/s: all
    • Fix Version/s: 1.10-consider
    • Component/s: libsvn_wc
    • Labels:


      The History:
      Revision 5663 changed svn_io_remove_file() to no longer chmod its
      target to read-write unconditionally, the idea being that a caller
      should know whether or not its target is writeable.  Inspection of the
      code showed that some call sites were already manually chmodding the
      targets anyway, so I just removed svn_io_remove_file()'s chmod call
      and ran 'make check'.  It passed (on Unix), so I committed.
      It turns out that the tests only passed because file removal under
      Unix doesn't demand that the file be read-write.  Windows is
      different, so I had broken SVN on Windows.  Whee.  So now that portion
      of revision 5663 has been reverted (I don't know the revision number
      of the reversion yet, but it will be 5834 or shortly after), thus
      allowing Michael Price to release a 0.22 that works on Windows, ahem.
      The Bigger Picture:
      Mike Pilato did some research into the callers of svn_io_remove_file()
      and svn_io_remove_dir(), to see which ones needed to set their targets
      read-write but were failing to do so.  The idea was that we'd add a
      `force' flag to those svn_io_ functions, and those callers that needed
      to pass it would do so.  As Mike points out:
         > As it turns out, in the svn_io_remove_dir() case, we currently are
         > looping over APR dirents, removing each one individually.  It would
         > be trivial to ask APR to populate each dirent's permission flagset as
         > it is handed to us from apr_dir_read().  That way our code can
         > simply examine finfo->protection, and optionally set the dirent
         > read-write before trying to remove that dirent.
      However, he also discovered a small bit of mess:
         > Oh heh, I just noticed that svn_wc__remove_adm_file() first sets the
         > to-be-removed file read-write, then does the removal.  Of course, I
         > also happened to notice that there are two uses of this function,
         > both of which are for files that don't get set read-only in the first
         > place.  Geez, this stuff is messed up.
      Cleaning that up would be part of this change.  But more importantly,
      there's the whole question of what exactly in the .svn/ area should be
      read-only.  If the purpose of setting stuff in there read-only is to
      lower the chance of accidental wc corruption, then the most sensible
      thing would be to make *everything* in .svn/ read-only, and make sure
      that every libsvn_wc call operating on the .svn/ area passes the
      `force' flag to the appropriate svn_io_ functions (and those that
      don't take one now would get one).  Or if we don't want to do `force'
      flags in svn_io_, then libsvn_wc should get wrapper functions for
      certain svn_io_ calls -- hmm, that might be better than a flag,
      because the wrappers could set stuff back to read-only after the
      underlying svn_io_ operation is done.
      Anyway, currently .svn/ and its subtree are not entirely read-only. 
      Filing this in Post-1.0, as it's really all a safety optimization.


          Issue Links



              • Assignee:
                kfogel Karl Fogel
              • Votes:
                0 Vote for this issue
                2 Start watching this issue


                • Created: