On Sat, Mar 10, 2001 at 06:51:17AM -0000, kfogel@tigris.org wrote:
>...
> +svn_fs__dag_set_entry (dag_node_t *node,
> + const char *entry,
> + svn_fs_id_t *id,
> + trail_t *trail)
> +{
> + /* kff todo: Argh, is this redundant? Could it be implemented using
> + find_dir_entry(), add_new_entry() and replace_dir_entry()? */
find_dir_entry() can be immediately used, and will avoid some problems you
have in this version.
add_new_entry() cannot; that function should be refactored to
add_new_entry_skel and add_new_entry; you can then use add_new_entry_skel.
[ and find_dir_entry should probably be renamed to find_dir_entry_skel ]
>...
> + /* Look for this entry. */
> + for (this_entry = entries->children, found_it = 0;
> + this_entry;
> + this_entry = this_entry->next)
> + {
> + skel_t *name = this_entry->children;
> +
> + if ((name->len == entry_len)
> + && (strncmp (name->data, entry, entry_len) == 0))
Should have used svn_fs__matches_atom here. It does this "right" and it
improves the readability.
("right" meaning: use memcmp because it is faster)
But this is moot because find_dir_entry(_skel) should be used.
>...
> + if (! found_it)
> + {
> + skel_t *new_entry_skel, *name_skel, *id_skel;
> +
> + /* Create the new entry. */
> + new_entry_skel = svn_fs__make_empty_list (trail->pool);
> + name_skel = svn_fs__str_atom (entry, trail->pool);
> + id_skel = svn_fs__str_atom (id_str->data, trail->pool);
> + svn_fs__prepend (id_skel, new_entry_skel);
> + svn_fs__prepend (name_skel, new_entry_skel);
> +
> + /* Stuff it onto the list. */
> + svn_fs__prepend (new_entry_skel, entries);
> + }
Refactor the above into add_new_entry_skel, then use it from add_new_entry.
>...
> --- skel.h 2001/03/05 17:07:05 1.17
> +++ skel.h 2001/03/10 06:51:17 1.18
> @@ -114,7 +114,7 @@
>
> /* Create an atom skel whose contents are the C string STR, allocated
> from POOL. */
> -skel_t *svn_fs__str_atom (char *str, apr_pool_t *pool);
> +skel_t *svn_fs__str_atom (const char *str, apr_pool_t *pool);
There are a lot of existing calls to this function with a (char *) cast for
arg1. Those casts should be tossed.
>...
> @@ -895,7 +984,24 @@
> if (apr_hash_get (source_entries, key, klen)
> && apr_hash_get (target_entries, key, klen))
> {
> - recurse;
> + char *new_apath, *new_spath, *new_tpath;
> + char *new_component = apr_palloc (pool, klen + 1);
> +
> + strncpy (new_component, key, klen);
> + new_component[klen] = '\0';
The above code is simplified by using apr_pstrndup().
> +
> + new_apath = apr_psprintf (pool, "%s/%s", ancestor_path,
> + new_component);
> + new_spath = apr_psprintf (pool, "%s/%s", source_path,
> + new_component);
> + new_tpath = apr_psprintf (pool, "%s/%s", target_path,
> + new_component);
Maybe leave a ### marker that these should use char* -based path functions
along with repos_style. *shrug*
[ actually, I'd like to just say url_style and repos_style are the same and
stop pretending there is/will-be a difference. ]
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Received on Sat Oct 21 14:36:25 2006