On Wed, 2005-01-12 at 08:12, Peter N. Lundblad wrote:
> On Tue, 11 Jan 2005 fitz@tigris.org wrote:
>
> > Author: fitz
> > Date: Tue Jan 11 14:15:26 2005
> > New Revision: 12676
> >
> > Modified: branches/locking/subversion/libsvn_fs_fs/lock.c
> > Url: http://svn.collab.net/viewcvs/svn/branches/locking/subversion/libsvn_fs_fs/lock.c?view=diff&rev=12676&p1=branches/locking/subversion/libsvn_fs_fs/lock.c&r1=12675&p2=branches/locking/subversion/libsvn_fs_fs/lock.c&r2=12676
> > ==============================================================================
> > --- branches/locking/subversion/libsvn_fs_fs/lock.c (original)
> > +++ branches/locking/subversion/libsvn_fs_fs/lock.c Tue Jan 11 14:15:26 2005
> > +
> > +/* If ABS_PATH exists, add DIGEST_STR to it iff it's not already in
> > + it. Else, create ABS_PATH with DIGEST_STR as its only member. */
> > +static svn_error_t *
> > +add_hash_to_entries_file (const char *abs_path,
> > + const char *digest_str,
> > + apr_pool_t *pool)
> > +{
> > + apr_status_t status;
> > + apr_finfo_t finfo;
> > + char *content;
> > +
> > + status = apr_stat (&finfo, abs_path, APR_FINFO_TYPE, pool);
> > + content = apr_pstrcat (pool, digest_str, "\n", NULL);
> > +
> > + if (APR_STATUS_IS_ENOENT (status)) /* Entries file doesn't exist. */
> > + {
> > + apr_status_t status;
> > + apr_file_t *fd;
> > +
> > + status = apr_file_open (&fd, abs_path, APR_WRITE | APR_CREATE,
> > + APR_OS_DEFAULT, pool);
> > +
> > + if (status)
> > + return svn_error_wrap_apr (status,
> > + _("Can't create '%s' for lock entry."),
> > + abs_path);
> > +
> > + SVN_ERR (svn_io_file_write_full (fd, content, strlen (content),
> > + NULL, pool));
> > + apr_file_close(fd);
> > + }
> > + else /* Entries file exists. */
> > + {
> > + apr_hash_t *entries;
> > +
> > + SVN_ERR (read_entries_file(&entries, abs_path, NULL, pool));
> > +
> > + /* - Append the MD5 hash of the next component to existing file. */
> > + if (apr_hash_get (entries, digest_str, APR_HASH_KEY_STRING)
> > + == NULL)
> > + {
> > + apr_status_t status;
> > + apr_file_t *fd;
> > +
> > + status = apr_file_open (&fd, abs_path, APR_WRITE | APR_APPEND,
> > + APR_OS_DEFAULT, pool);
> > +
> > + if (status)
> > + return svn_error_wrap_apr (status,
> > + _("Can't append lock entry to '%s'"),
> > + abs_path);
> > +
> > + SVN_ERR (svn_io_file_write_full (fd, content, strlen (content),
> > + NULL, pool));
> > + apr_file_close(fd);
> > + }
> > + }
> > + return SVN_NO_ERROR;
> > +}
> > +
>
> This makes adding many locks in a directory O(n^2), since you read the
> whole file for each added lock. Do you really need to check whether the
> entry exists before appending it? You can check whether a file exists in
> the tree by an apr_stat on its own file.
Duh. You're right. I should use the same do ... while technique I use
in delete_locks and (as you note below) as soon as I discover a path
that exists, I'm done! This will be a huge savings.
> > +
> > +/* Store the lock in the OS level filesystem under
> > + repos/db/locks/locks in a file named by composing the MD5 hash of
> > + lock->path. */
> > static svn_error_t *
> > write_lock_to_file (svn_fs_t *fs,
> > svn_lock_t *lock,
> > @@ -146,21 +350,44 @@
> > apr_hash_t *hash;
> > apr_file_t *fd;
> > svn_stream_t *stream;
> > - apr_status_t status = APR_SUCCESS;
> > - char *abs_path;
> > -
> > - char *dir;
> > - /* ###file and pathnames will be limited by the native filesystem's
> > - encoding--could that pose a problem? */
> > - SVN_ERR (abs_path_to_lock_file (&abs_path, fs, lock->path, pool));
> > + apr_status_t status;
> > + apr_array_header_t *nodes;
> > + char *abs_path, *node_name, *path_so_far = "/";
> > + const char *digest_str, *path;
> > + int i;
> >
> > - /* Make sure that the directory exists before we create the lock file. */
> > - dir = svn_path_dirname (abs_path, pool);
> > - status = apr_dir_make_recursive (dir, APR_OS_DEFAULT, pool);
> > + SVN_ERR (base_path_to_lock_file (&abs_path, fs, pool));
> > + status = apr_dir_make_recursive (abs_path, APR_OS_DEFAULT, pool);
> umask problems? I think you need to be careful when creating new files and
> directories. Other parts of libsvn_fs_fs is it. I think it copies
> permissions from another file, but I am not sure.
As ghudson verified, you're right. I'll be fixing that. Somehow.
> > if (status)
> > return svn_error_wrap_apr (status, _("Can't create lock directory '%s'"),
> > - dir);
> > + abs_path);
> > +
> > + path = repository_abs_path (lock->path, pool);
> > + nodes = svn_path_decompose (path, pool);
> > +
> > + /* Make sure that each parent directory, starting with "/", has an
> > + entries file on disk, and that the entries file contains an entry
> > + for its child. */
> > + for (i = 0; i < (nodes->nelts - 1); i++)
> > + {
> > + char *child_path = "/";
> > +
> > + node_name = APR_ARRAY_IDX (nodes, i, char *);
> > +
> > + SVN_ERR (merge_paths (&path_so_far, path_so_far, node_name, pool));
> > +
> > + SVN_ERR (merge_paths (&child_path,
> > + path_so_far,
> > + APR_ARRAY_IDX (nodes, i + 1, char *),
> > + pool));
> > +
> > + digest_str = make_digest (child_path, pool);
> > +
> > + SVN_ERR (abs_path_to_lock_file (&abs_path, fs, path_so_far, pool));
> > +
> > + SVN_ERR (add_hash_to_entries_file (abs_path, digest_str, pool));
> > + }
>
> If the parent exists, then its grandparents must exist, so why go the
> whole path each time. In my refcounting variant of this, I started to make
> sure that the parent exists and continued up to the root if it didn't.
Yup. As mentioned above, I'll rework this.
> > @@ -242,26 +471,85 @@
> > return SVN_NO_ERROR;
> > }
> >
> > +
> > +/* Join all items in COMPONENTS with directory separators to create
> > + PATH. */
> > +static svn_error_t *
> > +merge_array_components (char **path,
> > + apr_array_header_t *components,
> > + apr_pool_t *pool)
> > +{
> > + int i;
> > + *path = "/";
> > +
> > + for (i = 0; i < components->nelts; i++)
> > + {
> > + char *component;
> > + component = APR_ARRAY_IDX (components, i, char *);
> > + merge_paths (path, *path, component, pool);
> > + }
> > + return SVN_NO_ERROR;
> > +}
> > +
> > static svn_error_t *
> > delete_lock (svn_fs_t *fs,
> > svn_lock_t *lock,
> > apr_pool_t *pool)
> > {
> > apr_status_t status = APR_SUCCESS;
> > - char *abs_path;
> > + apr_array_header_t *nodes;
> > + char *abs_path, *path, *child_path, *parent_path, *node;
> > + const char *digest_str;
> >
> > - /* Delete lock from locks area */
> > - SVN_ERR (abs_path_to_lock_file (&abs_path, fs, lock->path, pool));
> > - SVN_ERR (svn_io_remove_file (abs_path, pool));
> > + path = repository_abs_path (lock->path, pool);
> > + nodes = svn_path_decompose (path, pool);
> >
> > - /* Prune directories on the way back */
> > - while (status == APR_SUCCESS)
> > + do /* Iterate in reverse. */
> > {
> > - abs_path = svn_path_dirname (abs_path, pool);
> > - /* If this fails, status will != APR_SUCCESS, so we drop out of
> > - the loop. */
> > - status = apr_dir_remove (abs_path, pool);
> > + apr_hash_t *entries;
> > +
> > + SVN_ERR (merge_array_components (&child_path, nodes, pool));
> > + digest_str = make_digest (child_path, pool);
> > +
> > + /* Compose the path to the parent entries file. */
> > + (node = (char *) apr_array_pop (nodes));
> > + SVN_ERR (merge_array_components (&parent_path, nodes, pool));
> > +
> > + /* Stop when we get to the root. */
> > + if (strcmp (child_path, "/") && strcmp (parent_path, "/"))
> > + break;
> > +
> > + /* Remove child hash from entries, deleting the entries file if
> > + we've removed the last hash. */
> > + SVN_ERR (abs_path_to_lock_file (&abs_path, fs, parent_path, pool));
> > + SVN_ERR (read_entries_file (&entries, abs_path, NULL, pool));
> > + apr_hash_set (entries, digest_str,
> > + APR_HASH_KEY_STRING, NULL); /* Delete from hash.*/
> > +
> > + if (apr_hash_count (entries) == 0) /* We just removed the last entry. */
> > + {
> > + status = apr_file_remove (abs_path, pool);
> > + if (status)
> > + return svn_error_wrap_apr (status,
> > + _("Can't delete lock entries file '%s'"),
> > + abs_path);
> > + }
> > + else
> > + {
> > + write_entries_file (entries, abs_path, pool);
> > + break; /* We're done deleting entries files. */
> > + }
> > }
> > + while (node);
> > +
> Something I don't understand here. Did you add an entry for a locked path
> in each ancestor up to the root. If so, why? See the above comment.
Got it.
Thanks for the review.
-Fitz
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Jan 12 19:28:39 2005