You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Kevin Pilch-Bisson <ke...@pilch-bisson.net> on 2001/11/28 18:27:02 UTC
[PATCH] Make working copies read-only.
Subject and log message say it all.
Log:
Make the .svn area read-only, with the exception of things in tmp, which
will be writable. This will have no effect on platforms other unix, since
apr_file_open does not obey the permission flags, and apr_file_perms_set
is not implemented.
This patch is safe to apply to an already existing working copy. It will
gradually make file read-only as they are updated or committed.
* subversion/include/svn_io.h:(svn_io_rename_file): New prototype for a
wrapper around apr_file_rename;
(svn_io_set_permissions): New prototype for a function which is able to
set the permissions in a matter that obeys a user's umask.
* subversion/libsvn_subr/io.c:(svn_io_append_file): Format for 80 column
lines.
(svn_io_rename_file): New function, a wrapper around apr_file_rename.
(svn_io_set_permissions): New function, on unix this is able to retreive
the user's umask and set the file as if it were newly created. On other
platforms this does nothing since apr_file_perms_set is not implemented
on any other platform.
* subversion/libsvn_wc/wc.h:Doc chanes for SVN_LOG_ATTR_MV and
SVN_LOG_ATTR_CP.
(SVN_WC__LOG_ATTR_PERM): New define, use to indicate what permissions a
move, copy, or append action should set the dest file to.
(SVN_WC__LOG_PERM_SRC): New define, don't change permissions.
(SVN_WC__LOG_DEFAULT): New define, use os-default permissions.
(SVN_WC__LOG_EXEC): New define, make file executable(not currently used,
but might be useful for making checkouts/updates recognize executable
files.
(SVN_WC__LOG_READONLY): New define, make file read-only.
* subversion/libsvn_wc/props.c:(svn_wc__do_property_merge): Pass
permissions to log entries.
* subversion/libsvn_wc/adm_crawler.c:(restore_file): Make files writable
when copying them out to the working copy.
* subversion/libsvn_wc/log.c:(svn_wc__perm_type): New enum.
(file_xfer_under_path) Takes new `perm' argument, sets dest file
permissions according to it.
(log_do_file_xfer) Determine perm argument for file_xfer_under_path from
the xml attributes.
* subversion/libsvn_wc/adm_files.c:(maybe_copy_file): Set permissions when
copying the file to the tmp area.
(sync_adm_file): Set permissions on the file when moving it out of the
tmp area.
(close_adm_file): Set permissions on the file when moving it
out of the tmp area.
(svn_wc__close_auth_file): Make the file readable only by the user when
closing it.
(svn_wc__remove_adm_file): Make the file writable before removing it.
* subversion/libsvn_wc/get_editor.c:(close_file): Pass permissions to be
used to log entries.
Index: ./subversion/include/svn_io.h
===================================================================
--- ./subversion/include/.svn/text-base/svn_io.h.svn-base Wed Nov 21 12:45:58 2001
+++ ./subversion/include/svn_io.h Tue Nov 27 13:47:16 2001
@@ -129,6 +129,19 @@
svn_stringbuf_t *dst,
apr_pool_t *pool);
+/* Rename SRC to DST. DST will be overwritten if it exists. */
+svn_error_t *svn_io_rename_file (svn_stringbuf_t *src,
+ svn_stringbuf_t *dst,
+ apr_pool_t *pool);
+
+/* Change the permissions on PATH to PERMS. If APPLY_UMASK is true then
+ * the permissions will be combined with the umask in the same way as
+ * open, if the platform permits. Otherwise, the permissions will be set
+ * to exactly PERMS. */
+svn_error_t *svn_io_set_permissions (svn_stringbuf_t *path,
+ apr_fileperms_t perms,
+ svn_boolean_t apply_umask,
+ apr_pool_t *pool);
/* Read a line from FILE into BUF, but not exceeding *LIMIT bytes.
* Does not include newline, instead '\0' is put there.
Index: ./subversion/libsvn_wc/props.c
===================================================================
--- ./subversion/libsvn_wc/.svn/text-base/props.c.svn-base Wed Nov 21 11:22:30 2001
+++ ./subversion/libsvn_wc/props.c Wed Nov 28 12:25:44 2001
@@ -620,6 +620,8 @@
tmp_prop_base,
SVN_WC__LOG_ATTR_DEST,
real_prop_base,
+ SVN_WC__LOG_ATTR_PERM,
+ svn_stringbuf_create (SVN_WC__LOG_PERM_SRC, pool),
NULL);
/* Write log entry to move working tmp copy to real working area. */
@@ -631,6 +633,8 @@
tmp_props,
SVN_WC__LOG_ATTR_DEST,
real_props,
+ SVN_WC__LOG_ATTR_PERM,
+ svn_stringbuf_create (SVN_WC__LOG_PERM_SRC, pool),
NULL);
if (reject_tmp_fp)
Index: ./subversion/libsvn_wc/wc.h
===================================================================
--- ./subversion/libsvn_wc/.svn/text-base/wc.h.svn-base Mon Nov 26 09:35:17 2001
+++ ./subversion/libsvn_wc/wc.h Wed Nov 28 12:26:52 2001
@@ -373,10 +373,12 @@
*/
#define SVN_WC__LOG_RUN_CMD "run"
-/* Move file SVN_WC__LOG_ATTR_NAME to SVN_WC__LOG_ATTR_DEST. */
+/* Move file SVN_WC__LOG_ATTR_NAME to SVN_WC__LOG_ATTR_DEST, with
+ * permissions SVN_WC__LOG_ATTR_PERM. */
#define SVN_WC__LOG_MV "mv"
-/* Copy file SVN_WC__LOG_ATTR_NAME to SVN_WC__LOG_ATTR_DEST. */
+/* Copy file SVN_WC__LOG_ATTR_NAME to SVN_WC__LOG_ATTR_DEST, with
+ * permissions SVN_WC__LOG_ATTR_PERM. */
#define SVN_WC__LOG_CP "cp"
/* Remove file SVN_WC__LOG_ATTR_NAME. */
@@ -415,6 +417,7 @@
#define SVN_WC__LOG_ATTR_NAME "name"
#define SVN_WC__LOG_ATTR_DEST "dest"
#define SVN_WC__LOG_ATTR_REVISION "revision"
+#define SVN_WC__LOG_ATTR_PERM "permission"
#define SVN_WC__LOG_ATTR_TEXT_REJFILE "text-rejfile"
#define SVN_WC__LOG_ATTR_PROP_REJFILE "prop-rejfile"
/* The rest are for SVN_WC__LOG_RUN_CMD. Extend as necessary. */
@@ -430,6 +433,15 @@
#define SVN_WC__LOG_ATTR_ARG_7 "arg7"
#define SVN_WC__LOG_ATTR_ARG_8 "arg8"
#define SVN_WC__LOG_ATTR_ARG_9 "arg9"
+
+/* Keep permissions the same as the source file. */
+#define SVN_WC__LOG_PERM_SRC "src"
+/* Make the permissions the operating system's default. */
+#define SVN_WC__LOG_PERM_DEFAULT "default"
+/* Make the permissions os/default, but with the execute bit set. */
+#define SVN_WC__LOG_PERM_EXEC "executable"
+/* Make the permissions read-only. */
+#define SVN_WC__LOG_PERM_READONLY "read-only"
/* Starting at PATH, write out log entries indicating that a commit
Index: ./subversion/libsvn_wc/adm_crawler.c
===================================================================
--- ./subversion/libsvn_wc/.svn/text-base/adm_crawler.c.svn-base Mon Nov 26 09:35:17 2001
+++ ./subversion/libsvn_wc/adm_crawler.c Tue Nov 27 14:09:04 2001
@@ -1795,12 +1795,9 @@
tmp_text_base_path = svn_wc__text_base_path (file_path, 1, pool);
SVN_ERR (svn_io_copy_file (text_base_path, tmp_text_base_path, pool));
- status = apr_file_rename (tmp_text_base_path->data, file_path->data, pool);
- if (status)
- return svn_error_createf (status, 0, NULL, pool,
- "error renaming `%s' to `%s'",
- tmp_text_base_path->data,
- file_path->data);
+ SVN_ERR (svn_io_set_permissions (tmp_text_base_path, APR_OS_DEFAULT,
+ TRUE, pool));
+ SVN_ERR (svn_io_rename_file (tmp_text_base_path, file_path, pool));
return SVN_NO_ERROR;
}
Index: ./subversion/libsvn_wc/log.c
===================================================================
--- ./subversion/libsvn_wc/.svn/text-base/log.c.svn-base Wed Nov 21 08:50:24 2001
+++ ./subversion/libsvn_wc/log.c Tue Nov 27 14:21:19 2001
@@ -51,18 +51,27 @@
svn_wc__xfer_mv,
};
+enum svn_wc__perm_type {
+ svn_wc__perm_src,
+ svn_wc__perm_default,
+ svn_wc__perm_exec,
+ svn_wc__perm_readonly
+};
-/* Copy (or rename, if RENAME is non-zero) NAME to DEST, assuming that
- PATH is the common parent of both locations. */
+/* Transfer contents of NAME to DEST, assuming that PATH is the common
+ parent of both locations, according to ACTION. If ACTION requires that
+ DEST be created, give it permissions appropriate to PERM. */
static svn_error_t *
file_xfer_under_path (svn_stringbuf_t *path,
const char *name,
const char *dest,
enum svn_wc__xfer_action action,
+ enum svn_wc__perm_type perm,
apr_pool_t *pool)
{
apr_status_t status;
+ apr_fileperms_t fperms;
svn_stringbuf_t *full_from_path, *full_dest_path;
full_from_path = svn_stringbuf_dup (path, pool);
@@ -71,22 +80,39 @@
svn_path_add_component_nts (full_from_path, name, svn_path_local_style);
svn_path_add_component_nts (full_dest_path, dest, svn_path_local_style);
+ switch (perm)
+ {
+ case svn_wc__perm_default:
+ fperms = APR_OS_DEFAULT;
+ break;
+ case svn_wc__perm_exec:
+ fperms = APR_UREAD | APR_GREAD | APR_WREAD | APR_UWRITE
+ | APR_GWRITE | APR_WWRITE | APR_UEXECUTE | APR_GEXECUTE
+ | APR_WEXECUTE;
+ break;
+ case svn_wc__perm_src:
+ /* Doesn't matter, since the permissions won't be applied anyway. */
+ break;
+ case svn_wc__perm_readonly:
+ fperms = APR_UREAD | APR_GREAD | APR_WREAD;
+ }
+
switch (action)
{
case svn_wc__xfer_append:
- return svn_io_append_file (full_from_path, full_dest_path, pool);
+ SVN_ERR (svn_io_append_file (full_from_path, full_dest_path, pool));
+ break;
case svn_wc__xfer_cp:
- return svn_io_copy_file (full_from_path, full_dest_path, pool);
+ SVN_ERR (svn_io_copy_file (full_from_path, full_dest_path, pool));
+ break;
case svn_wc__xfer_mv:
- status = apr_file_rename (full_from_path->data,
- full_dest_path->data, pool);
- if (status)
- return svn_error_createf (status, 0, NULL, pool,
- "file_xfer_under_path: "
- "can't move %s to %s",
- name, dest);
+ SVN_ERR (svn_io_rename_file (full_from_path, full_dest_path, pool));
+ break;
}
+ if (perm != svn_wc__perm_src)
+ SVN_ERR (svn_io_set_permissions (full_dest_path, fperms, TRUE, pool));
+
return SVN_NO_ERROR;
}
@@ -238,15 +264,34 @@
const XML_Char **atts)
{
svn_error_t *err;
+ enum svn_wc__perm_type perm = svn_wc__perm_src;
const char *dest = svn_xml_get_attr_value (SVN_WC__LOG_ATTR_DEST, atts);
+ const char *permstr = svn_xml_get_attr_value (SVN_WC__LOG_ATTR_PERM, atts);
if (! dest)
return svn_error_createf (SVN_ERR_WC_BAD_ADM_LOG, 0, NULL, loggy->pool,
"missing dest attr in %s", loggy->path->data);
- /* Else. */
+ if (permstr)
+ {
+ if (strcmp (permstr, SVN_WC__LOG_PERM_SRC) == 0)
+ perm = svn_wc__perm_src;
+ else if (strcmp (permstr, SVN_WC__LOG_PERM_DEFAULT) == 0)
+ perm = svn_wc__perm_default;
+ else if (strcmp (permstr, SVN_WC__LOG_PERM_EXEC) == 0)
+ perm = svn_wc__perm_exec;
+ else if (strcmp (permstr, SVN_WC__LOG_PERM_READONLY) == 0)
+ perm = svn_wc__perm_readonly;
+ else
+ return svn_error_createf (SVN_ERR_WC_BAD_ADM_LOG, 0, NULL,
+ loggy->pool,
+ "unrecognized perm attr in %s",
+ loggy->path->data);
+ }
- err = file_xfer_under_path (loggy->path, name, dest, action, loggy->pool);
+ /* Else. */
+ err = file_xfer_under_path (loggy->path, name, dest, action,
+ perm, loggy->pool);
if (err)
signal_error (loggy, err);
Index: ./subversion/libsvn_wc/adm_ops.c
===================================================================
--- ./subversion/libsvn_wc/.svn/text-base/adm_ops.c.svn-base Mon Nov 26 09:35:18 2001
+++ ./subversion/libsvn_wc/adm_ops.c Wed Nov 28 13:05:32 2001
@@ -911,6 +911,9 @@
(err->apr_err, 0, NULL, pool,
"revert_admin_things: Error restoring pristine text for '%s'",
full_path->data);
+ SVN_ERR (svn_io_set_permissions (full_path,
+ APR_OS_DEFAULT,
+ TRUE, pool));
SVN_ERR (svn_io_file_affected_time (&tstamp, full_path, pool));
/* Modify our entry structure. */
Index: ./subversion/libsvn_wc/adm_files.c
===================================================================
--- ./subversion/libsvn_wc/.svn/text-base/adm_files.c.svn-base Mon Nov 26 09:35:18 2001
+++ ./subversion/libsvn_wc/adm_files.c Wed Nov 28 13:10:44 2001
@@ -289,11 +289,10 @@
return SVN_NO_ERROR;
}
}
- else /* SRC exists, so copy it to DST. */
+ else /* SRC exists, so copy it to DST, and make sure we can write to it. */
{
- err = svn_io_copy_file (src, dst, pool);
- if (err)
- return err;
+ SVN_ERR (svn_io_copy_file (src, dst, pool));
+ SVN_ERR (svn_io_set_permissions (dst, APR_OS_DEFAULT, TRUE, pool));
}
return SVN_NO_ERROR;
@@ -313,7 +312,7 @@
given how C va_lists work. */
svn_stringbuf_t *tmp_path = svn_stringbuf_dup (path, pool);
- apr_status_t apr_err;
+ svn_error_t *err;
int components_added;
va_list ap;
@@ -327,18 +326,22 @@
v_extend_with_adm_name (tmp_path, extension, 1, pool, ap);
va_end (ap);
- /* Rename. */
- apr_err = apr_file_rename (tmp_path->data, path->data, pool);
+ /* Rename. 1) Make write-able, rename, make read-only. */
+ err = svn_io_set_permissions (path, APR_UREAD | APR_UWRITE, FALSE, pool);
+ if (!err || APR_STATUS_IS_ENOENT (err->apr_err))
+ {
+ err = svn_io_rename_file (tmp_path, path, pool);
+ if (!err)
+ err = svn_io_set_permissions (path,
+ APR_UREAD | APR_GREAD | APR_WREAD,
+ TRUE, pool);
+ }
+
/* Unconditionally restore path. */
chop_admin_name (path, components_added);
- if (apr_err)
- return svn_error_createf (apr_err, 0, NULL, pool,
- "error renaming %s to %s",
- tmp_path->data, path->data);
- else
- return SVN_NO_ERROR;
+ return err;
}
@@ -635,6 +638,7 @@
...)
{
apr_status_t apr_err = 0;
+ svn_error_t *err;
int components_added;
va_list ap;
@@ -672,17 +676,20 @@
va_end (ap);
/* Rename. */
- apr_err = apr_file_rename (tmp_path->data, path->data, pool);
+ err = svn_io_set_permissions (path, APR_UREAD | APR_UWRITE, FALSE, pool);
+ if (!err || APR_STATUS_IS_ENOENT (err->apr_err))
+ {
+ err = svn_io_rename_file (tmp_path, path, pool);
+ if (!err)
+ err = svn_io_set_permissions (path,
+ APR_UREAD | APR_GREAD | APR_WREAD,
+ TRUE, pool);
+ }
/* Unconditionally restore path. */
chop_admin_name (path, components_added);
- if (apr_err)
- return svn_error_createf (apr_err, 0, NULL, pool,
- "error renaming %s to %s",
- tmp_path->data, path->data);
- else
- return SVN_NO_ERROR;
+ return err;
}
return SVN_NO_ERROR;
@@ -767,8 +774,13 @@
int sync,
apr_pool_t *pool)
{
- return close_adm_file (handle, path, NULL, sync, pool,
- SVN_WC__ADM_AUTH_DIR, file->data, NULL);
+ svn_stringbuf_t *full_path = svn_stringbuf_dup(path, pool);
+ SVN_ERR (close_adm_file (handle, path, NULL, sync, pool,
+ SVN_WC__ADM_AUTH_DIR, file->data, NULL));
+ extend_with_adm_name (full_path, NULL, FALSE, pool, SVN_WC__ADM_AUTH_DIR,
+ file->data, NULL);
+ SVN_ERR (svn_io_set_permissions (full_path, APR_UREAD, FALSE, pool));
+ return SVN_NO_ERROR;
}
svn_error_t *
@@ -966,9 +978,18 @@
components_added = v_extend_with_adm_name (path, NULL, 0, pool, ap);
va_end (ap);
- apr_err = apr_file_remove (path->data, pool);
- if (apr_err)
- err = svn_error_create (apr_err, 0, NULL, pool, path->data);
+ /* Make sure we have permission to remove the file. */
+ err = svn_io_set_permissions (path, APR_UREAD | APR_UWRITE, FALSE, pool);
+
+ if (!err || APR_STATUS_IS_ENOENT (err->apr_err))
+ {
+ /* It's okay if we couldn't change permissions because the file
+ * didn't exist, since all we wanted to do was remove it. */
+ err = NULL;
+ apr_err = apr_file_remove (path->data, pool);
+ if (apr_err)
+ err = svn_error_create (apr_err, 0, NULL, pool, path->data);
+ }
/* Restore path to its original state no matter what. */
chop_admin_name (path, components_added);
Index: ./subversion/libsvn_wc/get_editor.c
===================================================================
--- ./subversion/libsvn_wc/.svn/text-base/get_editor.c.svn-base Mon Nov 26 09:35:19 2001
+++ ./subversion/libsvn_wc/get_editor.c Tue Nov 27 09:42:39 2001
@@ -1155,6 +1155,9 @@
tmp_txtb,
SVN_WC__LOG_ATTR_DEST,
txtb,
+ SVN_WC__LOG_ATTR_PERM,
+ svn_stringbuf_create
+ (SVN_WC__LOG_PERM_READONLY, fb->pool),
NULL);
if ( (! is_locally_modified)
@@ -1171,6 +1174,9 @@
txtb,
SVN_WC__LOG_ATTR_DEST,
fb->name,
+ SVN_WC__LOG_ATTR_PERM,
+ svn_stringbuf_create
+ (SVN_WC__LOG_PERM_DEFAULT, fb->pool),
NULL);
}
@@ -1198,6 +1204,10 @@
txtb,
SVN_WC__LOG_ATTR_DEST,
fb->name,
+ SVN_WC__LOG_ATTR_PERM,
+ svn_stringbuf_create
+ (SVN_WC__LOG_PERM_DEFAULT,
+ fb->pool),
NULL);
}
else /* working file exists */
@@ -1411,6 +1421,9 @@
fb->name,
SVN_WC__LOG_ATTR_DEST,
renamed_basename,
+ SVN_WC__LOG_ATTR_PERM,
+ svn_stringbuf_create
+ (SVN_WC__LOG_PERM_SRC, fb->pool),
NULL);
/* Copy the new file out into working area. */
@@ -1422,6 +1435,9 @@
txtb,
SVN_WC__LOG_ATTR_DEST,
fb->name,
+ SVN_WC__LOG_ATTR_PERM,
+ svn_stringbuf_create
+ (SVN_WC__LOG_PERM_DEFAULT, fb->pool),
NULL);
}
}
Index: ./subversion/libsvn_subr/io.c
===================================================================
--- ./subversion/libsvn_subr/.svn/text-base/io.c.svn-base Wed Nov 21 12:45:58 2001
+++ ./subversion/libsvn_subr/io.c Wed Nov 28 12:31:56 2001
@@ -342,7 +342,8 @@
svn_error_t *
-svn_io_append_file (svn_stringbuf_t *src, svn_stringbuf_t *dst, apr_pool_t *pool)
+svn_io_append_file (svn_stringbuf_t *src, svn_stringbuf_t *dst,
+ apr_pool_t *pool)
{
apr_status_t apr_err;
@@ -358,8 +359,45 @@
return SVN_NO_ERROR;
}
+svn_error_t *
+svn_io_rename_file (svn_stringbuf_t *src, svn_stringbuf_t *dst,
+ apr_pool_t *pool)
+{
+ apr_status_t apr_err = apr_file_rename (src->data, dst->data, pool);
+ if (apr_err)
+ return svn_error_createf (apr_err, 0, NULL, pool,
+ "Couldn't rename `%s' to `%s'.",
+ src->data, dst->data);
+ return SVN_NO_ERROR;
+}
+svn_error_t *
+svn_io_set_permissions (svn_stringbuf_t *path, apr_fileperms_t perms,
+ svn_boolean_t apply_umask, apr_pool_t *pool)
+{
+ apr_status_t apr_err;
+#if (!defined(OS2) && !defined(BEOS) && !defined(WIN32) && !defined(NETWARE))
+ if (apply_umask)
+ {
+ apr_fileperms_t mask = apr_unix_mode2perms (umask (0777));
+ umask (apr_unix_perms2mode (mask));
+ /* In order to apply the mask, we need to have an or of flags. For
+ * some reason, apr's APR_OS_DEFAULT is not an or of flags, but
+ * rather a special value. Convert it to its meaning here. */
+ if (perms == APR_OS_DEFAULT)
+ perms = APR_UREAD | APR_UWRITE | APR_GREAD | APR_GWRITE | APR_WREAD
+ | APR_WWRITE;
+ perms = perms & ~mask;
+ }
+#endif
+ apr_err = apr_file_perms_set (path->data, perms);
+ if (apr_err && !APR_STATUS_IS_EINCOMPLETE (apr_err)
+ && !APR_STATUS_IS_ENOTIMPL (apr_err))
+ return svn_error_createf (apr_err, 0, NULL, pool, "Could not change "
+ "permissions on `%s'.", path->data);
+ return SVN_NO_ERROR;
+}
svn_error_t *svn_io_copy_dir_recursively (svn_stringbuf_t *src,
svn_stringbuf_t *dst_parent,
--
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Kevin Pilch-Bisson http://www.pilch-bisson.net
"Historically speaking, the presences of wheels in Unix
has never precluded their reinvention." - Larry Wall
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Re: [PATCH] Make working copies read-only.
Posted by Karl Fogel <kf...@newton.ch.collab.net>.
Branko Čibej <br...@xbc.nu> writes:
> Right. I think it's also time somebody (me :-) pointed out that umasks
> and permission bits are a Unix-ism, and what you're doing here does
> not map nicely to (for instance) the read-only bit on Windows
> filesystems. We need a more abstract interface that doesn't mix
> write-protection and file permissions. For example:
>
> apr_file_[get/set]_perms
> apr_file_ [get/set]_read_only
> apr_file_[get/set]_executable
>
> On Unix, all of these would all map to twiddling permission bits. On
> Windows, for instance, the first would touch the file's ACL, the
> second would touch it's read-only bit, and the third is a no-op. And
> Subversion doesn't need (and shouldn't use, IMO) the first at all.
Mmmmm. Excellent thinking. Kevin, how do you feel about doing it
this way?
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Make working copies read-only.
Posted by Branko Čibej <br...@xbc.nu>.
Greg Hudson wrote:
>On Fri, 2001-11-30 at 09:02, Kevin Pilch-Bisson wrote:
>
>>>>Also even if we didn't we wind up with a read-only file in the working copy.
>>>>
>
>We should only wind up with a read-only file in the working copy if we
>open it read-only when we create it or set it read-only afterwards.
>We're doing the copy here; we can open the target file with whatever
>mode we want. If svn_io_copy_file() or apr_copy_file() does the wrong
>thing for this purpose, then we should make it more flexible or create a
>variant; playing games with umask to compensate is going in the wrong
>direction.
>
Right. I think it's also time somebody (me :-) pointed out that umasks
and permission bits are a Unix-ism, and what you're doing here does not
map nicely to (for instance) the read-only bit on Windows filesystems.
We need a more abstract interface that doesn't mix write-protection and
file permissions. For example:
apr_file_[get/set]_perms
apr_file_ [get/set]_read_only
apr_file_[get/set]_executable
On Unix, all of these would all map to twiddling permission bits. On
Windows, for instance, the first would touch the file's ACL, the second
would touch it's read-only bit, and the third is a no-op. And Subversion
doesn't need (and shouldn't use, IMO) the first at all.
--
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: [PATCH] Make working copies read-only.
Posted by Karl Fogel <kf...@newton.ch.collab.net>.
Kevin Pilch-Bisson <ke...@pilch-bisson.net> writes:
> > We're doing the copy here; we can open the target file with whatever
> > mode we want. If svn_io_copy_file() or apr_copy_file() does the wrong
> > thing for this purpose, then we should make it more flexible or create a
> > variant; playing games with umask to compensate is going in the wrong
> > direction.
>
> That's a distinct possibility, and I think what Karl suggested, and I said
> I would try and come up with a patch for.
Yeah, Greg H said it better than I did, I'm glad you also are okay
with it.
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Make working copies read-only.
Posted by Greg Hudson <gh...@MIT.EDU>.
On Fri, 2001-11-30 at 15:44, Karl Fogel wrote:
> Is it necessary to unlink the dst before doing a rename()? That's not
> crash-proof, though in our case it would probably be okay because "svn
> cleanup" would fix it, and "svn cleanup" would be forced because the
> wc would be in a locked state.
Oh, you're right, of course. No need to unlink before rename, even if
the destination file is non-writable.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Make working copies read-only.
Posted by Karl Fogel <kf...@newton.ch.collab.net>.
Greg Hudson <gh...@MIT.EDU> writes:
> Unless I'm terribly mistaken, we shouldn't be doing anything like "cp A
> B" when we move files from tmp/text-base to text-base. We should be
> unlinking the target and calling rename().
>
> If we have to do a copy instead of a rename for some reason (such as
> portability), then we should unlink the target first, if we know we
> might be trying to replace a non-writable file.
Is it necessary to unlink the dst before doing a rename()? That's not
crash-proof, though in our case it would probably be okay because "svn
cleanup" would fix it, and "svn cleanup" would be forced because the
wc would be in a locked state.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Make working copies read-only.
Posted by Greg Hudson <gh...@MIT.EDU>.
On Fri, 2001-11-30 at 10:26, Kevin Pilch-Bisson wrote:
> > We should only wind up with a read-only file in the working copy if we
> > open it read-only when we create it or set it read-only afterwards.
> We don't create it here, we copy it from .svn.
Creation is a step in copying (when the target file doesn't exist). Six
of one, a half-dozen of the other.
> > > I'm afraid we might run into problems when try to mv over files in text-base
> > > from tmp/text-base, since text-base will be read-only. This would
> > > mean we would need to stat the file, make it writable, mv the file,
> > > then set it back to its original perms.
> > There is no problem moving a non-writable file. The only permissions
> > issues for moving are on the source and target directories.
> [...]
> kevin@pilchie:~ [576]$ cp A B
> cp: cannot create regular file `B': Permission denied
> The same thing happens with copies too. It may be possible with the rename()
> system call, but it won't be with the way apr_copy_file is implemented.
Unless I'm terribly mistaken, we shouldn't be doing anything like "cp A
B" when we move files from tmp/text-base to text-base. We should be
unlinking the target and calling rename().
If we have to do a copy instead of a rename for some reason (such as
portability), then we should unlink the target first, if we know we
might be trying to replace a non-writable file.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Make working copies read-only.
Posted by Kevin Pilch-Bisson <ke...@pilch-bisson.net>.
On Fri, Nov 30, 2001 at 10:14:53AM -0500, Greg Hudson wrote:
> On Fri, 2001-11-30 at 09:02, Kevin Pilch-Bisson wrote:
> > > > Also even if we didn't we wind up with a read-only file in the working copy.
>
> We should only wind up with a read-only file in the working copy if we
> open it read-only when we create it or set it read-only afterwards.
We don't create it here, we copy it from .svn.
> We're doing the copy here; we can open the target file with whatever
> mode we want. If svn_io_copy_file() or apr_copy_file() does the wrong
> thing for this purpose, then we should make it more flexible or create a
> variant; playing games with umask to compensate is going in the wrong
> direction.
That's a distinct possibility, and I think what Karl suggested, and I said
I would try and come up with a patch for.
>
> > I'm afraid we might run into problems when try to mv over files in text-base
> > from tmp/text-base, since text-base will be read-only. This would
> > mean we would need to stat the file, make it writable, mv the file,
> > then set it back to its original perms.
>
> There is no problem moving a non-writable file. The only permissions
> issues for moving are on the source and target directories.
>
kevin@pilchie:~ [571]$ ll -d .
drwxr-xr-x 38 kevin users 4.1k Nov 30 10:17 ./
kevin@pilchie:~ [572]$ touch A B
kevin@pilchie:~ [573]$ ll A B
-rw------- 1 kevin users 0 Nov 30 10:18 A
-rw------- 1 kevin users 0 Nov 30 10:18 B
kevin@pilchie:~ [574]$ chmod a-w B
kevin@pilchie:~ [575]$ ll A B
-rw------- 1 kevin users 0 Nov 30 10:18 A
-r-------- 1 kevin users 0 Nov 30 10:18 B
kevin@pilchie:~ [576]$ cp A B
cp: cannot create regular file `B': Permission denied
kevin@pilchie:~ [577]$
The same thing happens with copies too. It may be possible with the rename()
system call, but it won't be with the way apr_copy_file is implemented.
>
> > 1) Put mutexes around the umask calls, AND all calls to open where O_CREAT are
> > specified, or
> >
> > 2) Have apr record the umask before any threads are running, and use that for
> > get operations, and have set record the new value and set the umask.
> >
> >
> > I'm open to either, but I thought that approach 2) causes a fixed amount of
> > memory increase (4bytes), in an apr app, whereas 1) introduces a run-time
> > overhead everytime a file is opened.
>
> Option 2 does, however, impose overhead on all APR applications, even if
> they don't do anything with the umask. So I don't really know. It
> seems sad that Unix doesn't provide a way to get the umask
> nondestructively. Hopefully, though, I'm right above, and we don't need
> to ever get the umask.
Agreed about the overhead, but I don't think a single call to umask, two
functions and a 4 byte variable is a too much.
--
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Kevin Pilch-Bisson http://www.pilch-bisson.net
"Historically speaking, the presences of wheels in Unix
has never precluded their reinvention." - Larry Wall
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Re: [PATCH] Make working copies read-only.
Posted by Branko Čibej <br...@xbc.nu>.
Greg Hudson wrote:
>There is no problem moving a non-writable file. The only permissions
>issues for moving are on the source and target directories.
>
Hah. Another Unix-ism.
--
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: [PATCH] Make working copies read-only.
Posted by Greg Hudson <gh...@MIT.EDU>.
On Fri, 2001-11-30 at 09:02, Kevin Pilch-Bisson wrote:
> > > Also even if we didn't we wind up with a read-only file in the working copy.
We should only wind up with a read-only file in the working copy if we
open it read-only when we create it or set it read-only afterwards.
We're doing the copy here; we can open the target file with whatever
mode we want. If svn_io_copy_file() or apr_copy_file() does the wrong
thing for this purpose, then we should make it more flexible or create a
variant; playing games with umask to compensate is going in the wrong
direction.
> I'm afraid we might run into problems when try to mv over files in text-base
> from tmp/text-base, since text-base will be read-only. This would
> mean we would need to stat the file, make it writable, mv the file,
> then set it back to its original perms.
There is no problem moving a non-writable file. The only permissions
issues for moving are on the source and target directories.
> 1) Put mutexes around the umask calls, AND all calls to open where O_CREAT are
> specified, or
>
> 2) Have apr record the umask before any threads are running, and use that for
> get operations, and have set record the new value and set the umask.
>
>
> I'm open to either, but I thought that approach 2) causes a fixed amount of
> memory increase (4bytes), in an apr app, whereas 1) introduces a run-time
> overhead everytime a file is opened.
Option 2 does, however, impose overhead on all APR applications, even if
they don't do anything with the umask. So I don't really know. It
seems sad that Unix doesn't provide a way to get the umask
nondestructively. Hopefully, though, I'm right above, and we don't need
to ever get the umask.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Make working copies read-only.
Posted by Kevin Pilch-Bisson <ke...@pilch-bisson.net>.
On Fri, Nov 30, 2001 at 12:00:26AM -0600, Karl Fogel wrote:
> Kevin Pilch-Bisson <ke...@pilch-bisson.net> writes:
> > Consider the way an update works if there are no local mods. It copies the
> > text-base file to the tmp area, applies the txdelta, and then moves it back to
> > text-base and copies it out to the working copy. Note that this means it
> > starts with the supposedly read-only text-base file. If we don't change the
> > permissions, then we run into problems moving stuff around. Also even if we
> > didn't we wind up with a read-only file in the working copy. However, if we
> > just set the permissions, without worrying about the umask, then what do we
> > set them to? We could set them -rw-------, but then people have to re-set
> > the permissions to allow group reading every time the do an update. We can't
> > set them -rw-r--r--, because they might contain information that a user doesn't
> > want to be shared. So overall our best strategy is to move or copy the file,
> > then set the permissions to look like we just created the file, which means
> > obeying the umask.
> >
> > There are however, two things I don't like about this patch.
> >
> > 1) It resets a file to non-executable after an update, even if the user had
> > made it executable (e.g. autogen.sh). Not a big deal in the long run, since
> > sometime (hopefully soon), we will know that a file should have the execute
> > bit set, and set it.
>
> Bingo.
>
> What we should do, in the case of copies over existing working files,
> is mimic the permissions of the file being replaced. (Unless the
> update also sets the execute bit via properties or something, but
> that's another story and isn't a concern yet.)
>
First lets go over permission semantics with the unix mv and cp commands.
mv - Permissions are the same as source.
cp - Permissions are the same as dest if dest exists, otherwise as if
file was opened (hence obeys umask).
We could acheive the same semantics, but it would require first stat'ing the
dest file to see if it exists, and storing the permissions if it does.
Our current svn_io_copy_file (and apr_copy_file, since it lives in io.c), set
the permissions to those of the source file regardless. I'll see if I can
work up a patch to implement a read-only working copy based on this, but I'm
afraid we might run into problems when try to mv over files in text-base from
tmp/text-base, since text-base will be read-only. This would mean we would
need to stat the file, make it writable, mv the file, then set it back to
its original perms.
> When creating a new file, then I guess best bet is to pay attention to
> umask and whatever the directory inheritance conventions are (I can't
> remember them off the top of my head, but I think there are some...?)
>
open() should deal with that automatically...
>
>
> > 2) There is a race condition in svn_io_set_permissions for multithreaded
> > apps, with regard to the umask. Just wondering if anyone had any ideas of
> > how to fix that for apr. I was thinking of making:
> >
> > apr_status_t apr_umask_get(apr_fileperms_t *mask);
> >
> > and
> >
> > apr_status_t apr_umask_set(apr_fileperms_t *mask);
> >
> > Then it would be possible to perform a umask(mask=umask(0777)) in
> > apr_initialize (before any threads are created) and store the umask somewhere.
> > Then apr_umask_get would just return the value, and apr_umask_set could set
> > both apr's copy and call umask to set the new value.
> >
> > Sound okay???
>
> Hmm, don't quite understand the bit about the race condition and the
> proposed solution, may need more explanation... What exactly is the
> problem? I thought umask was something APR will be getting from the
> user's environment?
>
umask() is actually a system call, which both sets and returns the current
process umask. Thus the only way to retrieve the umask is to do the
equivalent of: umask(mask=umask(0777)).
It is possible in a multithreaded app to call open between the two umask calls
meaning the file will be created with a bogus umask.
The only safe ways I see around this in creating apr_umask_[get/set] are:
1) Put mutexes around the umask calls, AND all calls to open where O_CREAT are
specified, or
2) Have apr record the umask before any threads are running, and use that for
get operations, and have set record the new value and set the umask.
I'm open to either, but I thought that approach 2) causes a fixed amount of
memory increase (4bytes), in an apr app, whereas 1) introduces a run-time
overhead everytime a file is opened.
--
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Kevin Pilch-Bisson http://www.pilch-bisson.net
"Historically speaking, the presences of wheels in Unix
has never precluded their reinvention." - Larry Wall
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Re: [PATCH] Make working copies read-only.
Posted by Karl Fogel <kf...@newton.ch.collab.net>.
Kevin Pilch-Bisson <ke...@pilch-bisson.net> writes:
> No problem. I did see the stuff there, and I thought it might be possible to
> put it there while waiting for it to go into APR. I'll re-work the patch to
> include a patch to APR as well.
Great, thanks.
> > - I don't understand why we suddenly need to pay attention to umask
> > for this change. The idea is to make the .svn area read-only,
> > except for the tmp area, which doesn't need umask afaik. So if we
> > weren't using umasks before now, why does this change require
> > paying attention to them?
> >
> > This isn't actually a nit, it's a real question -- probably
> > there's a good reason for the umask stuff, and I'm just not
> > thinking of it...
>
> Consider the way an update works if there are no local mods. It copies the
> text-base file to the tmp area, applies the txdelta, and then moves it back to
> text-base and copies it out to the working copy. Note that this means it
> starts with the supposedly read-only text-base file. If we don't change the
> permissions, then we run into problems moving stuff around. Also even if we
> didn't we wind up with a read-only file in the working copy. However, if we
> just set the permissions, without worrying about the umask, then what do we
> set them to? We could set them -rw-------, but then people have to re-set
> the permissions to allow group reading every time the do an update. We can't
> set them -rw-r--r--, because they might contain information that a user doesn't
> want to be shared. So overall our best strategy is to move or copy the file,
> then set the permissions to look like we just created the file, which means
> obeying the umask.
>
> There are however, two things I don't like about this patch.
>
> 1) It resets a file to non-executable after an update, even if the user had
> made it executable (e.g. autogen.sh). Not a big deal in the long run, since
> sometime (hopefully soon), we will know that a file should have the execute
> bit set, and set it.
Bingo.
What we should do, in the case of copies over existing working files,
is mimic the permissions of the file being replaced. (Unless the
update also sets the execute bit via properties or something, but
that's another story and isn't a concern yet.)
When creating a new file, then I guess best bet is to pay attention to
umask and whatever the directory inheritance conventions are (I can't
remember them off the top of my head, but I think there are some...?)
> 2) There is a race condition in svn_io_set_permissions for multithreaded
> apps, with regard to the umask. Just wondering if anyone had any ideas of
> how to fix that for apr. I was thinking of making:
>
> apr_status_t apr_umask_get(apr_fileperms_t *mask);
>
> and
>
> apr_status_t apr_umask_set(apr_fileperms_t *mask);
>
> Then it would be possible to perform a umask(mask=umask(0777)) in
> apr_initialize (before any threads are created) and store the umask somewhere.
> Then apr_umask_get would just return the value, and apr_umask_set could set
> both apr's copy and call umask to set the new value.
>
> Sound okay???
Hmm, don't quite understand the bit about the race condition and the
proposed solution, may need more explanation... What exactly is the
problem? I thought umask was something APR will be getting from the
user's environment?
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Make working copies read-only.
Posted by Kevin Pilch-Bisson <ke...@pilch-bisson.net>.
On Fri, Nov 30, 2001 at 12:26:23AM -0800, Greg Stein wrote:
> On Thu, Nov 29, 2001 at 10:57:00PM -0500, Kevin Pilch-Bisson wrote:
> >...
> > > > +++ ./subversion/libsvn_wc/get_editor.c Tue Nov 27 09:42:39 2001
> > > > @@ -1155,6 +1155,9 @@
> > > > tmp_txtb,
> > > > SVN_WC__LOG_ATTR_DEST,
> > > > txtb,
> > > > + SVN_WC__LOG_ATTR_PERM,
> > > > + svn_stringbuf_create
> > > > + (SVN_WC__LOG_PERM_READONLY, fb->pool),
> > > > NULL);
> > > >
> > > > if ( (! is_locally_modified)
> > > > @@ -1171,6 +1174,9 @@
> > > > txtb,
> > > > SVN_WC__LOG_ATTR_DEST,
> > > > fb->name,
> > > > + SVN_WC__LOG_ATTR_PERM,
> > > > + svn_stringbuf_create
> > > > + (SVN_WC__LOG_PERM_DEFAULT, fb->pool),
> > > > NULL);
> > > > }
> > > >
> > > > @@ -1198,6 +1204,10 @@
> > > > txtb,
> > > > SVN_WC__LOG_ATTR_DEST,
> > > > fb->name,
> > > > + SVN_WC__LOG_ATTR_PERM,
> > > > + svn_stringbuf_create
> > > > + (SVN_WC__LOG_PERM_DEFAULT,
> > > > + fb->pool),
> > > > NULL);
> > > > }
> > > > else /* working file exists */
> > > > @@ -1411,6 +1421,9 @@
> > > > fb->name,
> > > > SVN_WC__LOG_ATTR_DEST,
> > > > renamed_basename,
> > > > + SVN_WC__LOG_ATTR_PERM,
> > > > + svn_stringbuf_create
> > > > + (SVN_WC__LOG_PERM_SRC, fb->pool),
> > > > NULL);
> > > >
> > > > /* Copy the new file out into working area. */
> > > > @@ -1422,6 +1435,9 @@
> > > > txtb,
> > > > SVN_WC__LOG_ATTR_DEST,
> > > > fb->name,
> > > > + SVN_WC__LOG_ATTR_PERM,
> > > > + svn_stringbuf_create
> > > > + (SVN_WC__LOG_PERM_DEFAULT, fb->pool),
> > > > NULL);
>
> Holy crap, that is some major suckage. Allocate a heap buffer so that we can
> put a constant string into the output XML? Blearg.
>
> I guess the one saving grace is that it isn't within a loop, and fb->pool is
> about to be destroyed. But sheesh... what convolutions for an otherwise
> simple task :-(
>
Agreed. One possibility is to investigate the usage of svn_xml_make_open_tag,
and see whether it needs to be taking an svn_stringbuf, or could in fact
take an svn_string_t or const char *.
I didn't think this patch was the place to go into that though.
--
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Kevin Pilch-Bisson http://www.pilch-bisson.net
"Historically speaking, the presences of wheels in Unix
has never precluded their reinvention." - Larry Wall
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Re: [PATCH] Make working copies read-only.
Posted by Greg Hudson <gh...@MIT.EDU>.
On Fri, 2001-11-30 at 03:26, Greg Stein wrote:
> Holy crap, that is some major suckage. Allocate a heap buffer so that we can
> put a constant string into the output XML? Blearg.
> I guess the one saving grace is that it isn't within a loop, and fb->pool is
> about to be destroyed. But sheesh... what convolutions for an otherwise
> simple task :-(
This is going to sound rude and patronizing, especially since you
probably have at least as much experience as I do, but I don't have a
better way of saying it: I think you need to learn to care less about
extra operations like this. Often, uniformity of interface is more
important than making every function of a program use the few possible
number of operations. (Obviously, there are exceptions: when the extra
operations are causing a real, observed performance problem, or are
making parts of the code difficult to maintain, then something has to be
done.)
That said, given the conventions we've adopted, I strongly suspect the
function involved should be taking a const char * instead of an
svn_stringbuf_t *. (An XML attribute cannot contain 0 bytes, so
svn_string_t * doesn't buy us anything over const char *.) So my
disagreement is in your calling it "major suckage," not in the proper
course of action.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Make working copies read-only.
Posted by Greg Stein <gs...@lyra.org>.
On Thu, Nov 29, 2001 at 10:57:00PM -0500, Kevin Pilch-Bisson wrote:
>...
> > > +++ ./subversion/libsvn_wc/get_editor.c Tue Nov 27 09:42:39 2001
> > > @@ -1155,6 +1155,9 @@
> > > tmp_txtb,
> > > SVN_WC__LOG_ATTR_DEST,
> > > txtb,
> > > + SVN_WC__LOG_ATTR_PERM,
> > > + svn_stringbuf_create
> > > + (SVN_WC__LOG_PERM_READONLY, fb->pool),
> > > NULL);
> > >
> > > if ( (! is_locally_modified)
> > > @@ -1171,6 +1174,9 @@
> > > txtb,
> > > SVN_WC__LOG_ATTR_DEST,
> > > fb->name,
> > > + SVN_WC__LOG_ATTR_PERM,
> > > + svn_stringbuf_create
> > > + (SVN_WC__LOG_PERM_DEFAULT, fb->pool),
> > > NULL);
> > > }
> > >
> > > @@ -1198,6 +1204,10 @@
> > > txtb,
> > > SVN_WC__LOG_ATTR_DEST,
> > > fb->name,
> > > + SVN_WC__LOG_ATTR_PERM,
> > > + svn_stringbuf_create
> > > + (SVN_WC__LOG_PERM_DEFAULT,
> > > + fb->pool),
> > > NULL);
> > > }
> > > else /* working file exists */
> > > @@ -1411,6 +1421,9 @@
> > > fb->name,
> > > SVN_WC__LOG_ATTR_DEST,
> > > renamed_basename,
> > > + SVN_WC__LOG_ATTR_PERM,
> > > + svn_stringbuf_create
> > > + (SVN_WC__LOG_PERM_SRC, fb->pool),
> > > NULL);
> > >
> > > /* Copy the new file out into working area. */
> > > @@ -1422,6 +1435,9 @@
> > > txtb,
> > > SVN_WC__LOG_ATTR_DEST,
> > > fb->name,
> > > + SVN_WC__LOG_ATTR_PERM,
> > > + svn_stringbuf_create
> > > + (SVN_WC__LOG_PERM_DEFAULT, fb->pool),
> > > NULL);
Holy crap, that is some major suckage. Allocate a heap buffer so that we can
put a constant string into the output XML? Blearg.
I guess the one saving grace is that it isn't within a loop, and fb->pool is
about to be destroyed. But sheesh... what convolutions for an otherwise
simple task :-(
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Make working copies read-only.
Posted by Kevin Pilch-Bisson <ke...@pilch-bisson.net>.
On Thu, Nov 29, 2001 at 05:10:06PM -0600, Karl Fogel wrote:
> Kevin, thanks for taking this on! A couple of nits/questions:
>
> - No OS-specific code in Subversion itself -- use APR for that. So
> your new svn_io_set_permissions() needs to be changed. Note that
> it's okay for a patch to include changes to APR; we have a fair
> number of APR committers here, no worries.
>
> Now, you probably saw the `WIN32' stuff in config.c and io.c in
> the same directory and said to yourself "Hey, if it was okay
> there, it'll be okay in my new function.", which is
> understandable. :-) However, that old stuff is equally
> blasphemous, and will be removed ASAP (Greg Stein just noticed the
> io.c stuff, and plans to deal with it by not using file
> descriptors with Neon at all, which may require some tweaks to
> Neon, but that's fine).
>
> So it's still: no OS-sensitive code in Subversion. APR is our
> portability layer.
>
No problem. I did see the stuff there, and I thought it might be possible to
put it there while waiting for it to go into APR. I'll re-work the patch to
include a patch to APR as well.
>
> - I don't understand why we suddenly need to pay attention to umask
> for this change. The idea is to make the .svn area read-only,
> except for the tmp area, which doesn't need umask afaik. So if we
> weren't using umasks before now, why does this change require
> paying attention to them?
>
> This isn't actually a nit, it's a real question -- probably
> there's a good reason for the umask stuff, and I'm just not
> thinking of it...
Consider the way an update works if there are no local mods. It copies the
text-base file to the tmp area, applies the txdelta, and then moves it back to
text-base and copies it out to the working copy. Note that this means it
starts with the supposedly read-only text-base file. If we don't change the
permissions, then we run into problems moving stuff around. Also even if we
didn't we wind up with a read-only file in the working copy. However, if we
just set the permissions, without worrying about the umask, then what do we
set them to? We could set them -rw-------, but then people have to re-set
the permissions to allow group reading every time the do an update. We can't
set them -rw-r--r--, because they might contain information that a user doesn't
want to be shared. So overall our best strategy is to move or copy the file,
then set the permissions to look like we just created the file, which means
obeying the umask.
There are however, two things I don't like about this patch.
1) It resets a file to non-executable after an update, even if the user had
made it executable (e.g. autogen.sh). Not a big deal in the long run, since
sometime (hopefully soon), we will know that a file should have the execute
bit set, and set it.
2) There is a race condition in svn_io_set_permissions for multithreaded
apps, with regard to the umask. Just wondering if anyone had any ideas of
how to fix that for apr. I was thinking of making:
apr_status_t apr_umask_get(apr_fileperms_t *mask);
and
apr_status_t apr_umask_set(apr_fileperms_t *mask);
Then it would be possible to perform a umask(mask=umask(0777)) in
apr_initialize (before any threads are created) and store the umask somewhere.
Then apr_umask_get would just return the value, and apr_umask_set could set
both apr's copy and call umask to set the new value.
Sound okay???
>
> Best,
> -Karl
>
> Kevin Pilch-Bisson <ke...@pilch-bisson.net> writes:
> > Subject and log message say it all.
> >
> > Log:
> > Make the .svn area read-only, with the exception of things in tmp, which
> > will be writable. This will have no effect on platforms other unix, since
> > apr_file_open does not obey the permission flags, and apr_file_perms_set
> > is not implemented.
> >
> > This patch is safe to apply to an already existing working copy. It will
> > gradually make file read-only as they are updated or committed.
> >
> > * subversion/include/svn_io.h:(svn_io_rename_file): New prototype for a
> > wrapper around apr_file_rename;
> > (svn_io_set_permissions): New prototype for a function which is able to
> > set the permissions in a matter that obeys a user's umask.
> >
> > * subversion/libsvn_subr/io.c:(svn_io_append_file): Format for 80 column
> > lines.
> > (svn_io_rename_file): New function, a wrapper around apr_file_rename.
> > (svn_io_set_permissions): New function, on unix this is able to retreive
> > the user's umask and set the file as if it were newly created. On other
> > platforms this does nothing since apr_file_perms_set is not implemented
> > on any other platform.
> >
> > * subversion/libsvn_wc/wc.h:Doc chanes for SVN_LOG_ATTR_MV and
> > SVN_LOG_ATTR_CP.
> > (SVN_WC__LOG_ATTR_PERM): New define, use to indicate what permissions a
> > move, copy, or append action should set the dest file to.
> > (SVN_WC__LOG_PERM_SRC): New define, don't change permissions.
> > (SVN_WC__LOG_DEFAULT): New define, use os-default permissions.
> > (SVN_WC__LOG_EXEC): New define, make file executable(not currently used,
> > but might be useful for making checkouts/updates recognize executable
> > files.
> > (SVN_WC__LOG_READONLY): New define, make file read-only.
> >
> > * subversion/libsvn_wc/props.c:(svn_wc__do_property_merge): Pass
> > permissions to log entries.
> >
> > * subversion/libsvn_wc/adm_crawler.c:(restore_file): Make files writable
> > when copying them out to the working copy.
> >
> > * subversion/libsvn_wc/log.c:(svn_wc__perm_type): New enum.
> > (file_xfer_under_path) Takes new `perm' argument, sets dest file
> > permissions according to it.
> > (log_do_file_xfer) Determine perm argument for file_xfer_under_path from
> > the xml attributes.
> >
> > * subversion/libsvn_wc/adm_files.c:(maybe_copy_file): Set permissions when
> > copying the file to the tmp area.
> > (sync_adm_file): Set permissions on the file when moving it out of the
> > tmp area.
> > (close_adm_file): Set permissions on the file when moving it
> > out of the tmp area.
> > (svn_wc__close_auth_file): Make the file readable only by the user when
> > closing it.
> > (svn_wc__remove_adm_file): Make the file writable before removing it.
> >
> > * subversion/libsvn_wc/get_editor.c:(close_file): Pass permissions to be
> > used to log entries.
> >
> > Index: ./subversion/include/svn_io.h
> > ===================================================================
> > --- ./subversion/include/.svn/text-base/svn_io.h.svn-base Wed Nov 21 12:45:58 2001
> > +++ ./subversion/include/svn_io.h Tue Nov 27 13:47:16 2001
> > @@ -129,6 +129,19 @@
> > svn_stringbuf_t *dst,
> > apr_pool_t *pool);
> >
> > +/* Rename SRC to DST. DST will be overwritten if it exists. */
> > +svn_error_t *svn_io_rename_file (svn_stringbuf_t *src,
> > + svn_stringbuf_t *dst,
> > + apr_pool_t *pool);
> > +
> > +/* Change the permissions on PATH to PERMS. If APPLY_UMASK is true then
> > + * the permissions will be combined with the umask in the same way as
> > + * open, if the platform permits. Otherwise, the permissions will be set
> > + * to exactly PERMS. */
> > +svn_error_t *svn_io_set_permissions (svn_stringbuf_t *path,
> > + apr_fileperms_t perms,
> > + svn_boolean_t apply_umask,
> > + apr_pool_t *pool);
> >
> > /* Read a line from FILE into BUF, but not exceeding *LIMIT bytes.
> > * Does not include newline, instead '\0' is put there.
> > Index: ./subversion/libsvn_wc/props.c
> > ===================================================================
> > --- ./subversion/libsvn_wc/.svn/text-base/props.c.svn-base Wed Nov 21 11:22:30 2001
> > +++ ./subversion/libsvn_wc/props.c Wed Nov 28 12:25:44 2001
> > @@ -620,6 +620,8 @@
> > tmp_prop_base,
> > SVN_WC__LOG_ATTR_DEST,
> > real_prop_base,
> > + SVN_WC__LOG_ATTR_PERM,
> > + svn_stringbuf_create (SVN_WC__LOG_PERM_SRC, pool),
> > NULL);
> >
> > /* Write log entry to move working tmp copy to real working area. */
> > @@ -631,6 +633,8 @@
> > tmp_props,
> > SVN_WC__LOG_ATTR_DEST,
> > real_props,
> > + SVN_WC__LOG_ATTR_PERM,
> > + svn_stringbuf_create (SVN_WC__LOG_PERM_SRC, pool),
> > NULL);
> >
> > if (reject_tmp_fp)
> > Index: ./subversion/libsvn_wc/wc.h
> > ===================================================================
> > --- ./subversion/libsvn_wc/.svn/text-base/wc.h.svn-base Mon Nov 26 09:35:17 2001
> > +++ ./subversion/libsvn_wc/wc.h Wed Nov 28 12:26:52 2001
> > @@ -373,10 +373,12 @@
> > */
> > #define SVN_WC__LOG_RUN_CMD "run"
> >
> > -/* Move file SVN_WC__LOG_ATTR_NAME to SVN_WC__LOG_ATTR_DEST. */
> > +/* Move file SVN_WC__LOG_ATTR_NAME to SVN_WC__LOG_ATTR_DEST, with
> > + * permissions SVN_WC__LOG_ATTR_PERM. */
> > #define SVN_WC__LOG_MV "mv"
> >
> > -/* Copy file SVN_WC__LOG_ATTR_NAME to SVN_WC__LOG_ATTR_DEST. */
> > +/* Copy file SVN_WC__LOG_ATTR_NAME to SVN_WC__LOG_ATTR_DEST, with
> > + * permissions SVN_WC__LOG_ATTR_PERM. */
> > #define SVN_WC__LOG_CP "cp"
> >
> > /* Remove file SVN_WC__LOG_ATTR_NAME. */
> > @@ -415,6 +417,7 @@
> > #define SVN_WC__LOG_ATTR_NAME "name"
> > #define SVN_WC__LOG_ATTR_DEST "dest"
> > #define SVN_WC__LOG_ATTR_REVISION "revision"
> > +#define SVN_WC__LOG_ATTR_PERM "permission"
> > #define SVN_WC__LOG_ATTR_TEXT_REJFILE "text-rejfile"
> > #define SVN_WC__LOG_ATTR_PROP_REJFILE "prop-rejfile"
> > /* The rest are for SVN_WC__LOG_RUN_CMD. Extend as necessary. */
> > @@ -430,6 +433,15 @@
> > #define SVN_WC__LOG_ATTR_ARG_7 "arg7"
> > #define SVN_WC__LOG_ATTR_ARG_8 "arg8"
> > #define SVN_WC__LOG_ATTR_ARG_9 "arg9"
> > +
> > +/* Keep permissions the same as the source file. */
> > +#define SVN_WC__LOG_PERM_SRC "src"
> > +/* Make the permissions the operating system's default. */
> > +#define SVN_WC__LOG_PERM_DEFAULT "default"
> > +/* Make the permissions os/default, but with the execute bit set. */
> > +#define SVN_WC__LOG_PERM_EXEC "executable"
> > +/* Make the permissions read-only. */
> > +#define SVN_WC__LOG_PERM_READONLY "read-only"
> >
> >
> > /* Starting at PATH, write out log entries indicating that a commit
> > Index: ./subversion/libsvn_wc/adm_crawler.c
> > ===================================================================
> > --- ./subversion/libsvn_wc/.svn/text-base/adm_crawler.c.svn-base Mon Nov 26 09:35:17 2001
> > +++ ./subversion/libsvn_wc/adm_crawler.c Tue Nov 27 14:09:04 2001
> > @@ -1795,12 +1795,9 @@
> > tmp_text_base_path = svn_wc__text_base_path (file_path, 1, pool);
> >
> > SVN_ERR (svn_io_copy_file (text_base_path, tmp_text_base_path, pool));
> > - status = apr_file_rename (tmp_text_base_path->data, file_path->data, pool);
> > - if (status)
> > - return svn_error_createf (status, 0, NULL, pool,
> > - "error renaming `%s' to `%s'",
> > - tmp_text_base_path->data,
> > - file_path->data);
> > + SVN_ERR (svn_io_set_permissions (tmp_text_base_path, APR_OS_DEFAULT,
> > + TRUE, pool));
> > + SVN_ERR (svn_io_rename_file (tmp_text_base_path, file_path, pool));
> >
> > return SVN_NO_ERROR;
> > }
> > Index: ./subversion/libsvn_wc/log.c
> > ===================================================================
> > --- ./subversion/libsvn_wc/.svn/text-base/log.c.svn-base Wed Nov 21 08:50:24 2001
> > +++ ./subversion/libsvn_wc/log.c Tue Nov 27 14:21:19 2001
> > @@ -51,18 +51,27 @@
> > svn_wc__xfer_mv,
> > };
> >
> > +enum svn_wc__perm_type {
> > + svn_wc__perm_src,
> > + svn_wc__perm_default,
> > + svn_wc__perm_exec,
> > + svn_wc__perm_readonly
> > +};
> >
> >
> > -/* Copy (or rename, if RENAME is non-zero) NAME to DEST, assuming that
> > - PATH is the common parent of both locations. */
> > +/* Transfer contents of NAME to DEST, assuming that PATH is the common
> > + parent of both locations, according to ACTION. If ACTION requires that
> > + DEST be created, give it permissions appropriate to PERM. */
> > static svn_error_t *
> > file_xfer_under_path (svn_stringbuf_t *path,
> > const char *name,
> > const char *dest,
> > enum svn_wc__xfer_action action,
> > + enum svn_wc__perm_type perm,
> > apr_pool_t *pool)
> > {
> > apr_status_t status;
> > + apr_fileperms_t fperms;
> > svn_stringbuf_t *full_from_path, *full_dest_path;
> >
> > full_from_path = svn_stringbuf_dup (path, pool);
> > @@ -71,22 +80,39 @@
> > svn_path_add_component_nts (full_from_path, name, svn_path_local_style);
> > svn_path_add_component_nts (full_dest_path, dest, svn_path_local_style);
> >
> > + switch (perm)
> > + {
> > + case svn_wc__perm_default:
> > + fperms = APR_OS_DEFAULT;
> > + break;
> > + case svn_wc__perm_exec:
> > + fperms = APR_UREAD | APR_GREAD | APR_WREAD | APR_UWRITE
> > + | APR_GWRITE | APR_WWRITE | APR_UEXECUTE | APR_GEXECUTE
> > + | APR_WEXECUTE;
> > + break;
> > + case svn_wc__perm_src:
> > + /* Doesn't matter, since the permissions won't be applied anyway. */
> > + break;
> > + case svn_wc__perm_readonly:
> > + fperms = APR_UREAD | APR_GREAD | APR_WREAD;
> > + }
> > +
> > switch (action)
> > {
> > case svn_wc__xfer_append:
> > - return svn_io_append_file (full_from_path, full_dest_path, pool);
> > + SVN_ERR (svn_io_append_file (full_from_path, full_dest_path, pool));
> > + break;
> > case svn_wc__xfer_cp:
> > - return svn_io_copy_file (full_from_path, full_dest_path, pool);
> > + SVN_ERR (svn_io_copy_file (full_from_path, full_dest_path, pool));
> > + break;
> > case svn_wc__xfer_mv:
> > - status = apr_file_rename (full_from_path->data,
> > - full_dest_path->data, pool);
> > - if (status)
> > - return svn_error_createf (status, 0, NULL, pool,
> > - "file_xfer_under_path: "
> > - "can't move %s to %s",
> > - name, dest);
> > + SVN_ERR (svn_io_rename_file (full_from_path, full_dest_path, pool));
> > + break;
> > }
> >
> > + if (perm != svn_wc__perm_src)
> > + SVN_ERR (svn_io_set_permissions (full_dest_path, fperms, TRUE, pool));
> > +
> > return SVN_NO_ERROR;
> > }
> >
> > @@ -238,15 +264,34 @@
> > const XML_Char **atts)
> > {
> > svn_error_t *err;
> > + enum svn_wc__perm_type perm = svn_wc__perm_src;
> > const char *dest = svn_xml_get_attr_value (SVN_WC__LOG_ATTR_DEST, atts);
> > + const char *permstr = svn_xml_get_attr_value (SVN_WC__LOG_ATTR_PERM, atts);
> >
> > if (! dest)
> > return svn_error_createf (SVN_ERR_WC_BAD_ADM_LOG, 0, NULL, loggy->pool,
> > "missing dest attr in %s", loggy->path->data);
> >
> > - /* Else. */
> > + if (permstr)
> > + {
> > + if (strcmp (permstr, SVN_WC__LOG_PERM_SRC) == 0)
> > + perm = svn_wc__perm_src;
> > + else if (strcmp (permstr, SVN_WC__LOG_PERM_DEFAULT) == 0)
> > + perm = svn_wc__perm_default;
> > + else if (strcmp (permstr, SVN_WC__LOG_PERM_EXEC) == 0)
> > + perm = svn_wc__perm_exec;
> > + else if (strcmp (permstr, SVN_WC__LOG_PERM_READONLY) == 0)
> > + perm = svn_wc__perm_readonly;
> > + else
> > + return svn_error_createf (SVN_ERR_WC_BAD_ADM_LOG, 0, NULL,
> > + loggy->pool,
> > + "unrecognized perm attr in %s",
> > + loggy->path->data);
> > + }
> >
> > - err = file_xfer_under_path (loggy->path, name, dest, action, loggy->pool);
> > + /* Else. */
> > + err = file_xfer_under_path (loggy->path, name, dest, action,
> > + perm, loggy->pool);
> > if (err)
> > signal_error (loggy, err);
> >
> > Index: ./subversion/libsvn_wc/adm_ops.c
> > ===================================================================
> > --- ./subversion/libsvn_wc/.svn/text-base/adm_ops.c.svn-base Mon Nov 26 09:35:18 2001
> > +++ ./subversion/libsvn_wc/adm_ops.c Wed Nov 28 13:05:32 2001
> > @@ -911,6 +911,9 @@
> > (err->apr_err, 0, NULL, pool,
> > "revert_admin_things: Error restoring pristine text for '%s'",
> > full_path->data);
> > + SVN_ERR (svn_io_set_permissions (full_path,
> > + APR_OS_DEFAULT,
> > + TRUE, pool));
> > SVN_ERR (svn_io_file_affected_time (&tstamp, full_path, pool));
> >
> > /* Modify our entry structure. */
> > Index: ./subversion/libsvn_wc/adm_files.c
> > ===================================================================
> > --- ./subversion/libsvn_wc/.svn/text-base/adm_files.c.svn-base Mon Nov 26 09:35:18 2001
> > +++ ./subversion/libsvn_wc/adm_files.c Wed Nov 28 13:10:44 2001
> > @@ -289,11 +289,10 @@
> > return SVN_NO_ERROR;
> > }
> > }
> > - else /* SRC exists, so copy it to DST. */
> > + else /* SRC exists, so copy it to DST, and make sure we can write to it. */
> > {
> > - err = svn_io_copy_file (src, dst, pool);
> > - if (err)
> > - return err;
> > + SVN_ERR (svn_io_copy_file (src, dst, pool));
> > + SVN_ERR (svn_io_set_permissions (dst, APR_OS_DEFAULT, TRUE, pool));
> > }
> >
> > return SVN_NO_ERROR;
> > @@ -313,7 +312,7 @@
> > given how C va_lists work. */
> >
> > svn_stringbuf_t *tmp_path = svn_stringbuf_dup (path, pool);
> > - apr_status_t apr_err;
> > + svn_error_t *err;
> > int components_added;
> > va_list ap;
> >
> > @@ -327,18 +326,22 @@
> > v_extend_with_adm_name (tmp_path, extension, 1, pool, ap);
> > va_end (ap);
> >
> > - /* Rename. */
> > - apr_err = apr_file_rename (tmp_path->data, path->data, pool);
> > + /* Rename. 1) Make write-able, rename, make read-only. */
> > + err = svn_io_set_permissions (path, APR_UREAD | APR_UWRITE, FALSE, pool);
> > + if (!err || APR_STATUS_IS_ENOENT (err->apr_err))
> > + {
> > + err = svn_io_rename_file (tmp_path, path, pool);
> > + if (!err)
> > + err = svn_io_set_permissions (path,
> > + APR_UREAD | APR_GREAD | APR_WREAD,
> > + TRUE, pool);
> > + }
> > +
> >
> > /* Unconditionally restore path. */
> > chop_admin_name (path, components_added);
> >
> > - if (apr_err)
> > - return svn_error_createf (apr_err, 0, NULL, pool,
> > - "error renaming %s to %s",
> > - tmp_path->data, path->data);
> > - else
> > - return SVN_NO_ERROR;
> > + return err;
> > }
> >
> >
> > @@ -635,6 +638,7 @@
> > ...)
> > {
> > apr_status_t apr_err = 0;
> > + svn_error_t *err;
> > int components_added;
> > va_list ap;
> >
> > @@ -672,17 +676,20 @@
> > va_end (ap);
> >
> > /* Rename. */
> > - apr_err = apr_file_rename (tmp_path->data, path->data, pool);
> > + err = svn_io_set_permissions (path, APR_UREAD | APR_UWRITE, FALSE, pool);
> > + if (!err || APR_STATUS_IS_ENOENT (err->apr_err))
> > + {
> > + err = svn_io_rename_file (tmp_path, path, pool);
> > + if (!err)
> > + err = svn_io_set_permissions (path,
> > + APR_UREAD | APR_GREAD | APR_WREAD,
> > + TRUE, pool);
> > + }
> >
> > /* Unconditionally restore path. */
> > chop_admin_name (path, components_added);
> >
> > - if (apr_err)
> > - return svn_error_createf (apr_err, 0, NULL, pool,
> > - "error renaming %s to %s",
> > - tmp_path->data, path->data);
> > - else
> > - return SVN_NO_ERROR;
> > + return err;
> > }
> >
> > return SVN_NO_ERROR;
> > @@ -767,8 +774,13 @@
> > int sync,
> > apr_pool_t *pool)
> > {
> > - return close_adm_file (handle, path, NULL, sync, pool,
> > - SVN_WC__ADM_AUTH_DIR, file->data, NULL);
> > + svn_stringbuf_t *full_path = svn_stringbuf_dup(path, pool);
> > + SVN_ERR (close_adm_file (handle, path, NULL, sync, pool,
> > + SVN_WC__ADM_AUTH_DIR, file->data, NULL));
> > + extend_with_adm_name (full_path, NULL, FALSE, pool, SVN_WC__ADM_AUTH_DIR,
> > + file->data, NULL);
> > + SVN_ERR (svn_io_set_permissions (full_path, APR_UREAD, FALSE, pool));
> > + return SVN_NO_ERROR;
> > }
> >
> > svn_error_t *
> > @@ -966,9 +978,18 @@
> > components_added = v_extend_with_adm_name (path, NULL, 0, pool, ap);
> > va_end (ap);
> >
> > - apr_err = apr_file_remove (path->data, pool);
> > - if (apr_err)
> > - err = svn_error_create (apr_err, 0, NULL, pool, path->data);
> > + /* Make sure we have permission to remove the file. */
> > + err = svn_io_set_permissions (path, APR_UREAD | APR_UWRITE, FALSE, pool);
> > +
> > + if (!err || APR_STATUS_IS_ENOENT (err->apr_err))
> > + {
> > + /* It's okay if we couldn't change permissions because the file
> > + * didn't exist, since all we wanted to do was remove it. */
> > + err = NULL;
> > + apr_err = apr_file_remove (path->data, pool);
> > + if (apr_err)
> > + err = svn_error_create (apr_err, 0, NULL, pool, path->data);
> > + }
> >
> > /* Restore path to its original state no matter what. */
> > chop_admin_name (path, components_added);
> > Index: ./subversion/libsvn_wc/get_editor.c
> > ===================================================================
> > --- ./subversion/libsvn_wc/.svn/text-base/get_editor.c.svn-base Mon Nov 26 09:35:19 2001
> > +++ ./subversion/libsvn_wc/get_editor.c Tue Nov 27 09:42:39 2001
> > @@ -1155,6 +1155,9 @@
> > tmp_txtb,
> > SVN_WC__LOG_ATTR_DEST,
> > txtb,
> > + SVN_WC__LOG_ATTR_PERM,
> > + svn_stringbuf_create
> > + (SVN_WC__LOG_PERM_READONLY, fb->pool),
> > NULL);
> >
> > if ( (! is_locally_modified)
> > @@ -1171,6 +1174,9 @@
> > txtb,
> > SVN_WC__LOG_ATTR_DEST,
> > fb->name,
> > + SVN_WC__LOG_ATTR_PERM,
> > + svn_stringbuf_create
> > + (SVN_WC__LOG_PERM_DEFAULT, fb->pool),
> > NULL);
> > }
> >
> > @@ -1198,6 +1204,10 @@
> > txtb,
> > SVN_WC__LOG_ATTR_DEST,
> > fb->name,
> > + SVN_WC__LOG_ATTR_PERM,
> > + svn_stringbuf_create
> > + (SVN_WC__LOG_PERM_DEFAULT,
> > + fb->pool),
> > NULL);
> > }
> > else /* working file exists */
> > @@ -1411,6 +1421,9 @@
> > fb->name,
> > SVN_WC__LOG_ATTR_DEST,
> > renamed_basename,
> > + SVN_WC__LOG_ATTR_PERM,
> > + svn_stringbuf_create
> > + (SVN_WC__LOG_PERM_SRC, fb->pool),
> > NULL);
> >
> > /* Copy the new file out into working area. */
> > @@ -1422,6 +1435,9 @@
> > txtb,
> > SVN_WC__LOG_ATTR_DEST,
> > fb->name,
> > + SVN_WC__LOG_ATTR_PERM,
> > + svn_stringbuf_create
> > + (SVN_WC__LOG_PERM_DEFAULT, fb->pool),
> > NULL);
> > }
> > }
> > Index: ./subversion/libsvn_subr/io.c
> > ===================================================================
> > --- ./subversion/libsvn_subr/.svn/text-base/io.c.svn-base Wed Nov 21 12:45:58 2001
> > +++ ./subversion/libsvn_subr/io.c Wed Nov 28 12:31:56 2001
> > @@ -342,7 +342,8 @@
> >
> >
> > svn_error_t *
> > -svn_io_append_file (svn_stringbuf_t *src, svn_stringbuf_t *dst, apr_pool_t *pool)
> > +svn_io_append_file (svn_stringbuf_t *src, svn_stringbuf_t *dst,
> > + apr_pool_t *pool)
> > {
> > apr_status_t apr_err;
> >
> > @@ -358,8 +359,45 @@
> > return SVN_NO_ERROR;
> > }
> >
> > +svn_error_t *
> > +svn_io_rename_file (svn_stringbuf_t *src, svn_stringbuf_t *dst,
> > + apr_pool_t *pool)
> > +{
> > + apr_status_t apr_err = apr_file_rename (src->data, dst->data, pool);
> > + if (apr_err)
> > + return svn_error_createf (apr_err, 0, NULL, pool,
> > + "Couldn't rename `%s' to `%s'.",
> > + src->data, dst->data);
> >
> > + return SVN_NO_ERROR;
> > +}
> >
> > +svn_error_t *
> > +svn_io_set_permissions (svn_stringbuf_t *path, apr_fileperms_t perms,
> > + svn_boolean_t apply_umask, apr_pool_t *pool)
> > +{
> > + apr_status_t apr_err;
> > +#if (!defined(OS2) && !defined(BEOS) && !defined(WIN32) && !defined(NETWARE))
> > + if (apply_umask)
> > + {
> > + apr_fileperms_t mask = apr_unix_mode2perms (umask (0777));
> > + umask (apr_unix_perms2mode (mask));
> > + /* In order to apply the mask, we need to have an or of flags. For
> > + * some reason, apr's APR_OS_DEFAULT is not an or of flags, but
> > + * rather a special value. Convert it to its meaning here. */
> > + if (perms == APR_OS_DEFAULT)
> > + perms = APR_UREAD | APR_UWRITE | APR_GREAD | APR_GWRITE | APR_WREAD
> > + | APR_WWRITE;
> > + perms = perms & ~mask;
> > + }
> > +#endif
> > + apr_err = apr_file_perms_set (path->data, perms);
> > + if (apr_err && !APR_STATUS_IS_EINCOMPLETE (apr_err)
> > + && !APR_STATUS_IS_ENOTIMPL (apr_err))
> > + return svn_error_createf (apr_err, 0, NULL, pool, "Could not change "
> > + "permissions on `%s'.", path->data);
> > + return SVN_NO_ERROR;
> > +}
> >
> > svn_error_t *svn_io_copy_dir_recursively (svn_stringbuf_t *src,
> > svn_stringbuf_t *dst_parent,
> > --
> > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > Kevin Pilch-Bisson http://www.pilch-bisson.net
> > "Historically speaking, the presences of wheels in Unix
> > has never precluded their reinvention." - Larry Wall
> > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
--
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Kevin Pilch-Bisson http://www.pilch-bisson.net
"Historically speaking, the presences of wheels in Unix
has never precluded their reinvention." - Larry Wall
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Re: [PATCH] Make working copies read-only.
Posted by Karl Fogel <kf...@newton.ch.collab.net>.
Kevin, thanks for taking this on! A couple of nits/questions:
- No OS-specific code in Subversion itself -- use APR for that. So
your new svn_io_set_permissions() needs to be changed. Note that
it's okay for a patch to include changes to APR; we have a fair
number of APR committers here, no worries.
Now, you probably saw the `WIN32' stuff in config.c and io.c in
the same directory and said to yourself "Hey, if it was okay
there, it'll be okay in my new function.", which is
understandable. :-) However, that old stuff is equally
blasphemous, and will be removed ASAP (Greg Stein just noticed the
io.c stuff, and plans to deal with it by not using file
descriptors with Neon at all, which may require some tweaks to
Neon, but that's fine).
So it's still: no OS-sensitive code in Subversion. APR is our
portability layer.
- I don't understand why we suddenly need to pay attention to umask
for this change. The idea is to make the .svn area read-only,
except for the tmp area, which doesn't need umask afaik. So if we
weren't using umasks before now, why does this change require
paying attention to them?
This isn't actually a nit, it's a real question -- probably
there's a good reason for the umask stuff, and I'm just not
thinking of it...
Best,
-Karl
Kevin Pilch-Bisson <ke...@pilch-bisson.net> writes:
> Subject and log message say it all.
>
> Log:
> Make the .svn area read-only, with the exception of things in tmp, which
> will be writable. This will have no effect on platforms other unix, since
> apr_file_open does not obey the permission flags, and apr_file_perms_set
> is not implemented.
>
> This patch is safe to apply to an already existing working copy. It will
> gradually make file read-only as they are updated or committed.
>
> * subversion/include/svn_io.h:(svn_io_rename_file): New prototype for a
> wrapper around apr_file_rename;
> (svn_io_set_permissions): New prototype for a function which is able to
> set the permissions in a matter that obeys a user's umask.
>
> * subversion/libsvn_subr/io.c:(svn_io_append_file): Format for 80 column
> lines.
> (svn_io_rename_file): New function, a wrapper around apr_file_rename.
> (svn_io_set_permissions): New function, on unix this is able to retreive
> the user's umask and set the file as if it were newly created. On other
> platforms this does nothing since apr_file_perms_set is not implemented
> on any other platform.
>
> * subversion/libsvn_wc/wc.h:Doc chanes for SVN_LOG_ATTR_MV and
> SVN_LOG_ATTR_CP.
> (SVN_WC__LOG_ATTR_PERM): New define, use to indicate what permissions a
> move, copy, or append action should set the dest file to.
> (SVN_WC__LOG_PERM_SRC): New define, don't change permissions.
> (SVN_WC__LOG_DEFAULT): New define, use os-default permissions.
> (SVN_WC__LOG_EXEC): New define, make file executable(not currently used,
> but might be useful for making checkouts/updates recognize executable
> files.
> (SVN_WC__LOG_READONLY): New define, make file read-only.
>
> * subversion/libsvn_wc/props.c:(svn_wc__do_property_merge): Pass
> permissions to log entries.
>
> * subversion/libsvn_wc/adm_crawler.c:(restore_file): Make files writable
> when copying them out to the working copy.
>
> * subversion/libsvn_wc/log.c:(svn_wc__perm_type): New enum.
> (file_xfer_under_path) Takes new `perm' argument, sets dest file
> permissions according to it.
> (log_do_file_xfer) Determine perm argument for file_xfer_under_path from
> the xml attributes.
>
> * subversion/libsvn_wc/adm_files.c:(maybe_copy_file): Set permissions when
> copying the file to the tmp area.
> (sync_adm_file): Set permissions on the file when moving it out of the
> tmp area.
> (close_adm_file): Set permissions on the file when moving it
> out of the tmp area.
> (svn_wc__close_auth_file): Make the file readable only by the user when
> closing it.
> (svn_wc__remove_adm_file): Make the file writable before removing it.
>
> * subversion/libsvn_wc/get_editor.c:(close_file): Pass permissions to be
> used to log entries.
>
> Index: ./subversion/include/svn_io.h
> ===================================================================
> --- ./subversion/include/.svn/text-base/svn_io.h.svn-base Wed Nov 21 12:45:58 2001
> +++ ./subversion/include/svn_io.h Tue Nov 27 13:47:16 2001
> @@ -129,6 +129,19 @@
> svn_stringbuf_t *dst,
> apr_pool_t *pool);
>
> +/* Rename SRC to DST. DST will be overwritten if it exists. */
> +svn_error_t *svn_io_rename_file (svn_stringbuf_t *src,
> + svn_stringbuf_t *dst,
> + apr_pool_t *pool);
> +
> +/* Change the permissions on PATH to PERMS. If APPLY_UMASK is true then
> + * the permissions will be combined with the umask in the same way as
> + * open, if the platform permits. Otherwise, the permissions will be set
> + * to exactly PERMS. */
> +svn_error_t *svn_io_set_permissions (svn_stringbuf_t *path,
> + apr_fileperms_t perms,
> + svn_boolean_t apply_umask,
> + apr_pool_t *pool);
>
> /* Read a line from FILE into BUF, but not exceeding *LIMIT bytes.
> * Does not include newline, instead '\0' is put there.
> Index: ./subversion/libsvn_wc/props.c
> ===================================================================
> --- ./subversion/libsvn_wc/.svn/text-base/props.c.svn-base Wed Nov 21 11:22:30 2001
> +++ ./subversion/libsvn_wc/props.c Wed Nov 28 12:25:44 2001
> @@ -620,6 +620,8 @@
> tmp_prop_base,
> SVN_WC__LOG_ATTR_DEST,
> real_prop_base,
> + SVN_WC__LOG_ATTR_PERM,
> + svn_stringbuf_create (SVN_WC__LOG_PERM_SRC, pool),
> NULL);
>
> /* Write log entry to move working tmp copy to real working area. */
> @@ -631,6 +633,8 @@
> tmp_props,
> SVN_WC__LOG_ATTR_DEST,
> real_props,
> + SVN_WC__LOG_ATTR_PERM,
> + svn_stringbuf_create (SVN_WC__LOG_PERM_SRC, pool),
> NULL);
>
> if (reject_tmp_fp)
> Index: ./subversion/libsvn_wc/wc.h
> ===================================================================
> --- ./subversion/libsvn_wc/.svn/text-base/wc.h.svn-base Mon Nov 26 09:35:17 2001
> +++ ./subversion/libsvn_wc/wc.h Wed Nov 28 12:26:52 2001
> @@ -373,10 +373,12 @@
> */
> #define SVN_WC__LOG_RUN_CMD "run"
>
> -/* Move file SVN_WC__LOG_ATTR_NAME to SVN_WC__LOG_ATTR_DEST. */
> +/* Move file SVN_WC__LOG_ATTR_NAME to SVN_WC__LOG_ATTR_DEST, with
> + * permissions SVN_WC__LOG_ATTR_PERM. */
> #define SVN_WC__LOG_MV "mv"
>
> -/* Copy file SVN_WC__LOG_ATTR_NAME to SVN_WC__LOG_ATTR_DEST. */
> +/* Copy file SVN_WC__LOG_ATTR_NAME to SVN_WC__LOG_ATTR_DEST, with
> + * permissions SVN_WC__LOG_ATTR_PERM. */
> #define SVN_WC__LOG_CP "cp"
>
> /* Remove file SVN_WC__LOG_ATTR_NAME. */
> @@ -415,6 +417,7 @@
> #define SVN_WC__LOG_ATTR_NAME "name"
> #define SVN_WC__LOG_ATTR_DEST "dest"
> #define SVN_WC__LOG_ATTR_REVISION "revision"
> +#define SVN_WC__LOG_ATTR_PERM "permission"
> #define SVN_WC__LOG_ATTR_TEXT_REJFILE "text-rejfile"
> #define SVN_WC__LOG_ATTR_PROP_REJFILE "prop-rejfile"
> /* The rest are for SVN_WC__LOG_RUN_CMD. Extend as necessary. */
> @@ -430,6 +433,15 @@
> #define SVN_WC__LOG_ATTR_ARG_7 "arg7"
> #define SVN_WC__LOG_ATTR_ARG_8 "arg8"
> #define SVN_WC__LOG_ATTR_ARG_9 "arg9"
> +
> +/* Keep permissions the same as the source file. */
> +#define SVN_WC__LOG_PERM_SRC "src"
> +/* Make the permissions the operating system's default. */
> +#define SVN_WC__LOG_PERM_DEFAULT "default"
> +/* Make the permissions os/default, but with the execute bit set. */
> +#define SVN_WC__LOG_PERM_EXEC "executable"
> +/* Make the permissions read-only. */
> +#define SVN_WC__LOG_PERM_READONLY "read-only"
>
>
> /* Starting at PATH, write out log entries indicating that a commit
> Index: ./subversion/libsvn_wc/adm_crawler.c
> ===================================================================
> --- ./subversion/libsvn_wc/.svn/text-base/adm_crawler.c.svn-base Mon Nov 26 09:35:17 2001
> +++ ./subversion/libsvn_wc/adm_crawler.c Tue Nov 27 14:09:04 2001
> @@ -1795,12 +1795,9 @@
> tmp_text_base_path = svn_wc__text_base_path (file_path, 1, pool);
>
> SVN_ERR (svn_io_copy_file (text_base_path, tmp_text_base_path, pool));
> - status = apr_file_rename (tmp_text_base_path->data, file_path->data, pool);
> - if (status)
> - return svn_error_createf (status, 0, NULL, pool,
> - "error renaming `%s' to `%s'",
> - tmp_text_base_path->data,
> - file_path->data);
> + SVN_ERR (svn_io_set_permissions (tmp_text_base_path, APR_OS_DEFAULT,
> + TRUE, pool));
> + SVN_ERR (svn_io_rename_file (tmp_text_base_path, file_path, pool));
>
> return SVN_NO_ERROR;
> }
> Index: ./subversion/libsvn_wc/log.c
> ===================================================================
> --- ./subversion/libsvn_wc/.svn/text-base/log.c.svn-base Wed Nov 21 08:50:24 2001
> +++ ./subversion/libsvn_wc/log.c Tue Nov 27 14:21:19 2001
> @@ -51,18 +51,27 @@
> svn_wc__xfer_mv,
> };
>
> +enum svn_wc__perm_type {
> + svn_wc__perm_src,
> + svn_wc__perm_default,
> + svn_wc__perm_exec,
> + svn_wc__perm_readonly
> +};
>
>
> -/* Copy (or rename, if RENAME is non-zero) NAME to DEST, assuming that
> - PATH is the common parent of both locations. */
> +/* Transfer contents of NAME to DEST, assuming that PATH is the common
> + parent of both locations, according to ACTION. If ACTION requires that
> + DEST be created, give it permissions appropriate to PERM. */
> static svn_error_t *
> file_xfer_under_path (svn_stringbuf_t *path,
> const char *name,
> const char *dest,
> enum svn_wc__xfer_action action,
> + enum svn_wc__perm_type perm,
> apr_pool_t *pool)
> {
> apr_status_t status;
> + apr_fileperms_t fperms;
> svn_stringbuf_t *full_from_path, *full_dest_path;
>
> full_from_path = svn_stringbuf_dup (path, pool);
> @@ -71,22 +80,39 @@
> svn_path_add_component_nts (full_from_path, name, svn_path_local_style);
> svn_path_add_component_nts (full_dest_path, dest, svn_path_local_style);
>
> + switch (perm)
> + {
> + case svn_wc__perm_default:
> + fperms = APR_OS_DEFAULT;
> + break;
> + case svn_wc__perm_exec:
> + fperms = APR_UREAD | APR_GREAD | APR_WREAD | APR_UWRITE
> + | APR_GWRITE | APR_WWRITE | APR_UEXECUTE | APR_GEXECUTE
> + | APR_WEXECUTE;
> + break;
> + case svn_wc__perm_src:
> + /* Doesn't matter, since the permissions won't be applied anyway. */
> + break;
> + case svn_wc__perm_readonly:
> + fperms = APR_UREAD | APR_GREAD | APR_WREAD;
> + }
> +
> switch (action)
> {
> case svn_wc__xfer_append:
> - return svn_io_append_file (full_from_path, full_dest_path, pool);
> + SVN_ERR (svn_io_append_file (full_from_path, full_dest_path, pool));
> + break;
> case svn_wc__xfer_cp:
> - return svn_io_copy_file (full_from_path, full_dest_path, pool);
> + SVN_ERR (svn_io_copy_file (full_from_path, full_dest_path, pool));
> + break;
> case svn_wc__xfer_mv:
> - status = apr_file_rename (full_from_path->data,
> - full_dest_path->data, pool);
> - if (status)
> - return svn_error_createf (status, 0, NULL, pool,
> - "file_xfer_under_path: "
> - "can't move %s to %s",
> - name, dest);
> + SVN_ERR (svn_io_rename_file (full_from_path, full_dest_path, pool));
> + break;
> }
>
> + if (perm != svn_wc__perm_src)
> + SVN_ERR (svn_io_set_permissions (full_dest_path, fperms, TRUE, pool));
> +
> return SVN_NO_ERROR;
> }
>
> @@ -238,15 +264,34 @@
> const XML_Char **atts)
> {
> svn_error_t *err;
> + enum svn_wc__perm_type perm = svn_wc__perm_src;
> const char *dest = svn_xml_get_attr_value (SVN_WC__LOG_ATTR_DEST, atts);
> + const char *permstr = svn_xml_get_attr_value (SVN_WC__LOG_ATTR_PERM, atts);
>
> if (! dest)
> return svn_error_createf (SVN_ERR_WC_BAD_ADM_LOG, 0, NULL, loggy->pool,
> "missing dest attr in %s", loggy->path->data);
>
> - /* Else. */
> + if (permstr)
> + {
> + if (strcmp (permstr, SVN_WC__LOG_PERM_SRC) == 0)
> + perm = svn_wc__perm_src;
> + else if (strcmp (permstr, SVN_WC__LOG_PERM_DEFAULT) == 0)
> + perm = svn_wc__perm_default;
> + else if (strcmp (permstr, SVN_WC__LOG_PERM_EXEC) == 0)
> + perm = svn_wc__perm_exec;
> + else if (strcmp (permstr, SVN_WC__LOG_PERM_READONLY) == 0)
> + perm = svn_wc__perm_readonly;
> + else
> + return svn_error_createf (SVN_ERR_WC_BAD_ADM_LOG, 0, NULL,
> + loggy->pool,
> + "unrecognized perm attr in %s",
> + loggy->path->data);
> + }
>
> - err = file_xfer_under_path (loggy->path, name, dest, action, loggy->pool);
> + /* Else. */
> + err = file_xfer_under_path (loggy->path, name, dest, action,
> + perm, loggy->pool);
> if (err)
> signal_error (loggy, err);
>
> Index: ./subversion/libsvn_wc/adm_ops.c
> ===================================================================
> --- ./subversion/libsvn_wc/.svn/text-base/adm_ops.c.svn-base Mon Nov 26 09:35:18 2001
> +++ ./subversion/libsvn_wc/adm_ops.c Wed Nov 28 13:05:32 2001
> @@ -911,6 +911,9 @@
> (err->apr_err, 0, NULL, pool,
> "revert_admin_things: Error restoring pristine text for '%s'",
> full_path->data);
> + SVN_ERR (svn_io_set_permissions (full_path,
> + APR_OS_DEFAULT,
> + TRUE, pool));
> SVN_ERR (svn_io_file_affected_time (&tstamp, full_path, pool));
>
> /* Modify our entry structure. */
> Index: ./subversion/libsvn_wc/adm_files.c
> ===================================================================
> --- ./subversion/libsvn_wc/.svn/text-base/adm_files.c.svn-base Mon Nov 26 09:35:18 2001
> +++ ./subversion/libsvn_wc/adm_files.c Wed Nov 28 13:10:44 2001
> @@ -289,11 +289,10 @@
> return SVN_NO_ERROR;
> }
> }
> - else /* SRC exists, so copy it to DST. */
> + else /* SRC exists, so copy it to DST, and make sure we can write to it. */
> {
> - err = svn_io_copy_file (src, dst, pool);
> - if (err)
> - return err;
> + SVN_ERR (svn_io_copy_file (src, dst, pool));
> + SVN_ERR (svn_io_set_permissions (dst, APR_OS_DEFAULT, TRUE, pool));
> }
>
> return SVN_NO_ERROR;
> @@ -313,7 +312,7 @@
> given how C va_lists work. */
>
> svn_stringbuf_t *tmp_path = svn_stringbuf_dup (path, pool);
> - apr_status_t apr_err;
> + svn_error_t *err;
> int components_added;
> va_list ap;
>
> @@ -327,18 +326,22 @@
> v_extend_with_adm_name (tmp_path, extension, 1, pool, ap);
> va_end (ap);
>
> - /* Rename. */
> - apr_err = apr_file_rename (tmp_path->data, path->data, pool);
> + /* Rename. 1) Make write-able, rename, make read-only. */
> + err = svn_io_set_permissions (path, APR_UREAD | APR_UWRITE, FALSE, pool);
> + if (!err || APR_STATUS_IS_ENOENT (err->apr_err))
> + {
> + err = svn_io_rename_file (tmp_path, path, pool);
> + if (!err)
> + err = svn_io_set_permissions (path,
> + APR_UREAD | APR_GREAD | APR_WREAD,
> + TRUE, pool);
> + }
> +
>
> /* Unconditionally restore path. */
> chop_admin_name (path, components_added);
>
> - if (apr_err)
> - return svn_error_createf (apr_err, 0, NULL, pool,
> - "error renaming %s to %s",
> - tmp_path->data, path->data);
> - else
> - return SVN_NO_ERROR;
> + return err;
> }
>
>
> @@ -635,6 +638,7 @@
> ...)
> {
> apr_status_t apr_err = 0;
> + svn_error_t *err;
> int components_added;
> va_list ap;
>
> @@ -672,17 +676,20 @@
> va_end (ap);
>
> /* Rename. */
> - apr_err = apr_file_rename (tmp_path->data, path->data, pool);
> + err = svn_io_set_permissions (path, APR_UREAD | APR_UWRITE, FALSE, pool);
> + if (!err || APR_STATUS_IS_ENOENT (err->apr_err))
> + {
> + err = svn_io_rename_file (tmp_path, path, pool);
> + if (!err)
> + err = svn_io_set_permissions (path,
> + APR_UREAD | APR_GREAD | APR_WREAD,
> + TRUE, pool);
> + }
>
> /* Unconditionally restore path. */
> chop_admin_name (path, components_added);
>
> - if (apr_err)
> - return svn_error_createf (apr_err, 0, NULL, pool,
> - "error renaming %s to %s",
> - tmp_path->data, path->data);
> - else
> - return SVN_NO_ERROR;
> + return err;
> }
>
> return SVN_NO_ERROR;
> @@ -767,8 +774,13 @@
> int sync,
> apr_pool_t *pool)
> {
> - return close_adm_file (handle, path, NULL, sync, pool,
> - SVN_WC__ADM_AUTH_DIR, file->data, NULL);
> + svn_stringbuf_t *full_path = svn_stringbuf_dup(path, pool);
> + SVN_ERR (close_adm_file (handle, path, NULL, sync, pool,
> + SVN_WC__ADM_AUTH_DIR, file->data, NULL));
> + extend_with_adm_name (full_path, NULL, FALSE, pool, SVN_WC__ADM_AUTH_DIR,
> + file->data, NULL);
> + SVN_ERR (svn_io_set_permissions (full_path, APR_UREAD, FALSE, pool));
> + return SVN_NO_ERROR;
> }
>
> svn_error_t *
> @@ -966,9 +978,18 @@
> components_added = v_extend_with_adm_name (path, NULL, 0, pool, ap);
> va_end (ap);
>
> - apr_err = apr_file_remove (path->data, pool);
> - if (apr_err)
> - err = svn_error_create (apr_err, 0, NULL, pool, path->data);
> + /* Make sure we have permission to remove the file. */
> + err = svn_io_set_permissions (path, APR_UREAD | APR_UWRITE, FALSE, pool);
> +
> + if (!err || APR_STATUS_IS_ENOENT (err->apr_err))
> + {
> + /* It's okay if we couldn't change permissions because the file
> + * didn't exist, since all we wanted to do was remove it. */
> + err = NULL;
> + apr_err = apr_file_remove (path->data, pool);
> + if (apr_err)
> + err = svn_error_create (apr_err, 0, NULL, pool, path->data);
> + }
>
> /* Restore path to its original state no matter what. */
> chop_admin_name (path, components_added);
> Index: ./subversion/libsvn_wc/get_editor.c
> ===================================================================
> --- ./subversion/libsvn_wc/.svn/text-base/get_editor.c.svn-base Mon Nov 26 09:35:19 2001
> +++ ./subversion/libsvn_wc/get_editor.c Tue Nov 27 09:42:39 2001
> @@ -1155,6 +1155,9 @@
> tmp_txtb,
> SVN_WC__LOG_ATTR_DEST,
> txtb,
> + SVN_WC__LOG_ATTR_PERM,
> + svn_stringbuf_create
> + (SVN_WC__LOG_PERM_READONLY, fb->pool),
> NULL);
>
> if ( (! is_locally_modified)
> @@ -1171,6 +1174,9 @@
> txtb,
> SVN_WC__LOG_ATTR_DEST,
> fb->name,
> + SVN_WC__LOG_ATTR_PERM,
> + svn_stringbuf_create
> + (SVN_WC__LOG_PERM_DEFAULT, fb->pool),
> NULL);
> }
>
> @@ -1198,6 +1204,10 @@
> txtb,
> SVN_WC__LOG_ATTR_DEST,
> fb->name,
> + SVN_WC__LOG_ATTR_PERM,
> + svn_stringbuf_create
> + (SVN_WC__LOG_PERM_DEFAULT,
> + fb->pool),
> NULL);
> }
> else /* working file exists */
> @@ -1411,6 +1421,9 @@
> fb->name,
> SVN_WC__LOG_ATTR_DEST,
> renamed_basename,
> + SVN_WC__LOG_ATTR_PERM,
> + svn_stringbuf_create
> + (SVN_WC__LOG_PERM_SRC, fb->pool),
> NULL);
>
> /* Copy the new file out into working area. */
> @@ -1422,6 +1435,9 @@
> txtb,
> SVN_WC__LOG_ATTR_DEST,
> fb->name,
> + SVN_WC__LOG_ATTR_PERM,
> + svn_stringbuf_create
> + (SVN_WC__LOG_PERM_DEFAULT, fb->pool),
> NULL);
> }
> }
> Index: ./subversion/libsvn_subr/io.c
> ===================================================================
> --- ./subversion/libsvn_subr/.svn/text-base/io.c.svn-base Wed Nov 21 12:45:58 2001
> +++ ./subversion/libsvn_subr/io.c Wed Nov 28 12:31:56 2001
> @@ -342,7 +342,8 @@
>
>
> svn_error_t *
> -svn_io_append_file (svn_stringbuf_t *src, svn_stringbuf_t *dst, apr_pool_t *pool)
> +svn_io_append_file (svn_stringbuf_t *src, svn_stringbuf_t *dst,
> + apr_pool_t *pool)
> {
> apr_status_t apr_err;
>
> @@ -358,8 +359,45 @@
> return SVN_NO_ERROR;
> }
>
> +svn_error_t *
> +svn_io_rename_file (svn_stringbuf_t *src, svn_stringbuf_t *dst,
> + apr_pool_t *pool)
> +{
> + apr_status_t apr_err = apr_file_rename (src->data, dst->data, pool);
> + if (apr_err)
> + return svn_error_createf (apr_err, 0, NULL, pool,
> + "Couldn't rename `%s' to `%s'.",
> + src->data, dst->data);
>
> + return SVN_NO_ERROR;
> +}
>
> +svn_error_t *
> +svn_io_set_permissions (svn_stringbuf_t *path, apr_fileperms_t perms,
> + svn_boolean_t apply_umask, apr_pool_t *pool)
> +{
> + apr_status_t apr_err;
> +#if (!defined(OS2) && !defined(BEOS) && !defined(WIN32) && !defined(NETWARE))
> + if (apply_umask)
> + {
> + apr_fileperms_t mask = apr_unix_mode2perms (umask (0777));
> + umask (apr_unix_perms2mode (mask));
> + /* In order to apply the mask, we need to have an or of flags. For
> + * some reason, apr's APR_OS_DEFAULT is not an or of flags, but
> + * rather a special value. Convert it to its meaning here. */
> + if (perms == APR_OS_DEFAULT)
> + perms = APR_UREAD | APR_UWRITE | APR_GREAD | APR_GWRITE | APR_WREAD
> + | APR_WWRITE;
> + perms = perms & ~mask;
> + }
> +#endif
> + apr_err = apr_file_perms_set (path->data, perms);
> + if (apr_err && !APR_STATUS_IS_EINCOMPLETE (apr_err)
> + && !APR_STATUS_IS_ENOTIMPL (apr_err))
> + return svn_error_createf (apr_err, 0, NULL, pool, "Could not change "
> + "permissions on `%s'.", path->data);
> + return SVN_NO_ERROR;
> +}
>
> svn_error_t *svn_io_copy_dir_recursively (svn_stringbuf_t *src,
> svn_stringbuf_t *dst_parent,
> --
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> Kevin Pilch-Bisson http://www.pilch-bisson.net
> "Historically speaking, the presences of wheels in Unix
> has never precluded their reinvention." - Larry Wall
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org