Code review comment for lp:~danigm/loggerhead/loggerhead

Revision history for this message
danigm (danigm) wrote :

"Hi there-

  Thanks for this branch! One suggestion I would have for the review process is
to explain a little bit about how to demo this. As it is, I'm pretty good at
detective work, so it's not a huge problem.

  I think this is a great addition to loggerhead. However, I'm concerned that
generating a tarball on the fly like this could bring a server to its knees if
the branch has a lot of data (like, say, MySQL). Maybe there needs to be some
configuration variable the enables of disables this functionality.

 vote needs-fixing

Cheers,
Paul

> === modified file 'loggerhead/apps/branch.py'
> --- loggerhead/apps/branch.py 2009-01-23 20:39:54 +0000
> +++ loggerhead/apps/branch.py 2009-01-30 18:51:27 +0000
> @@ -116,10 +118,15 @@ class BranchWSGIApp(object):
> try:
> try:
> c = cls(self, self.get_history)
> - return c(environ, start_response)
> + to_ret = c(environ, start_response)
> except:
> environ['exc_info'] = sys.exc_info()
> environ['branch'] = self
> raise
> finally:
> self.branch.unlock()
> +
> + if type(to_ret) == type(httpexceptions.HTTPSeeOther('/')):
> + raise to_ret
> + else:
> + return to_ret
>

I think a comment here would be helpful. After looking at the greater patch,
I understand what's going on here. However, it's not completely obvious by
just looking at this bit why it's even there.

Maybe something like:

        # If this is a download, cause the download dialog to be shown
        # as opposed to returning a response.

> === modified file 'loggerhead/controllers/download_ui.py'
> --- loggerhead/controllers/download_ui.py 2008-12-05 18:52:44 +0000
> +++ loggerhead/controllers/download_ui.py 2009-01-30 18:51:27 +0000
> @@ -25,10 +25,26 @@ from paste import httpexceptions
> from paste.request import path_info_pop
>
> from loggerhead.controllers import TemplatedBranchView
> +import os
>
> log = logging.getLogger("loggerhead.controllers")
>
>
> +class TGZ(TemplatedBranchView):
> + def get_values(self, path, kwargs, headers):
> + history = self._history
> + revid = self.get_revid()
> +
> + revno = history.get_revno(revid)
> + filename = '%s_%s.tgz' % (history._branch_nick, revno)
> + rpath = os.getcwd()+ '/loggerhead/static/downloads/' + filename
> +
> + if not os.path.exists(rpath):
> + history.export(revid, rpath)
> +
> + return {'download': '/static/downloads/'+filename}
> +
> +
> class DownloadUI (TemplatedBranchView):
>
> def __call__(self, environ, start_response):
>

I think the class could use a docstring as well as the get_values function.

> === modified file 'loggerhead/history.py'
> --- loggerhead/history.py 2008-12-28 05:10:10 +0000
> +++ loggerhead/history.py 2009-01-30 18:51:27 +0000
> @@ -51,6 +51,7 @@ import bzrlib.revision
> import bzrlib.textfile
> import bzrlib.tsort
> import bzrlib.ui
> +import bzrlib.export
>
> # bzrlib's UIFactory is not thread-safe
> uihack = threading.local()
> @@ -919,3 +920,8 @@ delta.renamed:
> lineno += 1
>
> self.log.debug('annotate: %r secs' % (time.time() - z))
> +
> + def export(self, revid, dest):
> + rev_tree = self._branch.repository.revision_tree(revid)
> + bzrlib.export.export(rev_tree, dest)
> + return dest
>

Ditto on the docstring here.

--
Paul Hummer
http://theironlion.net
1024/862FF08F C921 E962 58F8 5547 6723 0E8C 1C4D 8AC5 862F F08F
"

I make changes that you suggest and push it:
http://bazaar.launchpad.net/~danigm/loggerhead/loggerhead/revision/267

Then I make it avaliable to download zip archive going "branch/zip/" instead of "branch/tgz/". But for now it not possible to disable download and all download links points to tgz, I don't put in templates links to zip.

How can I make it optional? Where can I put a variable that could be seen from controllers and templates? I need that because it's needed to disable "branch/tgz/" url and don't show the download link in templates.

« Back to merge proposal