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.
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.
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.
"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' apps/branch. py 2009-01-23 20:39:54 +0000 apps/branch. py 2009-01-30 18:51:27 +0000 object) : unlock( ) ions.HTTPSeeOth er('/') ):
> --- loggerhead/
> +++ loggerhead/
> @@ -116,10 +118,15 @@ class BranchWSGIApp(
> 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.
> +
> + if type(to_ret) == type(httpexcept
> + 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' controllers/ download_ ui.py 2008-12-05 18:52:44 +0000 controllers/ download_ ui.py 2009-01-30 18:51:27 +0000 controllers import TemplatedBranchView getLogger( "loggerhead. controllers" ) anchView) : get_revno( revid) _branch_ nick, revno) static/ downloads/ ' + filename exists( rpath): export( revid, rpath) downloads/ '+filename} hView):
> --- loggerhead/
> +++ loggerhead/
> @@ -25,10 +25,26 @@ from paste import httpexceptions
> from paste.request import path_info_pop
>
> from loggerhead.
> +import os
>
> log = logging.
>
>
> +class TGZ(TemplatedBr
> + def get_values(self, path, kwargs, headers):
> + history = self._history
> + revid = self.get_revid()
> +
> + revno = history.
> + filename = '%s_%s.tgz' % (history.
> + rpath = os.getcwd()+ '/loggerhead/
> +
> + if not os.path.
> + history.
> +
> + return {'download': '/static/
> +
> +
> class DownloadUI (TemplatedBranc
>
> 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' history. py 2008-12-28 05:10:10 +0000 history. py 2009-01-30 18:51:27 +0000 debug(' annotate: %r secs' % (time.time() - z)) repository. revision_ tree(revid) export. export( rev_tree, dest)
> --- loggerhead/
> +++ loggerhead/
> @@ -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.
> +
> + def export(self, revid, dest):
> + rev_tree = self._branch.
> + bzrlib.
> + return dest
>
Ditto on the docstring here.
-- theironlion. net
Paul Hummer
http://
1024/862FF08F C921 E962 58F8 5547 6723 0E8C 1C4D 8AC5 862F F08F
"
I make changes that you suggest and push it: bazaar. launchpad. net/~danigm/ loggerhead/ loggerhead/ revision/ 267
http://
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.