Daniel Berlin wrote:
>
It looks like Philip has reviewed the code. That's no excuse for me not to
review it too but I'm tired so I hope you don't mind me just pointing out some
trivia.
> [[[
> Add new function to get filenames in a directory without extra stat
> information, and ues it during revision reporting to avoid extra stat
"ues" -> "use"
> calls on some filesystems.
>
> * subversion/include/svn_io.h
> (svn_io_get_dir_filenames): New function.
Er... and an implementation for it? :-) Of course there is.
> * subversion/libsvn_wc/adm_crawler.c
> (report_revisions): Use svn_io_get_dir_filenames, and don't do
> error checking here.
>
> * subversion/libsvn_wc/lock.c
> (svn_wc_adm_retrieve): Do better error message reporting here.
> ]]]
> Index: subversion/include/svn_io.h
> ===================================================================
> --- subversion/include/svn_io.h (revision 17369)
> +++ subversion/include/svn_io.h (working copy)
> @@ -677,6 +677,18 @@ svn_error_t *svn_io_remove_file (const c
> /** Recursively remove directory @a path. @a path is utf8-encoded. */
> svn_error_t *svn_io_remove_dir (const char *path, apr_pool_t *pool);
>
> +/** Read all of the disk entries in directory @a path, a utf8-encoded
> + * path. Set @a *dirents to a hash mapping dirent names (<tt>char *</tt>) to
> + * dirent names, allocated in @a pool.
Mapping dirent names to dirent names? It's a pity a hash can't store NULL as
the value, but there is probably no reason to advertise duplicate data, so can
you say "The values are undefined"? I've seen a precedent somewhere in our
API; I can't remember where or what the exact wording was.
> Index: subversion/libsvn_wc/adm_crawler.c
> ===================================================================
> --- subversion/libsvn_wc/adm_crawler.c (revision 17369)
> +++ subversion/libsvn_wc/adm_crawler.c (working copy)
[...]
> @@ -262,17 +260,6 @@ report_revisions (svn_wc_adm_access_t *a
> /*** Files ***/
> if (current_entry->kind == svn_node_file)
> {
> - /* If the dirent changed kind, report it as missing and
> - move on to the next entry. Later on, the update
> - editor will return an 'obstructed update' error. :) */
> - if ((dirent_kind != svn_node_none)
> - && (dirent_kind != svn_node_file)
> - && (! report_everything))
> - {
> - SVN_ERR (reporter->delete_path (report_baton, this_path,
> - iterpool));
> - continue;
> - }
>
> /* If the item is missing from disk, and we're supposed to
> restore missing things, and it isn't missing as a result
I know this is the lowest of trivia, but please could you try to preserve the
amount of white space except when you intentionally change it. Here, this
block deletion leaves a blank line after an opening brace; ...
> Index: subversion/include/svn_io.h
> ===================================================================
> --- subversion/include/svn_io.h (revision 17369)
> +++ subversion/include/svn_io.h (working copy)
> @@ -677,6 +677,18 @@ svn_error_t *svn_io_remove_file (const c
> /** Recursively remove directory @a path. @a path is utf8-encoded. */
> svn_error_t *svn_io_remove_dir (const char *path, apr_pool_t *pool);
>
> +/** Read all of the disk entries in directory @a path, a utf8-encoded
> + * path. Set @a *dirents to a hash mapping dirent names (<tt>char *</tt>) to
> + * dirent names, allocated in @a pool.
> + *
> + * @note The `.' and `..' directories normally returned by
> + * apr_dir_read() are NOT returned in the hash.
> + *
> + * @since New in 1.4.
> + */
> +svn_error_t *svn_io_get_dir_filenames (apr_hash_t **dirents,
> + const char *path,
> + apr_pool_t *pool);
>
> /** Read all of the disk entries in directory @a path, a utf8-encoded
> * path. Set @a *dirents to a hash mapping dirent names (<tt>char *</tt>) to
... this changes two blank lines between declarations to one line between, but
that's OK (I'll regard it as an intentional change) because the other gaps in
the file are inconsistent and are mostly one line in this area; ...
> Index: subversion/libsvn_wc/lock.c
> ===================================================================
> --- subversion/libsvn_wc/lock.c (revision 17369)
> +++ subversion/libsvn_wc/lock.c (working copy)
> @@ -675,10 +675,89 @@ svn_wc_adm_retrieve (svn_wc_adm_access_t
> generally makes the calling code simpler as it doesn't need to check
> for NULL batons. */
> if (! *adm_access)
> - return svn_error_createf (SVN_ERR_WC_NOT_LOCKED, NULL,
> - _("Working copy '%s' is missing or not locked"),
> - svn_path_local_style (path, pool));
> + {
> + const char *wcpath;
> + const svn_wc_entry_t *subdir_entry;
> + svn_node_kind_t wckind;
> + svn_node_kind_t kind;
> + svn_error_t *err;
> +
> + err = svn_wc_entry (&subdir_entry, path, associated, TRUE, pool);
[...]
> + /* If all else fails, return our useless generic error. */
> + return svn_error_createf (SVN_ERR_WC_NOT_LOCKED, NULL,
> + _("Working copy '%s' is not locked"),
> + svn_path_local_style (path, pool));
>
> +
> + }
> return SVN_NO_ERROR;
... and this adds two blank lines before that closing brace and removes the one
that was before the "return". Let's keep it fanatically tidy!
Thanks in advance for not shouting at me. :-)
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Nov 17 03:02:06 2005