> -----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