[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 5521 [...]

From: Brian Denny <brian_at_briandenny.net>
Date: 2003-04-03 01:25:00 CEST

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
Received on Thu Apr 3 01:33:22 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.