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/11/11 22:54:17 UTC
Re: svn commit: rev 430 - trunk/subversion/include trunk/subversion/libsvn_wc trunk/subversion/libsvn_ra_local trunk/subversion/libsvn_delta trunk/subversion/libsvn_ra_dav
On Fri, Nov 09, 2001 at 12:41:03PM -0600, sussman@tigris.org wrote:
>...
> +++ NEW/trunk/subversion/include/svn_ra.h Fri Nov 9 12:41:03 2001
> @@ -56,9 +56,10 @@
> /* A function type for "cleaning up" after a commit. The client layer
> supplies this routine to an RA layer. RA calls this routine on
> each PATH that was committed, allowing the client to bump revision
> - numbers. */
> + numbers, possibly recursively. */
> typedef svn_error_t *(*svn_ra_close_commit_func_t) (void *close_baton,
> svn_stringbuf_t *path,
> + svn_boolean_t recurse,
> svn_revnum_t new_rev);
Why did you introduce enum svn_recurse_kind, and then use a boolean here?
Same for the bump function.
>...
> --- OLD/trunk/subversion/libsvn_wc/entries.c Fri Nov 9 12:41:03 2001
> +++ NEW/trunk/subversion/libsvn_wc/entries.c Fri Nov 9 12:41:03 2001
>...
> +svn_wc_get_version_controlled_paths (apr_hash_t *paths,
> + svn_stringbuf_t *path,
> + apr_pool_t *pool)
> +{
> + apr_hash_t *entries;
> + apr_hash_index_t *hi;
> + svn_wc_entry_t *this_dir;
> +
> + /* Read PATH's entries. */
> + SVN_ERR (svn_wc_entries_read (&entries, path, pool));
> +
> + /* Add PATH to the hash. */
> + apr_hash_set (paths, path->data, APR_HASH_KEY_STRING, (void *) 1);
You have the length as path->len, so you should use it. APR_HASH_KEY_STRING
just makes the hash code work harder. The whole point of counted strings is
to keep the length handy so you don't have to use strlen() all the time.
The APR_HASH_KEY_STRING is handy if you're using a string constant, or you
just have a char*. But if you already have the length, then use it.
>...
> + /* If a file, add its path to the hash. */
> + if (current_entry->kind == svn_node_file)
> + apr_hash_set (paths, child_path->data,
> + APR_HASH_KEY_STRING, (void *) 1);
Same thing here.
>...
> --- OLD/trunk/subversion/libsvn_wc/adm_ops.c Fri Nov 9 12:41:03 2001
> +++ NEW/trunk/subversion/libsvn_wc/adm_ops.c Fri Nov 9 12:41:03 2001
>...
> + /* Loop over this list, calling self: */
> + for (hi = apr_hash_first (pool, path_list); hi; hi = apr_hash_next (hi))
> + {
> + const void *key;
> + apr_size_t keylen;
> + void *val;
> + const char *child_path;
> + svn_stringbuf_t *child_path_str;
> +
> + apr_hash_this (hi, &key, &keylen, &val);
> + child_path = (const char *) key;
That cast isn't needed. void * -> const char * is automatic.
> + child_path_str = svn_stringbuf_create (child_path, pool);
Use ncreate() since you have keylen. If you don't want to use keylen, then
pass NULL to apr_hash_this.
>...
> --- OLD/trunk/subversion/libsvn_ra_local/ra_plugin.c Fri Nov 9 12:41:03 2001
> +++ NEW/trunk/subversion/libsvn_ra_local/ra_plugin.c Fri Nov 9 12:41:03 2001
> @@ -55,16 +55,20 @@
> {
> char *path;
> apr_size_t ignored_len;
> - void *ignored_val;
> + void *val;
> svn_stringbuf_t path_str;
> + enum svn_recurse_kind r;
>
> - apr_hash_this (hi, (void *) &path, &ignored_len, &ignored_val);
> + apr_hash_this (hi, (void *) &path, &ignored_len, &val);
Pass NULL rather than &ignored_len.
>
> /* Oh yes, the flogging ritual, how could I forget. */
> path_str.data = path;
> path_str.len = strlen (path);
Woah. That is an invalid svn_stringbuf_t. It doesn't have a pool associated
with it. Either make a real svn_stringbuf_t, or change things to use
svn_string_t (allowing you to do the above logic).
> + r = (enum svn_recurse_kind) val;
>
> - SVN_ERR (closer->close_func (closer->close_baton, &path_str, new_rev));
> + SVN_ERR (closer->close_func (closer->close_baton, &path_str,
> + (r == svn_recursive) ? TRUE : FALSE,
If you had used enum svn_recurse_kind, then this logic wouldn't have been
necessary...
>...
> +++ NEW/trunk/subversion/libsvn_delta/track_editor.c Fri Nov 9 12:41:03 2001
>...
> @@ -124,8 +124,18 @@
> child_d->edit_baton->pool);
> svn_path_add_component (child_d->path, name, svn_path_local_style);
>
> - apr_hash_set (parent_d->edit_baton->committed_targets,
> - child_d->path->data, APR_HASH_KEY_STRING, (void *) 1);
> + /* If this was an add-with-history (copy), then indicate in the
> + hash-value that this dir needs to be RECURSIVELY bumped after the
> + commit completes. */
> + if (copyfrom_path && SVN_IS_VALID_REVNUM(copyfrom_revision))
> + apr_hash_set (parent_d->edit_baton->committed_targets,
> + child_d->path->data, APR_HASH_KEY_STRING,
> + (void *) svn_recursive);
> + else
> + apr_hash_set (parent_d->edit_baton->committed_targets,
> + child_d->path->data, APR_HASH_KEY_STRING,
> + (void *) svn_nonrecursive);
You've got the lengths for those values. Use 'em
>...
> apr_hash_set (parent_d->edit_baton->committed_targets,
> - child_fb->path->data, APR_HASH_KEY_STRING, (void *) 1);
> + child_fb->path->data, APR_HASH_KEY_STRING,
>...
> apr_hash_set (parent_d->edit_baton->committed_targets,
> - path->data, APR_HASH_KEY_STRING, (void *) 1);
> + path->data, APR_HASH_KEY_STRING, (void *) svn_nonrecursive);
>...
> apr_hash_set (db->edit_baton->committed_targets,
> - db->path->data, APR_HASH_KEY_STRING, (void *) 1);
> + db->path->data, APR_HASH_KEY_STRING,
> + (void *) svn_nonrecursive);
>...
> apr_hash_set (fb->parent_dir_baton->edit_baton->committed_targets,
> - fb->path->data, APR_HASH_KEY_STRING, (void *) 1);
> + fb->path->data, APR_HASH_KEY_STRING,
> + (void *) svn_nonrecursive);
>...
> apr_hash_set (fb->parent_dir_baton->edit_baton->committed_targets,
> - fb->path->data, APR_HASH_KEY_STRING, (void *) 1);
> + fb->path->data, APR_HASH_KEY_STRING,
> + (void *) svn_nonrecursive);
Woof... Lots of them. All of these should use the length since you have it.
>...
> @@ -285,17 +299,21 @@
> hi = apr_hash_next (hi))
> {
> char *path;
> - void *ignored_val;
> + void *val;
> apr_size_t ignored_len;
> svn_stringbuf_t path_str;
> + enum svn_recurse_kind r;
>
> - apr_hash_this (hi, (void *) &path, &ignored_len, &ignored_val);
> + apr_hash_this (hi, (void *) &path, &ignored_len, &val);
Use NULL rather than &ignored_len.
> /* Sigh. */
> path_str.data = path;
> path_str.len = strlen (path);
> + r = (enum svn_recurse_kind) val;
>
> - SVN_ERR (eb->bump_func (eb->bump_baton, &path_str, eb->new_rev));
> + SVN_ERR (eb->bump_func (eb->bump_baton, &path_str,
> + (r == svn_recursive) ? TRUE : FALSE,
Use the recurse value directly after changing the bump function type.
>...
> +++ NEW/trunk/subversion/libsvn_ra_dav/merge.c Fri Nov 9 12:41:03 2001
> @@ -29,6 +29,7 @@
>
> #include "svn_string.h"
> #include "svn_error.h"
> +#include "svn_path.h"
> #include "svn_ra.h"
>
> #include "ra_dav.h"
> @@ -120,6 +121,37 @@
> /* ### remember the file and issue a report/warning later */
> }
>
> +
> +static svn_boolean_t okay_to_bump_path (const char *path,
> + apr_hash_t *valid_targets,
> + apr_pool_t *pool)
merge.c does not use spaces before the paren. Please retain the local style.
> +{
> + svn_stringbuf_t *parent_path;
> + enum svn_recurse_kind r;
> +
> + /* Easy check: if path itself is in the hash, then it's legit. */
> + if (apr_hash_get (valid_targets, path, APR_HASH_KEY_STRING))
> + return TRUE;
> +
> + /* Otherwise, this path is bumpable IFF one of its parents in in the
> + hash and marked with a 'recursion' flag. */
> + parent_path = svn_stringbuf_create (path, pool);
> +
> + do {
> + svn_path_remove_component (parent_path, svn_path_local_style);
> + if (r = (enum svn_recurse_kind) apr_hash_get (valid_targets,
> + parent_path->data,
> + APR_HASH_KEY_STRING))
"if (x = y)" is confusing. Some compilers will warn about, and every reader
is going to be suspicioues. For these constructs, make the test explicit
with "if ((x = y) != 0)"
Also, note that you have parent_path->len, so toss the APR_HASH_KEY_STRING.
> + if (r == svn_recursive)
> + return TRUE;
But since you test for an explicit value here, just drop the if statement
above. Assign straight to 'r' and then test it once.
>...
> @@ -147,7 +179,7 @@
> mc->vsn_url_name, vsn_url_str) );
>
> /* bump the revision and commit the file */
> - return (*mc->close_commit)(mc->close_baton, path_str, mc->rev);
> + return (*mc->close_commit)(mc->close_baton, path_str, FALSE, mc->rev);
> }
>
> static svn_error_t * handle_resource(merge_ctx_t *mc)
> @@ -570,7 +602,7 @@
> svn_stringbuf_set(path_str,
> APR_ARRAY_IDX(deleted_entries, i, const char *));
>
> - SVN_ERR( (*close_commit)(close_baton, path_str, mc.rev) );
> + SVN_ERR( (*close_commit)(close_baton, path_str, FALSE, mc.rev) );
These would be svn_nonrecursive, presuming you change the recurse flag.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 430 - trunk/subversion/include trunk/subversion/libsvn_wc trunk/subversion/libsvn_ra_local trunk/subversion/libsvn_delta trunk/subversion/libsvn_ra_dav
Posted by Ben Collins-Sussman <su...@collab.net>.
Greg Stein <gs...@lyra.org> writes:
> You put svn_recurse_kind into svn_types.h. Thus, I would expect it to be
> used everywhere that recursion is an issue. If you intended it only to be
> used for the hash, then it should have been called
> "svn_only_use_this_as_a_hash_value_recursion_thingy"
>
> Why the arbitrary restriction to using it only for the hash value? I see no
> rationale for that limitation.
The rationale is that I don't want to launch off on a crusade to
upgrade every 'svn_boolean_t recurse' into an 'enum svn_recurse_kind
recurse'. I'm only limiting the usage of this new type temporarily,
to solve exactly the problem it was created for...
> And we're in the midst of considering that recursion would be expanded into
> a Depth-like thing: no recurse, children only, full recurse. This enum is
> exactly the path towards that.
I have no objection to using 'enum svn_recurse_kind' everywhere
possible. I agree, it's a good road to take. But at the moment, it's
a sidetrack I don't have time to follow. Therefore, for the extreme
short term, I'm only using the enum as a hash-value in one special
case.
I'll add a note to the appropriate 'recurse flags' issue about this,
so whoever is working on that (kpb?) can upgrade all dem flags to the
new enum type.
>
> apr_hash_set(foo, "this is a long key", strlen("this is a lng key"), blah);
>
> Oops. Typo. Or sometimes there was a symbolic constant, or a variable, or
> whatever. The point is that using APR_HASH_KEY_STRING lets the hash
> implementation optimize how it will handle the key, rather than forcing the
> strlen in the caller.
>
> But if you *have* the length, then it should be used.
Ahhhhhhh, thank you, now I understand.
> > > > + apr_hash_this (hi, &key, &keylen, &val);
> > > > + child_path = (const char *) key;
> > >
> > > That cast isn't needed. void * -> const char * is automatic.
> >
> > But isn't it more readable? It reminds us that we're getting a string.
>
> I never find casts to be more readable. They will also clutter things up
> when we go and try to find invalid casts in the future (e.g. I'd like to be
> able to find places where we've casted around cost problems).
>
> The name and type of child_path provide enough information. We don't litter
> the code with casts just to keep track of types.
I don't know what to say, except that it's a matter of style, I guess.
You're older and wiser, so I'll take to heart what you say. :-)
If this style is important to you, then again, go look at nearly
*every* hash loop; we're doing these 'readability' casts all the
time. Maybe we need a 'cleanup code style' issue?
> > Hmmm, again, we need to go find every instance of apr_hash_this() in
> > the code. There's not single instance where we don't generate
> > keylen. And I'm sure that we ignore it most of the time.
>
> There are quite a few places where NULL is passed to the keylen.
Um, I did a tags-search, and out of 30-something calls to
apr_hash_this, only 7 of them pass NULL as keylen. And most of those
are in mod_dav_svn or libsvn_ra_dav -- proof that you know how to
properly use the call, and the rest of us don't. :-)
I can add this to the code-style-cleanup task.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 430 - trunk/subversion/include trunk/subversion/libsvn_wc trunk/subversion/libsvn_ra_local trunk/subversion/libsvn_delta trunk/subversion/libsvn_ra_dav
Posted by Greg Stein <gs...@lyra.org>.
On Mon, Nov 12, 2001 at 11:18:35AM -0600, Ben Collins-Sussman wrote:
> Greg Stein <gs...@lyra.org> writes:
> > > typedef svn_error_t *(*svn_ra_close_commit_func_t) (void *close_baton,
> > > svn_stringbuf_t *path,
> > > + svn_boolean_t recurse,
> > > svn_revnum_t new_rev);
> >
> > Why did you introduce enum svn_recurse_kind, and then use a boolean here?
> >
> > Same for the bump function.
>
> Because the svn_recurse_kind is used as the *value* of the hash of
> paths. The value tells us whether a committed path needs to be bumped
> recursively or not.
You put svn_recurse_kind into svn_types.h. Thus, I would expect it to be
used everywhere that recursion is an issue. If you intended it only to be
used for the hash, then it should have been called
"svn_only_use_this_as_a_hash_value_recursion_thingy"
Why the arbitrary restriction to using it only for the hash value? I see no
rationale for that limitation.
> Now, in an ideal world, I would have stored an svn_boolean_t as the
> value instead, but we can't store 0 as a hash value, due to apr's hash
> implementation. That's the only reason svn_recurse_kind exists: to
> store 1/2 instead of 0/1. Just because this kind exists, I don't feel
> we should use it everywhere. An svn_boolean_t is what I really
> "mean", so I'd rather that whereever allowed, and use the enum only
> when talking about hash values. I'd rather not add more abstractions
> to the API declaration. (It matches all other 'recurse' args of other
> functions, anyway.)
And we're in the midst of considering that recursion would be expanded into
a Depth-like thing: no recurse, children only, full recurse. This enum is
exactly the path towards that.
Using a boolean in some places and an enum in others is going to cause
problems in understanding the API. "Why are these different?" "Whoops! I
can't pass that value to that one!" Just think what happens when people pass
an enum value to a boolean arg, or vice versa. They are type-copmatible
after all...
Separate types is opening us up for bugs :-(
> > > + /* Add PATH to the hash. */
> > > + apr_hash_set (paths, path->data, APR_HASH_KEY_STRING, (void *) 1);
> >
> > You have the length as path->len, so you should use it. APR_HASH_KEY_STRING
> > just makes the hash code work harder. The whole point of counted strings is
> > to keep the length handy so you don't have to use strlen() all the time.
> >
> > The APR_HASH_KEY_STRING is handy if you're using a string constant, or you
> > just have a char*. But if you already have the length, then use it.
>
> Then we have a big code change to make. I guess there are at least a
> dozen or more places in the code where we use this macro with an
> svn_stringbuf_t. In fact, I think *every* apr_hash_set uses the
> macro; about a year ago you told us to use the macro, and since then
> it's been used exclusively and without abandon. But I see now we
> should be a bit more thoughtful. :-)
I said to use that rather than calling strlen() in the caller. A lot of code
was doing stuff like:
apr_hash_set(foo, "this is a long key", strlen("this is a lng key"), blah);
Oops. Typo. Or sometimes there was a symbolic constant, or a variable, or
whatever. The point is that using APR_HASH_KEY_STRING lets the hash
implementation optimize how it will handle the key, rather than forcing the
strlen in the caller.
But if you *have* the length, then it should be used.
> > > + apr_hash_this (hi, &key, &keylen, &val);
> > > + child_path = (const char *) key;
> >
> > That cast isn't needed. void * -> const char * is automatic.
>
> But isn't it more readable? It reminds us that we're getting a string.
I never find casts to be more readable. They will also clutter things up
when we go and try to find invalid casts in the future (e.g. I'd like to be
able to find places where we've casted around cost problems).
The name and type of child_path provide enough information. We don't litter
the code with casts just to keep track of types.
> > > + child_path_str = svn_stringbuf_create (child_path, pool);
> >
> > Use ncreate() since you have keylen. If you don't want to use keylen, then
> > pass NULL to apr_hash_this.
>
> Hmmm, again, we need to go find every instance of apr_hash_this() in
> the code. There's not single instance where we don't generate
> keylen. And I'm sure that we ignore it most of the time.
There are quite a few places where NULL is passed to the keylen.
>...
> > > /* Oh yes, the flogging ritual, how could I forget. */
> > > path_str.data = path;
> > > path_str.len = strlen (path);
> >
> > Woah. That is an invalid svn_stringbuf_t. It doesn't have a pool associated
> > with it. Either make a real svn_stringbuf_t, or change things to use
> > svn_string_t (allowing you to do the above logic).
>
> Now I'm scared; this code is copied and pasted from other parts. It
> probably vastly predates the "stringbuf internal pool" change. Hmmm.
Anywhere it exists is a problem. :-(
Switching to svn_string_t will help.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 430 - trunk/subversion/include trunk/subversion/libsvn_wc trunk/subversion/libsvn_ra_local trunk/subversion/libsvn_delta trunk/subversion/libsvn_ra_dav
Posted by Ben Collins-Sussman <su...@collab.net>.
Greg Stein <gs...@lyra.org> writes:
> > typedef svn_error_t *(*svn_ra_close_commit_func_t) (void *close_baton,
> > svn_stringbuf_t *path,
> > + svn_boolean_t recurse,
> > svn_revnum_t new_rev);
>
> Why did you introduce enum svn_recurse_kind, and then use a boolean here?
>
> Same for the bump function.
Because the svn_recurse_kind is used as the *value* of the hash of
paths. The value tells us whether a committed path needs to be bumped
recursively or not.
Now, in an ideal world, I would have stored an svn_boolean_t as the
value instead, but we can't store 0 as a hash value, due to apr's hash
implementation. That's the only reason svn_recurse_kind exists: to
store 1/2 instead of 0/1. Just because this kind exists, I don't feel
we should use it everywhere. An svn_boolean_t is what I really
"mean", so I'd rather that whereever allowed, and use the enum only
when talking about hash values. I'd rather not add more abstractions
to the API declaration. (It matches all other 'recurse' args of other
functions, anyway.)
> > + /* Add PATH to the hash. */
> > + apr_hash_set (paths, path->data, APR_HASH_KEY_STRING, (void *) 1);
>
> You have the length as path->len, so you should use it. APR_HASH_KEY_STRING
> just makes the hash code work harder. The whole point of counted strings is
> to keep the length handy so you don't have to use strlen() all the time.
>
> The APR_HASH_KEY_STRING is handy if you're using a string constant, or you
> just have a char*. But if you already have the length, then use it.
Then we have a big code change to make. I guess there are at least a
dozen or more places in the code where we use this macro with an
svn_stringbuf_t. In fact, I think *every* apr_hash_set uses the
macro; about a year ago you told us to use the macro, and since then
it's been used exclusively and without abandon. But I see now we
should be a bit more thoughtful. :-)
> > + apr_hash_this (hi, &key, &keylen, &val);
> > + child_path = (const char *) key;
>
> That cast isn't needed. void * -> const char * is automatic.
But isn't it more readable? It reminds us that we're getting a string.
>
> > + child_path_str = svn_stringbuf_create (child_path, pool);
>
> Use ncreate() since you have keylen. If you don't want to use keylen, then
> pass NULL to apr_hash_this.
Hmmm, again, we need to go find every instance of apr_hash_this() in
the code. There's not single instance where we don't generate
keylen. And I'm sure that we ignore it most of the time.
> >
> > - apr_hash_this (hi, (void *) &path, &ignored_len, &ignored_val);
> > + apr_hash_this (hi, (void *) &path, &ignored_len, &val);
>
> Pass NULL rather than &ignored_len.
Check.
> > /* Oh yes, the flogging ritual, how could I forget. */
> > path_str.data = path;
> > path_str.len = strlen (path);
>
> Woah. That is an invalid svn_stringbuf_t. It doesn't have a pool associated
> with it. Either make a real svn_stringbuf_t, or change things to use
> svn_string_t (allowing you to do the above logic).
Now I'm scared; this code is copied and pasted from other parts. It
probably vastly predates the "stringbuf internal pool" change. Hmmm.
>
> merge.c does not use spaces before the paren. Please retain the local style.
>
Check.
> > + do {
> > + svn_path_remove_component (parent_path, svn_path_local_style);
> > + if (r = (enum svn_recurse_kind) apr_hash_get (valid_targets,
> > + parent_path->data,
> > + APR_HASH_KEY_STRING))
>
> "if (x = y)" is confusing. Some compilers will warn about, and every reader
> is going to be suspicioues. For these constructs, make the test explicit
> with "if ((x = y) != 0)"
Good style point. Nobody's ever said that to me, but now I know why
you do things this way.
>
> > + if (r == svn_recursive)
> > + return TRUE;
>
> But since you test for an explicit value here, just drop the if statement
> above. Assign straight to 'r' and then test it once.
Check.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org