A couple bugs and some varied suggestions...
On Tue, Mar 19, 2002 at 06:22:40PM -0600, kfogel@tigris.org wrote:
>...
> +++ trunk/subversion/libsvn_wc/props.c Tue Mar 19 18:22:37 2002
>...
> @@ -778,7 +775,7 @@
> *props = apr_hash_make (pool);
>
> /* Check validity of PATH */
> - SVN_ERR( svn_io_check_path (pathbuf, &kind, pool) );
> + SVN_ERR( svn_io_check_path (pathbuf->data, &kind, pool) );
That should just be 'path' rather than pathbuf->data.
>...
> @@ -910,7 +907,7 @@
> SVN_ERR (svn_wc__prop_path (&prop_path, pathbuf, 0, pool));
>
> /* Does the property file exist? */
> - SVN_ERR (svn_io_check_path (prop_path, &pkind, pool));
> + SVN_ERR (svn_io_check_path (prop_path->data, &pkind, pool));
This should be 'path' with the corresponding elimination of 'pathbuf'
>...
> +++ trunk/subversion/libsvn_wc/copy.c Tue Mar 19 18:22:37 2002
>...
> @@ -191,12 +191,12 @@
> SVN_ERR (svn_io_copy_file (src_txtb->data, dst_txtb->data, TRUE, pool));
>
> /* Copy the props over if they exist. */
> - SVN_ERR (svn_io_check_path (src_wprop, &kind, pool));
> + SVN_ERR (svn_io_check_path (src_wprop->data, &kind, pool));
> if (kind == svn_node_file)
> SVN_ERR (svn_io_copy_file (src_wprop->data, dst_wprop->data, TRUE, pool));
>
> /* Copy the base-props over if they exist */
> - SVN_ERR (svn_io_check_path (src_bprop, &kind, pool));
> + SVN_ERR (svn_io_check_path (src_bprop->data, &kind, pool));
> if (kind == svn_node_file)
> SVN_ERR (svn_io_copy_file (src_bprop->data, dst_bprop->data, TRUE, pool));
With this change, 'src_wprop' and 'src_bprop' are never used as a stringbuf.
If svn_wc__prop_path() were to return a 'const char **', then you could
eliminate the usage of a stringbuf.
>...
> +++ trunk/subversion/libsvn_wc/util.c Tue Mar 19 18:22:37 2002
> @@ -38,7 +38,7 @@
> {
> enum svn_node_kind kind;
> svn_stringbuf_t *npath = svn_stringbuf_dup (path, pool);
> - svn_error_t *err = svn_io_check_path (npath, &kind, pool);
> + svn_error_t *err = svn_io_check_path (npath->data, &kind, pool);
The 'npath' variable isn't needed. Just use the 'path' parameter.
>...
> +++ trunk/subversion/libsvn_wc/adm_crawler.c Tue Mar 19 18:22:37 2002
> @@ -193,7 +193,7 @@
> svn_wc__text_base_path (svn_stringbuf_create ((char *)key, pool),
> TRUE, pool);
>
> - SVN_ERR (svn_io_check_path (tmpfile_path, &kind, pool));
> + SVN_ERR (svn_io_check_path (tmpfile_path->data, &kind, pool));
This changes the last reference of tmpfile_path to be non-buf. If
svn_wc__text_base_path() were to return a 'const char *', then you wouldn't
need a buf at all.
>...
> +++ trunk/subversion/libsvn_wc/log.c Tue Mar 19 18:22:37 2002
> @@ -167,7 +167,7 @@
> svn_path_add_component_nts (filepath, name);
>
> tmp_text_base = svn_wc__text_base_path (filepath, 1, pool);
> - err = svn_io_check_path (tmp_text_base, &kind, pool);
> + err = svn_io_check_path (tmp_text_base->data, &kind, pool);
Potentially the svn_wc__text_base_path() change noted above would help this
call, too.
>...
> @@ -491,7 +491,7 @@
> if (strcmp (sname->data, SVN_WC_ENTRY_THIS_DIR))
> svn_path_add_component (tfile, sname);
Potential BUG:
This add_component() call should probably move outside of the if{} block it
is within. I'm assuming it should apply to the PROP_TIME check, too. If it
*doesn't* apply to the prop time, then we have an alternate bug, in that the
first if{} block can modify 'tfile', which would throw off the second if{}
block.
>...
> @@ -736,7 +736,7 @@
>
> /* Make sure our working file copy is present in the temp area. */
> tmpf = svn_wc__text_base_path (wf, 1, pool);
> - if ((err = svn_io_check_path (tmpf, &kind, pool)))
> + if ((err = svn_io_check_path (tmpf->data, &kind, pool)))
Changing scn_wc__text_base_path()'s return (as above) could fix this one,
too.
>...
> @@ -778,7 +778,7 @@
>
> SVN_ERR (svn_wc__prop_path (&tmpf, is_this_dir ? loggy->path : full_path,
> 1, pool));
> - if ((err = svn_io_check_path (tmpf, &kind, pool)))
> + if ((err = svn_io_check_path (tmpf->data, &kind, pool)))
The prior note about svn_wc__prop_path() would help here, too.
>...
> @@ -1103,7 +1103,7 @@
> SVN_ERR (svn_wc__lock (path, 0, pool));
Looking at this call to the lock code here, I wondered if there was a race
condition between the lock-test, and the actual-lock. But if svn_wc__lock
threw a problem with an already-existing lock, then it wouldn't be an issue
(and it does, so great).
Potential BUG:
However, it does seem like there is a problem here. The __locked() function
doesn't have any test to determine, "is this MY lock?" Since that isn't
there, then you could end up with two programs operating within the
directory at the same time; therefore, the lock is completely useless.
>...
> +++ trunk/subversion/libsvn_wc/adm_ops.c Tue Mar 19 18:22:37 2002
> @@ -435,7 +435,7 @@
> svn_node_kind_t kind;
>
> /* Does this file exist? If not, get outta here. */
> - SVN_ERR (svn_io_check_path (file, &kind, pool));
> + SVN_ERR (svn_io_check_path (file->data, &kind, pool));
The local function, remove_file_if_present() could have its param type
changed since the function doesn't need a stringbuf any more. Being local,
this should be straightforward to adjust all callers.
>...
> @@ -877,7 +877,7 @@
> /* If there is a pristing property file, copy it out as the
> working property file, else just remove the working property
> file. */
> - SVN_ERR (svn_io_check_path (pthing, &kind, pool));
> + SVN_ERR (svn_io_check_path (pthing->data, &kind, pool));
With this change, nobody needs 'pthing' to be a stringbuf, which implies
that svn_wc__prop_base_path() might be able to change its return type.
>...
> +++ trunk/subversion/libsvn_wc/questions.c Tue Mar 19 18:22:37 2002
>...
> @@ -639,7 +639,7 @@
> {
> path = svn_stringbuf_dup (dir_path, subpool);
> svn_path_add_component (path, entry->conflict_old);
> - SVN_ERR (svn_io_check_path (path, &kind, subpool));
> + SVN_ERR (svn_io_check_path (path->data, &kind, subpool));
All of the constructions in this function should be changed to:
const char *path = svn_path_join (dir_path->data, SOMETHING);
No need to dup a buf, and add a component.
>...
> +++ trunk/subversion/libsvn_subr/io.c Tue Mar 19 18:22:37 2002
>...
> @@ -241,19 +242,19 @@
>...
> - SVN_ERR (svn_io_check_path (dst_path, &kind, subpool));
> + SVN_ERR (svn_io_check_path (dst_path->data, &kind, subpool));
dst_path should be build using:
svn_path_join(dst_parent->data, dst_basename->data)
No need for a stringbuf for that.
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 Wed Mar 20 02:20:31 2002