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
Received on Sat Oct 21 14:36:48 2006