Merge lp:~danigm/loggerhead/loggerhead into lp:loggerhead

Proposed by danigm
Status: Rejected
Rejected by: Martin Albisetti
Proposed branch: lp:~danigm/loggerhead/loggerhead
Merge into: lp:loggerhead
To merge this branch: bzr merge lp:~danigm/loggerhead/loggerhead
Reviewer Review Type Date Requested Status
Paul Hummer (community) Needs Fixing
Review via email: mp+3266@code.launchpad.net
To post a comment you must log in.
Revision history for this message
danigm (danigm) wrote :

Resolve the bug: https://bugs.launchpad.net/loggerhead/+bug/240580

With this modify it's possible to download any revisiĆ³n as tar.gz.

Revision history for this message
Paul Hummer (rockstar) wrote :
Download full text (3.5 KiB)

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

Read more...

review: Needs Fixing
Revision history for this message
Alexander Belchenko (bialix) wrote :

will be nice to have ability to export as zip-archive too.

lp:~danigm/loggerhead/loggerhead updated
267. By danigm

Better comments in code
Added zip download

Revision history for this message
danigm (danigm) wrote :
Download full text (4.1 KiB)

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

Read more...

Revision history for this message
danigm (danigm) wrote :

"will be nice to have ability to export as zip-archive too."

I made it. But isn't finished yet. It's needed to change all "branch/tgz" links in "directory.pt, menu.pt, inventory.pt and revision.pt"

lp:~danigm/loggerhead/loggerhead updated
268. By danigm

tgz string removed from template

Revision history for this message
Martin Albisetti (beuno) wrote :

I'll work on making this configurable, and land this before Sunday, with or without my fix :)

Revision history for this message
danigm (danigm) wrote :

"I'll work on making this configurable, and land this before Sunday, with or without my fix :)"

Thanks.

Revision history for this message
Sune Keller (sune-keller) wrote :

Is there any progress on this?

Revision history for this message
danigm (danigm) wrote :

I didn't work in that since last commit.

Revision history for this message
Martin Albisetti (beuno) wrote :

So, I'd like to pick up this branch, finish and land it, but it seems be broken.
Could you perhaps re-push it?
It seems to be in an old format.

Revision history for this message
danigm (danigm) wrote :

On Wed, Mar 09, 2011 at 09:53:05PM -0000, Martin Albisetti wrote:
> So, I'd like to pick up this branch, finish and land it, but it seems be broken.
> Could you perhaps re-push it?
> It seems to be in an old format.
> --
> https://code.launchpad.net/~danigm/loggerhead/loggerhead/+merge/3266
> You are the owner of lp:~danigm/loggerhead/loggerhead.

Long time ago, I changed all my own repos to git, so here is the last
branch of this changes:

http://git.danigm.net/gitphp/index.php?p=loggerhead.git&a=summary

You can get an snapshot here:

http://git.danigm.net/gitphp/index.php?p=loggerhead.git&a=snapshot&h=63b583c4401522ee1fd71dbaf7805b8cafa5486c

Sorry but I haven't the bzr branch.

Revision history for this message
Martin Albisetti (beuno) wrote :

I'm rejecting this one, and will propose one based on this code.

Unmerged revisions

268. By danigm

tgz string removed from template

267. By danigm

Better comments in code
Added zip download

266. By danigm

Added download directory, needed to download tarball

265. By danigm

Download tarball, remove use_threadpool=False

264. By danigm

Added download tarball

Subscribers

People subscribed via source and target branches