I think I agree with pretty much everything below, thanks Greg. Will
continue working on it today.
-K
Greg Stein <gstein@lyra.org> writes:
> 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