You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Greg Stein <gs...@lyra.org> on 2001/03/10 15:56:34 UTC
Re: CVS update: subversion/subversion/libsvn_fs dag.c dag.h err.c err.h id.c skel.c skel.h tree.c txn-table.c txn-table.h
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/
Re: CVS update: subversion/subversion/libsvn_fs dag.c dag.h err.c err.h id.c skel.c skel.h tree.c txn-table.c txn-table.h
Posted by Karl Fogel <kf...@galois.collab.net>.
I think I agree with pretty much everything below, thanks Greg. Will
continue working on it today.
-K
Greg Stein <gs...@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/