[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: rev 5763 - in trunk/subversion: clients/cmdline include libsvn_client libsvn_subr tests/libsvn_subr

From: Philip Martin <philip_at_codematters.co.uk>
Date: 2003-04-30 00:01:07 CEST

cmpilato@tigris.org writes:

> Modified: trunk/subversion/libsvn_subr/target.c
> ==============================================================================
> --- trunk/subversion/libsvn_subr/target.c (original)
> +++ trunk/subversion/libsvn_subr/target.c Tue Apr 29 15:37:11 2003
> @@ -32,158 +32,202 @@
>
> /*** Code. ***/
>
> +
> +static svn_boolean_t
> +redundancy_check (const char *ancestor,
> + const char *path1,
> + const char *path2,
> + svn_depth_t depth,
> + apr_pool_t *pool)
> +{

This function has no documentation comment.

> + int path1_len;
> + svn_node_kind_t kind;
> + svn_error_t *err;
> +
> + /* For depth zero, there is no redundancy check. */
> + if (depth == svn_depth_zero)
> + return FALSE;
> +
> + /* See if PATH1 is an ancestor of PATH2 */
> + if (strcmp (ancestor, path1) != 0)
> + return FALSE;
> +
> + /* For Depth Infinity, it's enough just to know that PATH1 is an
> + ancestor of PATH2. */
> + if (depth == svn_depth_infinity)
> + return TRUE;
> +
> + /* For Depth 1, we only care if PATH2 is a file immediately inside
> + PATH1. So if the difference of PATH2 and PATH1 contains a
> + directory separator, we know PATH2 is not an immediate child of
> + PATH1. */
> + path1_len = strlen (path1);
> + if (strchr (path2 + path1_len + 1, '/'))
> + return FALSE;
> +
> + /* We now know that PATH2 is an immediate child of PATH1. All that
> + remains is to see if PATH2 is a file or not. If we can't answer
> + that, we'll go the safe route and assume the path is not
> + redundant. */
> + if (! svn_path_is_url (path2))
> + {
> + err = svn_io_check_path (path2, &kind, pool);

This is checking the physical item on the disk, and it's possible for
a physical item to obstruct a versioned item of a different kind,
e.g. an unversioned file could obstruct a versioned directory. Does
it matter that the redundancy will apply to the physical kind rather
than the versioned kind? My guess is that in practice it will not
matter, but it may be worth mentioning it in the documentation for
svn_path_condense_targets.

> + if ((err == SVN_NO_ERROR) && (kind == svn_node_file))
> + return TRUE;
> + if (err)
> + svn_error_clear (err);
> + }
> +
> + return FALSE;
> +}
> +

More importantly, this change appears to introduce a regression as
follows (note the two commit targets)

  svnadmin create repo
  svn co file://`pwd`/repo wc
  echo x >> wc/foo
  svn add wc/foo
  svn ci wc wc/foo

Using valgrind the commit gives several warnings, I think they all
follow from the first at line 210 in target.c where an invalid string
is added to the array.

Ah, I've discovered target-test, it shows a similar problem, try

valgrind subversion/tests/libsvn_subr/.libs/lt-target-test 0 wc wc/foo

-- 
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Apr 30 00:02:30 2003

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.