You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Yoshiki Hayashi <yo...@xemacs.org> on 2001/02/23 07:30:44 UTC
[PATCH] svn_fs__open_dag, take 3
Jim Blandy <ji...@zwingli.cygnus.com> writes:
[...]
Thanks for detailed review. This is an updated patch.
(svn_fs__dag_dup): New function.
(svn_fs__dag_open): Implement it.
(svn_fs__err_no_such_entry): New function.
Index: dag.c
===================================================================
RCS file: /cvs/subversion/subversion/libsvn_fs/dag.c,v
retrieving revision 1.35
diff -u -r1.35 dag.c
--- dag.c 2001/02/23 06:16:20 1.35
+++ dag.c 2001/02/23 07:28:16
@@ -11,6 +11,10 @@
* ====================================================================
*/
+#define APR_WANT_STRFUNC
+#define APR_WANT_MEMFUNC
+#include "apr_want.h"
+
#include "svn_path.h"
#include "svn_fs.h"
#include "dag.h"
@@ -107,6 +111,15 @@
*/
+dag_node_t *
+svn_fs__dag_dup (dag_node_t *node,
+ trail_t *trail)
+{
+ dag_node_t *dag_dup = apr_palloc (trail->pool, sizeof (*node));
+ memcpy (dag_dup, node, sizeof (*node));
+ return dag_dup;
+}
+
const svn_fs_id_t *svn_fs__dag_get_id (dag_node_t *node)
{
return node->id;
@@ -402,16 +415,49 @@
}
-svn_error_t *svn_fs__dag_open (dag_node_t **child_p,
- dag_node_t *parent,
- const char *name,
- trail_t *trail)
+svn_error_t *
+svn_fs__dag_open (dag_node_t **child_p,
+ dag_node_t *parent,
+ const char *name,
+ trail_t *trail)
{
- abort();
- /* NOTREACHED */
- return NULL;
-}
+ skel_t *entry = find_dir_entry (parent, name);
+ dag_node_t *child = apr_pcalloc (trail->pool, sizeof (*child));
+
+ if (! svn_fs__dag_is_directory (parent))
+ {
+ svn_string_t *unparsed_id = svn_fs_unparse_id (parent->id, trail->pool);
+ return
+ svn_error_createf
+ (SVN_ERR_FS_NOT_DIRECTORY, 0, NULL, parent->pool,
+ "Node revision `%s' is not a directory in filesystem `%s'",
+ unparsed_id->data, parent->fs->env_path);
+ }
+
+ /* ### Turn this into a separate function? */
+
+ /* Verify NAME is a valid entry. */
+ if (! strcmp (name, ".") || ! strcmp (name, "..") || ! strcmp (name, ""))
+ return svn_fs__err_path_syntax (parent->fs, name);
+ if (entry)
+ {
+ /* Entry is (NAME ID) */
+ skel_t *id = entry->children->next;
+ child->id = svn_fs_parse_id (id->data, id->len, trail->pool);
+ }
+ else
+ return svn_fs__err_no_such_entry (parent->fs, parent->id, name);
+
+ child->fs = parent->fs;
+ SVN_ERR (svn_fs__get_node_revision (&child->contents,
+ child->fs, child->id, trail));
+
+ child->pool = trail->pool;
+ *child_p = child;
+
+ return SVN_NO_ERROR;
+}
svn_error_t *svn_fs__dag_delete (dag_node_t *parent,
Index: err.c
===================================================================
RCS file: /cvs/subversion/subversion/libsvn_fs/err.c,v
retrieving revision 1.24
diff -u -r1.24 err.c
--- err.c 2001/02/15 23:02:13 1.24
+++ err.c 2001/02/23 07:28:16
@@ -238,7 +238,18 @@
path, fs->env_path);
}
+svn_error_t *
+svn_fs__err_no_such_entry (svn_fs_t *fs,
+ const svn_fs_id_t *id,
+ const char *name)
+{
+ svn_string_t *unparsed_id = svn_fs_unparse_id (id, fs->pool);
+ return svn_error_createf
+ (SVN_ERR_FS_NO_SUCH_ENTRY, 0, 0, fs->pool,
+ "No entry named `%s' in filesystem `%s', directory revision `%s'",
+ name, fs->env_path, unparsed_id->data);
+}
/*
Index: err.h
===================================================================
RCS file: /cvs/subversion/subversion/libsvn_fs/err.h,v
retrieving revision 1.20
diff -u -r1.20 err.h
--- err.h 2001/02/15 23:02:13 1.20
+++ err.h 2001/02/23 07:28:16
@@ -133,6 +133,9 @@
/* SVN_ERR_FS_NOT_DIRECTORY: PATH does not refer to a directory in FS. */
svn_error_t *svn_fs__err_not_directory (svn_fs_t *fs, const char *path);
+/* SVN_ERR_FS_NO_SUCH_ENTRY: NAME does not exists in directory ID in FS. */
+svn_error_t *svn_fs__err_no_such_entry (svn_fs_t *fs, const svn_fs_id_t *id,
+ const char *name);
#endif /* SVN_LIBSVN_FS_ERR_H */
--
Yoshiki Hayashi
Re: [PATCH] svn_fs__open_dag, take 3
Posted by Jim Blandy <ji...@zwingli.cygnus.com>.
Yoshiki Hayashi <yo...@xemacs.org> writes:
> +dag_node_t *
> +svn_fs__dag_dup (dag_node_t *node,
> + trail_t *trail)
> +{
> + dag_node_t *dag_dup = apr_palloc (trail->pool, sizeof (*node));
> + memcpy (dag_dup, node, sizeof (*node));
> + return dag_dup;
> +}
The duplicate must have its pool set correctly, and its ID must be
allocated in its own pool.
Mike's committed his own definition for this function, which had the
same problem with the ID; I'll commit a fix for it shortly, along with
the content caching stuff.
Re: [PATCH] svn_fs__open_dag, take 3
Posted by Karl Fogel <kf...@galois.collab.net>.
Jim Blandy writes, regarding the svn_fs__dag_open patch:
> Shouldn't you call find_dir_entry on parent before you verify that
> it's a directory?
+1 (Clarification: I think Jim meant to write "after", not "before".)
Re: [PATCH] svn_fs__open_dag, take 3
Posted by Jim Blandy <ji...@zwingli.cygnus.com>.
Yoshiki Hayashi <yo...@xemacs.org> writes:
> -svn_error_t *svn_fs__dag_open (dag_node_t **child_p,
> - dag_node_t *parent,
> - const char *name,
> - trail_t *trail)
> +svn_error_t *
> +svn_fs__dag_open (dag_node_t **child_p,
> + dag_node_t *parent,
> + const char *name,
> + trail_t *trail)
> {
> - abort();
> - /* NOTREACHED */
> - return NULL;
> -}
> + skel_t *entry = find_dir_entry (parent, name);
> + dag_node_t *child = apr_pcalloc (trail->pool, sizeof (*child));
> +
> + if (! svn_fs__dag_is_directory (parent))
> + {
> + svn_string_t *unparsed_id = svn_fs_unparse_id (parent->id, trail->pool);
> + return
> + svn_error_createf
> + (SVN_ERR_FS_NOT_DIRECTORY, 0, NULL, parent->pool,
> + "Node revision `%s' is not a directory in filesystem `%s'",
> + unparsed_id->data, parent->fs->env_path);
> + }
> +
> + /* ### Turn this into a separate function? */
> +
> + /* Verify NAME is a valid entry. */
> + if (! strcmp (name, ".") || ! strcmp (name, "..") || ! strcmp (name, ""))
> + return svn_fs__err_path_syntax (parent->fs, name);
>
> + if (entry)
> + {
> + /* Entry is (NAME ID) */
> + skel_t *id = entry->children->next;
> + child->id = svn_fs_parse_id (id->data, id->len, trail->pool);
> + }
> + else
> + return svn_fs__err_no_such_entry (parent->fs, parent->id, name);
> +
> + child->fs = parent->fs;
> + SVN_ERR (svn_fs__get_node_revision (&child->contents,
> + child->fs, child->id, trail));
> +
> + child->pool = trail->pool;
> + *child_p = child;
> +
> + return SVN_NO_ERROR;
> +}
Shouldn't you call find_dir_entry on parent before you verify that
it's a directory?
Shouldn't you verify that NAME is valid before you try to look it up
in the directory? Granted, directories should never contain invalid
names, but this is a bit subtle, and I'm not sure everyone will notice
that before changing the code in the future. I think it's generally
best to validate the input before doing anything with it.
> svn_error_t *svn_fs__dag_delete (dag_node_t *parent,
> Index: err.c
> ===================================================================
> RCS file: /cvs/subversion/subversion/libsvn_fs/err.c,v
> retrieving revision 1.24
> diff -u -r1.24 err.c
> --- err.c 2001/02/15 23:02:13 1.24
> +++ err.c 2001/02/23 07:28:16
> @@ -238,7 +238,18 @@
> path, fs->env_path);
> }
>
> +svn_error_t *
> +svn_fs__err_no_such_entry (svn_fs_t *fs,
> + const svn_fs_id_t *id,
> + const char *name)
> +{
> + svn_string_t *unparsed_id = svn_fs_unparse_id (id, fs->pool);
>
> + return svn_error_createf
> + (SVN_ERR_FS_NO_SUCH_ENTRY, 0, 0, fs->pool,
> + "No entry named `%s' in filesystem `%s', directory revision `%s'",
> + name, fs->env_path, unparsed_id->data);
> +}
>
>
> /*
> Index: err.h
> ===================================================================
> RCS file: /cvs/subversion/subversion/libsvn_fs/err.h,v
> retrieving revision 1.20
> diff -u -r1.20 err.h
> --- err.h 2001/02/15 23:02:13 1.20
> +++ err.h 2001/02/23 07:28:16
> @@ -133,6 +133,9 @@
> /* SVN_ERR_FS_NOT_DIRECTORY: PATH does not refer to a directory in FS. */
> svn_error_t *svn_fs__err_not_directory (svn_fs_t *fs, const char *path);
>
> +/* SVN_ERR_FS_NO_SUCH_ENTRY: NAME does not exists in directory ID in FS. */
> +svn_error_t *svn_fs__err_no_such_entry (svn_fs_t *fs, const svn_fs_id_t *id,
> + const char *name);
>
> #endif /* SVN_LIBSVN_FS_ERR_H */
You should use SVN_ERR_FS_NOT_FOUND, which already exists.