On 2005-03-31 11:55:33 -0600, firstname.lastname@example.org 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"
> 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.
> 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.
> Also, you have a comment line exceeding 80 characters; please keep
> lines under 78 chars or so.
thanks for all your comments and critics and especially for the fast
i have attached v2 of the patch.
irssi - the client of the smart and beautiful people
Received on Thu Mar 31 22:23:24 2005
To unsubscribe, e-mail: email@example.com
For additional commands, e-mail: firstname.lastname@example.org