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

Proposed by danigm on 2009-01-30
Status: Rejected
Rejected by: Martin Albisetti on 2011-03-10
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) 2009-01-30 Needs Fixing on 2009-01-30
Review via email: mp+3266@code.launchpad.net
To post a comment you must log in.
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.

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
Alexander Belchenko (bialix) wrote :

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

lp:~danigm/loggerhead/loggerhead updated on 2009-02-10
267. By danigm on 2009-02-10

Better comments in code
Added zip download

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

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 on 2009-02-10
268. By danigm on 2009-02-10

tgz string removed from template

Martin Albisetti (beuno) wrote :

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

danigm (danigm) wrote :

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

Thanks.

Sune Keller (sune-keller) wrote :

Is there any progress on this?

danigm (danigm) wrote :

I didn't work in that since last commit.

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.

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.

Martin Albisetti (beuno) wrote :

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

Unmerged revisions

268. By danigm on 2009-02-10

tgz string removed from template

267. By danigm on 2009-02-10

Better comments in code
Added zip download

266. By danigm on 2009-01-30

Added download directory, needed to download tarball

265. By danigm on 2009-01-30

Download tarball, remove use_threadpool=False

264. By danigm on 2009-01-30

Added download tarball

Subscribers

People subscribed via source and target branches