"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.