Re: mod_dav_svn api

On 2005-03-31 11:55:33 -0600, kfogel@collab.net wrote:
> Seems an odd name, why not 'dav_svn_repos_path' or something like that?

renamed it to dav_get_repos_path().
  
> The doc string here indicates that it returns "path to the repository"
> too.
>
> This doc string describes the NULL return in terms of an
> implementation detail. A reader may not know or care why
> dav_svn_split_uri() would be called anyway. Can you reword it to
> describe something meaningful about the circumstances under which NULL
> could be returned?

fixed. see the new patch.
 
> Why did you have to add this #include line? I'm surprised it wasn't
> required before. Also, you don't mention it in your log message.

so far mod_dav_svn.c didnt need dav_svn_uri_split(). i need it now in
dav_svn_get_repos_path(). furthermore my own function should be
public available and has its prototype in mod_dav_svn.h.
 
> Indentation problem in the declaration.

fixed.
 
> You have one variable "repo_name", and another "repos_name", and a
> third "repos_path". Can you see why a reader might get confused? :-)

repo_name was a relict from the old testing code. removed. ;)
 
> Some of these are used but ignored, for example, 'had_slash'. You
> should named the variable 'ignored', or 'ignored_had_slash', or put a
> comment next to it indicating that it is unused, or *something* to
> give the reader some guidance.

i renamed all unused variables.
 
> I'm a confused by this code.
>
> You *call* dav_svn_split_uri(), but if conf->fs_parent_path is NULL,
> you don't use any of the results, all you use is conf->fs_path, which
> was obtained from the very first function call. I don't understand
> how the normal SVNPath case is handled. Why is it sufficient to set
> fs_path to conf->fs_path and just return it, in this case? Don't we
> need to do something more? (And if we don't, then why do we do so
> much other work before the return?)

the config code in mod_dav_svn makes sure svnpath and svnparentpath are
not set at the same time. now i set the default value for the return
code to conf->fs_path. then i check if fs_parent_path is set. if
fs_parent_path is set i need to attach the repos path to get the full
qualified repos path.

i moved the dav_svn_split_uri() into the if condition.

i commented this in the source code too.
 
> You set 'repo_name' to the result of dav_svn_get_repo_name(), but you
> never use the value anywhere.

removed.
 
> Also, you have a comment line exceeding 80 characters; please keep
> lines under 78 chars or so.

fixed.

thanks for all your comments and critics and especially for the fast
reply.

i have attached v2 of the patch.

darix

-- 
irssi - the client of the smart and beautiful people
              http://www.irssi.de/

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org