[PATCH] hgweb: Added directory browsing support when the descend setting is set to a false value

Mads Kiilerich mads at kiilerich.com
Mon Sep 6 07:51:56 CDT 2010


Thanks for the patch!

On 09/06/2010 01:45 PM, Paul Boddie wrote:
> # HG changeset patch
> # User Paul Boddie<paul at boddie.org.uk>
> # Date 1283717736 -7200
> # Node ID 3db11696610d4c6109751655fe4e98766bc3cdf7
> # Parent  74f54b7775f2fac7283f6f674822da0145e18dd1
> Added directory browsing support when the descend setting is set to a false value.

See the "reasonable one-line summary" part of ContributingChanges. (I 
just updated it a bit.)

Perhaps something like "hgweb: allow directory browsing when not descending"

The description should probably also mention the history of descend 
option and that this patch changes the behavior of some configurations, 
but that it is considered a bugfix.

> diff -r 74f54b7775f2 -r 3db11696610d mercurial/hgweb/hgwebdir_mod.py
> --- a/mercurial/hgweb/hgwebdir_mod.py	Mon Sep 06 07:14:18 2010 +0200
> +++ b/mercurial/hgweb/hgwebdir_mod.py	Sun Sep 05 22:15:36 2010 +0200
> @@ -200,28 +200,30 @@
>           def rawentries(subdir="", **map):
>
>               descend = self.ui.configbool('web', 'descend', True)
> +            seen = set()
>               for name, path in self.repos:
>
>                   if not name.startswith(subdir):
>                       continue
>                   name = name[len(subdir):]
> -                if not descend and '/' in name:
> -                    continue
>
> -                u = self.ui.copy()
> -                try:
> -                    u.readconfig(os.path.join(path, '.hg', 'hgrc'))
> -                except Exception, e:
> -                    u.warn(_('error reading %s/.hg/hgrc: %s\n') % (path, e))
> -                    continue
> -                def get(section, name, default=None):
> -                    return u.config(section, name, default, untrusted=True)
> +                # identify directories containing repositories
> +                directory = not descend and '/' in name
> +                if directory:
> +                    nameparts = name.split('/')

name.split('/', 1) ?

> +                    name = nameparts[0]
>
> -                if u.configbool("web", "hidden", untrusted=True):
> -                    continue
> +                    # skip already seen directories

Such detailed comments isn't necessary. The code is so clear that it 
should be obvious what is going on. And if the code isn't clear then we 
would prefer to make it clear, rather than having a comment that 
explained the bad code.

"why" comments is however better than "what" comments.

Try to follow the style of the rest of the code - even if it doesn't 
match your preferred style.

> +                    if name in seen:
> +                        continue
> +                    else:

That else isn't needed and doesn't improve readability.

> +                        seen.add(name)
>
> -                if not self.read_allowed(u, req):
> -                    continue
> +                    # redefine the path to refer to the directory
> +                    discarded = '/'.join(nameparts[1:])
> +
> +                    # remove name parts plus accompanying slash
> +                    path = path[:-len(discarded) - 1]

That computation looks a bit hairy. BUT I don't think it has any value 
to display the modification time of the directory, so we don't need path 
at all. It would be fine to leave the field empty - if that is possible.

It could make more sense to show the latest of the modification dates of 
the hidden repos, but I think that could be left out for now.

>                   parts = [name]
>                   if 'PATH_INFO' in req.env:
> @@ -230,30 +232,64 @@
>                       parts.insert(0, req.env['SCRIPT_NAME'])
>                   url = re.sub(r'/+', '/', '/'.join(parts) + '/')
>
> -                # update time with local timezone
> -                try:
> -                    r = hg.repository(self.ui, path)
> -                except error.RepoError:
> -                    u.warn(_('error accessing repository at %s\n') % path)
> -                    continue
> -                try:
> -                    d = (get_mtime(r.spath), util.makedate()[1])
> -                except OSError:
> -                    continue
> +                # show either a directory entry or a repository
> +                if directory:
> +                    # get the directory's time information
> +                    try:
> +                        d = (get_mtime(path), util.makedate()[1])
> +                    except OSError:
> +                        continue
>
> -                contact = get_contact(get)
> -                description = get("web", "description", "")
> -                name = get("web", "name", name)
> -                row = dict(contact=contact or "unknown",
> -                           contact_sort=contact.upper() or "unknown",
> -                           name=name,
> -                           name_sort=name,
> -                           url=url,
> -                           description=description or "unknown",
> -                           description_sort=description.upper() or "unknown",
> -                           lastchange=d,
> -                           lastchange_sort=d[1]-d[0],
> -                           archives=archivelist(u, "tip", url))
> +                    row = dict(contact="",
> +                               contact_sort="",
> +                               name=name,
> +                               name_sort=name,
> +                               url=url,
> +                               description="",

"directory" ?

> +                               description_sort="",
> +                               lastchange=d,
> +                               lastchange_sort=d[1]-d[0],
> +                               archives=[])
> +                else:

The patch is hard to review because some code is re-indented and diff 
thus messes things up.

Directories and repos only have the "yield" in common, and I think 
directories is the exception. Perhaps it would be simpler, both code- 
and review-wise to yield the directory row directly and then continue?

> +                    u = self.ui.copy()
> +                    try:
> +                        u.readconfig(os.path.join(path, '.hg', 'hgrc'))
> +                    except Exception, e:
> +                        u.warn(_('error reading %s/.hg/hgrc: %s\n') % (path, e))
> +                        continue
> +                    def get(section, name, default=None):
> +                        return u.config(section, name, default, untrusted=True)
> +
> +                    if u.configbool("web", "hidden", untrusted=True):
> +                        continue
> +
> +                    if not self.read_allowed(u, req):
> +                        continue
> +
> +                    # update time with local timezone
> +                    try:
> +                        r = hg.repository(self.ui, path)
> +                    except error.RepoError:
> +                        u.warn(_('error accessing repository at %s\n') % path)
> +                        continue
> +                    try:
> +                        d = (get_mtime(r.spath), util.makedate()[1])
> +                    except OSError:
> +                        continue
> +
> +                    contact = get_contact(get)
> +                    description = get("web", "description", "")
> +                    name = get("web", "name", name)
> +                    row = dict(contact=contact or "unknown",
> +                               contact_sort=contact.upper() or "unknown",
> +                               name=name,
> +                               name_sort=name,
> +                               url=url,
> +                               description=description or "unknown",
> +                               description_sort=description.upper() or "unknown",
> +                               lastchange=d,
> +                               lastchange_sort=d[1]-d[0],
> +                               archives=archivelist(u, "tip", url))
>                   yield row
>
>           sortdefault = None, False

A couple of other things missing from the patch:

The documentation (doc/hgrc.5.txt - and perhaps other places too) will 
also need an update.

I guess this will change the test output slightly, but we also need more 
thorough test of this new behavior.

/Mads


More information about the Mercurial-devel mailing list