> > > 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 */
Well, you don't actually need to strlen the string: if you explicitly
check each of the 4 chars (0..3) for values, the comparison will stop
at the first non-match. That way, you will never compare more
characters than there are in the string.
> 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.
Well, if you check them for the expected value, it will give you an
immediate exit from the comparison, so there's no real need to check
them all for \0: if it's a null length string (""\0), the first char
won't be in a-zA-Z, so that's where the comparison exits. Right where
you want it.
> > > 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?
No idea, probably history. I think it's an error we didn't see before
releasing 1.0...
> > 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:
[snip]
Ok. Well, I think that in that case, your change is probably best.
Should any calls to assemble_status be added, then your change will
catch their edge-cases too.
> > 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:
>
[ snip ]
No, I prefer your solution now. Thanks!
Bye,
Erik.
BTW: I forgot all about my lxr of the Subversion code:
http://hix.nu/subversion/lxr/source; I used it to have a look at the
status.c lines you indicated. It seems I do have a working copy at
hand!
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Aug 19 00:02:04 2006