Jan Nijtmans wrote on Fri, 28 Mar 2008 at 16:00 +0100:
> 2008/3/28, Hyrum K. Wright <hyrum_wright_at_mail.utexas.edu>:
>> I haven't reviewed the patch, but it would be more useful if it:
>> a) was in unified diff format
(I missed this one)
>> b) was against a trunk working copy instead of a beta tarball
>> c) had an accompanying log message to explain the change
>
> Thanks!
>
> New attempt follows:
>
> Proposed Log message:
> Allow browser navigation from repository top to repository list
> when SVNListParentPath is on.
>
This would be a good start for a log message. Log messages should also
include a list of all changed files and symbols, as explained in
http://subversion.tigris.org/hacking.html#log-messages (which is pointed
to from #patches, where Hyrum linked to).
> Reproduction recipe.
> - Go with your browser (IE or any other) to the top of any svn repository
> where SVNListParentPath is set to on, e.g.:
> <http://svn.webwideconsulting.com/plugins/friendship_plugin/>
> - Note that the super-directory (/plugins/) is browsable as well, but
> you cannot navigate to it from here.
> - I would expect that the first entry of the list is a hyperlinked "..",
> which would navigate to its super-directory.
>
> I checked this with the trunk, and the buglet is still there.
>
> Rationale: in repos.c, the decision whether to include the link or not
> is based on two things:
> - If repos_path is of length 1 (so it must be "/"), then skip this step
> - If the collection is of type DAV_SVN_RESTYPE_PARENTPATH_COLLECTION
> (the highest browsable directory), then skip this step too.
>
> This can be fixed by ignoring the first condition when SVNListParentPath is on.
> Unfortunately I don't have an environment set up to build mod_dav_svn to test
> this, but I'm pretty confident that this fix will work (for what's worth).
>
> Can I file an issue for this? See patch below.
>
> Regards,
> Jan Nijtmans
>
> Here is the patch against svn trunk in unified diff (I re-ordered it
> a little because
> dav_svn__get_list_parentpath_flag() is a more expensive test, so it should
> be done last):
>
> Index: repos.c
^
Subversion normally outputs "Index: subversion/mod_dav_svn/repos.c" here.
> ===================================================================
> --- subversion/mod_dav_svn/repos.c (revision 30100)
> +++ subversion/mod_dav_svn/repos.c (working copy)
> @@ -2810,8 +2810,9 @@
> ap_fputs(output, bb, ">\n");
> }
>
> - if ((resource->info->repos_path && resource->info->repos_path[1] != '\0')
> - && (resource->info->restype != DAV_SVN_RESTYPE_PARENTPATH_COLLECTION))
> + if ((resource->info->restype != DAV_SVN_RESTYPE_PARENTPATH_COLLECTION)
> + && resource->info->repos_path && ((resource->info->repos_path[1] != '\0')
> + || dav_svn__get_list_parentpath_flag(resource->info->r)))
The indentation here is misleading: it does not reflect the nesting of the
operators. It should be:
if ((resource->info->restype != DAV_SVN_RESTYPE_PARENTPATH_COLLECTION)
&& resource->info->repos_path
&& ((resource->info->repos_path[1] != '\0')
|| dav_svn__get_list_parentpath_flag(resource->info->r)))
Also, please wrap at 79 columns. (See
http://subversion.tigris.org/hacking.html#coding-style.)
Thanks,
Daniel
> {
> if (gen_html)
> ap_fprintf(output, bb, " <li><a href=\"../\">..</a></li>\n");
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-03-29 07:21:11 CET