Re: [PATCH] issue #532 read-only admin area

Greg Stein <gstein@lyra.org> writes:

> On Mon, Jan 28, 2002 at 12:24:31AM +0000, Philip Martin wrote:
> >...
> > * subversion/libsvn_wc/log.c
> > (replace_text_base): Pass new parameter to svn_io_copy_file.
> > (log_do_file_readonly): New function.
> > (start_handler): Handle SVN_WC__LOG_READONLY.
>
> I can see the need for a 'readonly' log item, to make a file readonly after
> it has been created in tmp and then installed. But how about if this was
> part of the 'mv' operation -- move it into place and make it readonly. That
> would seem to be the /real/ operation that is being attempted, rather than a
> distinct 'make readonly' operation.

Is that better? Then one has to worry about permissions on every mv
(and copy?) operation. At present permissions only enter at certain
specific places. I suppose it could be an optional part of mv/cp. From
my (Unixcentric) point of view, setting permissions is a separate
operation.

Is one complex function better than two simple ones?

>
> >...
> > +++ ../svn/subversion/libsvn_subr/io.c Sun Jan 27 22:28:57 2002
> > @@ -324,15 +324,55 @@
> >
> >
> > svn_error_t *
> > -svn_io_copy_file (const char *src, const char *dst, apr_pool_t *pool)
> > +svn_io_copy_file (const char *src,
> > + const char *dst,
> > + svn_boolean_t copy_perms,
> > + apr_pool_t *pool)
> > {
> > apr_status_t apr_err;
> > + apr_finfo_t finfo;
> > +
> > + /* apr_copy_file will copy the permissions, if that is not wanted we need
> > + to get the initial permissions and restore them after the copy. */
> > + if (!copy_perms)
>
> I'd much rather see a patch that changes apr_copy_file() to take a
> 'copy_perms' flag. That is much better than the extra operations here.

I was thinking about this, see my earlier mail to the list asking
about the APR functions.

>
> > + {
> > + apr_file_t *tmp;
> > +
> > + /* It must be possible to open APR_WRITE if the copy is going to
> > + succeed. */
> > + apr_err = apr_file_open (&tmp, dst,
> > + APR_WRITE | APR_CREATE, APR_OS_DEFAULT,
> > + pool);
> > + if (! APR_STATUS_IS_SUCCESS (apr_err))
> > + return svn_error_createf (apr_err, 0, NULL, pool,
> > + "cannot open '%s'", dst);
> > +
> > + apr_err = apr_stat (&finfo, dst,
> > + APR_FINFO_UPROT | APR_FINFO_GROUP | APR_FINFO_WPROT,
> > + pool);
> > + if (! APR_STATUS_IS_SUCCESS (apr_err))
> > + return svn_error_createf (apr_err, 0, NULL, pool,
> > + "cannot stat '%s'", dst);
> > +
> > + apr_err = apr_file_close (tmp);
> > + if (! APR_STATUS_IS_SUCCESS (apr_err))
> > + return svn_error_createf (apr_err, 0, NULL, pool,
> > + "cannot close '%s'", dst);
> > + }
>
> And even if we *didn't* patch APR, then you don't need to open/close the
> file ahead of time. Just do the apr_stat(). If the file wasn't there, then
> you would simply skip the copy of the perms (and leave the new copy as
> APR_OS_DEFAULT).

I don't understand. With the current APR, if I skip the copy of the
perms I do not get APR_OS_DEFAULT. That is the problem.

-- 
Philip
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org