You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Benjamin Pflugmann <be...@pflugmann.de> on 2003/04/02 07:37:24 UTC
Re: svn commit: rev 5521 [...]
Hi!
Sorry about the late comment, but I didn't read my mails earlier.
On Tue 2003-04-01 at 21:58:08 -0600, bdenny@tigris.org wrote:
> Author: bdenny
> Date: Tue Apr 1 21:58:04 2003
> New Revision: 5521
>
[...]
> * path.c
> (get_longest_path_ancestor): New helper function, mostly identical with
> the old svn_path_get_longest_ancestor, but fixed to return NULL if there
> is no common ancestor, as per the svn_path_get_longest_ancestor docstring.
Wouldn't be fixing the docstring instead be more handy (see below)?
I.e. having an empty string meaning "no common ancestor". This would
allow to use the result directly in a function like apr_pstrcat().
There would be no ambiguity, because currently (at least after this
patch), an empty path is not possible: There is a check for it and
NULL returned instead.
> (svn_path_get_longest_ancestor): Accept URL targets; don't count
> 'protocol://' as a common ancestor.
> (svn_path_get_absolute): Accept URL targets; treat them as absolute paths.
[...]
> -char *
> -svn_path_get_longest_ancestor (const char *path1,
> - const char *path2,
> - apr_pool_t *pool)
> +/* Return the longest common ancestor of PATH1 and PATH2.
> + *
> + * This function handles everything except the URL-handling logic
> + * of svn_path_get_longest_ancestor, and assumes that PATH1 and
> + * PATH2 are *not* URLs.
> + *
> + * If the two paths do not share a common ancestor, NULL is returned.
> + *
> + * New strings are allocated in POOL.
> + */
Is this function used elsewhere? Or is there another reason that it
creates and returns a string instead of the length of the common part?
The reason I ask is detailed below.
> +static char *
> +get_longest_path_ancestor (const char *path1,
> + const char *path2,
> + apr_pool_t *pool)
> {
> char *common_path;
> apr_size_t path1_len, path2_len;
> @@ -500,10 +510,14 @@
> /* last_dirsep is now the offset of the last directory separator we
> crossed before reaching a non-matching byte. i is the offset of
> that non-matching byte. */
> - if (((i == path1_len) && (path2[i] == '/'))
> - || ((i == path2_len) && (path1[i] == '/'))
> - || ((i == path1_len) && (i == path2_len)))
> + if (i == 0)
> + return NULL;
> + else if (((i == path1_len) && (path2[i] == '/'))
> + || ((i == path2_len) && (path1[i] == '/'))
> + || ((i == path1_len) && (i == path2_len)))
> common_path = apr_pstrmemdup (pool, path1, i);
> + else if (last_dirsep == 0)
> + return NULL;
> else
> common_path = apr_pstrmemdup (pool, path1, last_dirsep);
>
> @@ -511,6 +525,55 @@
> }
>
>
> +char *
> +svn_path_get_longest_ancestor (const char *path1,
> + const char *path2,
> + apr_pool_t *pool)
> +{
> + if (svn_path_is_url (path1))
> + {
> + if (svn_path_is_url (path2))
> + {
> + char *ancestor, *path_ancestor, *prefix;
> + int i = 0;
> +
> + /* Find ':' */
> + for (i = 0; path1[i] != ':'; i++)
> + {
> + /* They're both URLs, so EOS can't come before ':' */
> + assert (path1[i] != '\0' && path2[i] != '\0');
> +
> + /* No shared protocol => no common prefix */
> + if (path1[i] != path2[i])
> + return NULL;
> + }
> +
> + i += 3; /* Advance past '://' */
> +
> + path_ancestor = get_longest_path_ancestor (path1 + i, path2 + i,
> + pool);
> +
> + /* No shared path => no common prefix */
> + if ((! path_ancestor) || (*path_ancestor == '\0'))
> + return NULL;
> +
> + else
There is an empty line too much above the else.
If svn_path_get_longest_ancestor() returned an empty string, the
apr_strcat would work unconditionally, i.e. the if-statement wouldn't
be needed.
Btw, with the current documented behaviour of
svn_path_get_longest_ancestor(), path_ancestor may not be an empty
string, so "|| (*path_ancestor == '\0')" is redundant
> + return apr_pstrcat (pool, apr_pstrndup (pool, path1, i),
> + path_ancestor, NULL);
Overall, the scheme is stripped away from the URL, then the common
part is looked for and then it is concatenated again. I had rather
expected that the substring of path1 of the common length is taken.
Just a nitpick, but could help clarity a bit. Something like
apr_size_t common_part_lenth; // not sure, if that's the correct type
[...]
common_part_length = longest_path_ancestor_length (path1+i, path2+i, pool));
return apr_pstrndup (pool, path1, common_part_length);
That's 2 lines for the functional part instead of 8 and in this case I
think shorter is clearer.
> + }
> + else
> + {
> + /* A URL and a non-URL => no common prefix */
> + return NULL;
> + }
> + }
> + else
> + {
> + return get_longest_path_ancestor (path1, path2, pool);
> + }
With the suggested change to svn_path_get_longest_ancestor() this
wouldn't work anymore. If it returns an empty string, either a check
for that is needed or the docstring must be changed. Hm. In the former
case we are back to the if-condition for the return value and have
only effectively moved down from above. On the other hand, most
consumers did not check for NULL values until before you changed it
(Please don't hurt me... I already apologized that I did not read the
mail earlier. ;-). It gets a little bit prettier with the suggested
longest_path_ancestor_length approach.
> +}
There is some inconsistency in the error handling. The outer structure
of the function is like this:
if ( is_url(path1) )
if ( is_url(path2) )
return result_of_urls_handled;
else
return NULL;
else
return get_longest_path_ancestor()
If path1 is an URL, but path2 is not, NULL is returned.
If path2 is an URL, but path1 is not, get_longest_path_ancestor() is returned.
Although get_longest_path_ancestor() returns the correct result,
namely NULL, in most cases, when given an URL and a file name[1], the
code should be consistent or comment why it is inconsistent.
The nicest way I can currently think of is to save the result of the
svn_path_is_url calls and do something like this (I stayed with the
current approach instead of my suggestions to make it easier to
follow):
path1_is_url = is_url(path1);
path2_is_url = is_url(path2);
// don't like to use binary xor(^) for logical expression, so use long story
if ((!path1_is_url && path2_is_url) || (path1_is_url && !path2_is_url))
return NULL;
offset = 0; // has to be moved to start of function; we don't allow C99
if (path1_is_url) // implies path2_is_url
offset = ... // find end of scheme (://) and set offset accordingly
// i.e. effectively extract the path-like part
path_ancestor = get_longest_path_ancestor (path1+offset, path2+offset, ...));
if( ! path_ancestor )
return NULL;
else
return apr_pstrncat [...]
If you want to go a step further, you could even consider that the
scheme part is a valid path, call get_longest_path_ancestor beforehand
and replace the offset check by a "return NULL" in case only the
schemes are equal. But that uses too much knowledge about URLs for my
taste. (Correct knowledge, mind you; at least with current RFCs).
HTH,
Benjamin.
[1] An counter-example, if I am not mistaken: a directly named "ftp:"
and an URL "ftp://foobar". Yes, "ftp:" is a strange file name, but
not as strange as "ftp://", and until now we only agreed that we
shouldn't care about the latter.
Re: svn commit: rev 5521 [...]
Posted by Benjamin Pflugmann <be...@pflugmann.de>.
> > Wouldn't be fixing the docstring instead be more handy (see below)?
> > I.e. having an empty string meaning "no common ancestor". This would
> > allow to use the result directly in a function like apr_pstrcat().
[...]
> > With the suggested change to svn_path_get_longest_ancestor() this
That should read get_longest_path_ancestor (the function the suggested
change above is about).
> > wouldn't work anymore. If it returns an empty string, either a check
> > for that is needed or the docstring must be changed. Hm. In the former
> > case we are back to the if-condition for the return value and have
> > only effectively moved down from above. On the other hand, most
> > consumers did not check for NULL values until before you changed it
>
> i don't understand what you're saying here.
Sorry. I'll try again.
Short version: If you change both, svn_path_get_longest_ancestor and
get_longest_path_ancestor to return empty strings instead of NULL,
it's not important anymore.
Long version: As you know, I suggested to simplify
get_longest_path_ancestor by returning an empty string instead of NULL
in case of no ancestor. That gets rid of an if-condition before using
the return value for assembling new strings.
Later down, you make use of get_longest_path_ancestor's original
interface by directly passing through the its return value to the
caller. So if the interface of svn_path_get_longest_ancestor (the
function we are in) stays the same, you'd to add an if-condition that
checks if get_longest_path_ancestor gives an empty string and return a
NULL instead.
That would mean that, effictively, my suggestion did just move around
the condition instead of making in superfluous. Except, if we
propagate the change and also make the interface of
svn_path_get_longest_ancestor return an empty string instead of NULL.
Which was the behaviour (despite the doc) of most callers of
svn_path_get_longest_ancestor before your change anyhow.
In summary, I simply suggest to make svn_path_get_longest_ancestor
return empty strings, too, because else there is not much benefit in
changing get_longest_path_ancestor.
Benjamin.
Re: svn commit: rev 5521 [...]
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On Wednesday, April 2, 2003, at 06:25 PM, Brian Denny wrote:
>> There is an empty line too much above the else.
>
> you mean, you want to see
> if (condition)
> statement;
> else
> statement;
>
> instead of
> if (condition)
> statement;
>
> else
> statement;
>
> ... right? is there a consensus on this, or is it part of our coding
> standard? personally i sometimes like a blank line before an else
> statement, but i don't feel strongly about it.
i think the consensus is that if you are going to add anything extra
(including vertical whitespace), you should be using braces, so it's
clear what's contained in each branch of the if/else.
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 5521 [...]
Posted by Brian Denny <br...@briandenny.net>.
On Wed, Apr 02, 2003 at 09:37:24AM +0200, Benjamin Pflugmann wrote:
> Hi!
>
> Sorry about the late comment, but I didn't read my mails earlier.
i knew this patch would get comments, it was just a question of when.
:-)
> Wouldn't be fixing the docstring instead be more handy (see below)?
> I.e. having an empty string meaning "no common ancestor". This would
> allow to use the result directly in a function like apr_pstrcat().
so far, i can't think of a reason not to do as you suggest.
i'll think about it some more and maybe take your advice.
> There is an empty line too much above the else.
you mean, you want to see
if (condition)
statement;
else
statement;
instead of
if (condition)
statement;
else
statement;
... right? is there a consensus on this, or is it part of our coding
standard? personally i sometimes like a blank line before an else
statement, but i don't feel strongly about it.
> If svn_path_get_longest_ancestor() returned an empty string, the
> apr_strcat would work unconditionally, i.e. the if-statement wouldn't
> be needed.
true.
> Btw, with the current documented behaviour of
> svn_path_get_longest_ancestor(), path_ancestor may not be an empty
> string, so "|| (*path_ancestor == '\0')" is redundant
true.
> common_part_length = longest_path_ancestor_length (path1+i, path2+i, pool));
> return apr_pstrndup (pool, path1, common_part_length);
i think i like it.
> With the suggested change to svn_path_get_longest_ancestor() this
> wouldn't work anymore. If it returns an empty string, either a check
> for that is needed or the docstring must be changed. Hm. In the former
> case we are back to the if-condition for the return value and have
> only effectively moved down from above. On the other hand, most
> consumers did not check for NULL values until before you changed it
i don't understand what you're saying here.
> If path1 is an URL, but path2 is not, NULL is returned.
> If path2 is an URL, but path1 is not, get_longest_path_ancestor() is returned.
ooh, that i will definitely fix.
thanks for the eyes, late is better than never. :)
-brian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org