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

RE: [PATCH] fix for issue #2556: support working copies on the root of a (virtual) drive

From: Lieven Govaerts <lgo_at_mobsol.be>
Date: 2006-08-17 21:24:03 CEST

> -----Original Message-----
> From: Erik Huelsmann [mailto:ehuels@gmail.com]
> Sent: donderdag 17 augustus 2006 20:39
> > > > > 1. Introduce 'X:/' as a syntax for a root folder, on Windows.
> > >
> > > This change is contained in issue-2556-wc-on-root.patch.txt.
> >
> > Regarding this patch: It's basically fine, but you skip testing for
> > the first character of "X:\\"<nul>, but that means you may
> be checking
> > past the end of the string. (this is in svn_path_is_root
> and further
> > down in the patch [the last hunk]; in is_canonical, you do it
> > correctly.)

In is_canonical we already have the length of the path, while in
svn_path_is_root we'd have to calculate it. Not that that's a problem, but
there's this comment in svn_path_is_empty in path.c:
 /* assert (is_canonical (path, strlen (path))); ### Expensive strlen */

I've taken this as a general rule to avoid calculating string lenghts too
often. Considering the time svn spends creating and deleting temporary files
during a commit I doubt anyone will notice an extra strlen call though.
I can check all chars for '\0' first too, no real need to calculate strlen.

> > Isn't it so that the X should be in the A-Za-z set of characters?
> > Should we explicitly test for that?

I'll add that.

> Ok, but I did find a problem: svn_path_is_root returns an int
> in your patch, but it should return svn_boolean_t, since
> that's what it is (a boolean).

Well, I thought the same thing, but I based svn_path_is_root on another
function in path.c:
int svn_path_is_empty(const char *path);

which returns either 1 or 0. You know why we use the 'int' there?

> I have no working copy here, so I'll ask you this question instead:
> Does this change make it so that the parent of the root is
> also the root? Or was that the defined behaviour already?
> (Your next patch is based on that behaviour, that's why I ask...)

Yep, that's the current (both pre and post my patches) behaviour of
svn_path_dirname:

/** Get the dirname of the specified canonicalized @a path, defined as
 * the path with its basename removed.
 *
 * Get the dirname of the specified @a path, defined as the path with its
 * basename removed. If @a path is root ("/"), it is returned unchanged.
 *
 * The returned dirname will be allocated in @a pool.
 */
char *svn_path_dirname(const char *path, apr_pool_t *pool);

> > > > > 2. Fix an issue where Subversion thinks a working copy on a root
path
> > > (both '/' and 'X:/') is switched.
> > >
> > > This change is contained in issue-2556-wc-switched.patch.txt.
> >
> I've looked at it now. It's a nice and localized change. But
> what would need to happen so that the parent of the root was
> something that returns 'unversioned' (parent_entry == NULL),
> so that this change isn't even required?
>
> What I mean is: currently, if a parent of an entry isn't
> versioned, we already have code which determines that the
> current entry can't be switched. My question is: can't we
> make some change somewhere so that we can benefit from the
> fact that this behaviour is correctly coded throughout the
> system? (Instead of introducing yet another edge
> case...)

Since you don't have a working copy at hand, I'll copy the relevant portion
of svn_wc_status2 here:

line 2068:
  if (entry && ! svn_path_is_empty(path))
    {
      const char *parent_path = svn_path_dirname(path, pool);
      svn_wc_adm_access_t *parent_access;
      SVN_ERR(svn_wc__adm_retrieve_internal(&parent_access, adm_access,
                                            parent_path, pool));
      if (parent_access)
        SVN_ERR(svn_wc_entry(&parent_entry, parent_path, parent_access,
                             FALSE, pool));
    }

  SVN_ERR(assemble_status(status, path, adm_access, entry, parent_entry,
                          svn_node_unknown, FALSE, /* bogus */
                          TRUE, FALSE, NULL, NULL, pool));

If we check that parent_path equals path, we can skip retrieving adm_access,
but that creates a similar edge case in my opinion. Would you prefer that
option?

Lieven.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Aug 17 21:26:07 2006

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.