Merge lp:~beuno/loggerhead/serve-config into lp:loggerhead
- serve-config
- Merge into trunk-rich
Status: | Merged |
---|---|
Merged at revision: | not available |
Proposed branch: | lp:~beuno/loggerhead/serve-config |
Merge into: | lp:loggerhead |
Diff against target: | None lines |
To merge this branch: | bzr merge lp:~beuno/loggerhead/serve-config |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Michael Hudson-Doyle | Approve | ||
Matt Nordhoff | Needs Fixing | ||
Review via email: mp+6821@code.launchpad.net |
Commit message
Description of the change
Martin Albisetti (beuno) wrote : | # |
Matt Nordhoff (mnordhoff) wrote : | # |
Without doing a real review, I spotted a couple trivial whitespace issues:
* The copyright headers tend to have 2 spaces between "2009" and "Canonical".
* TestEmptyBranch's docstring starts with a space.
Aside from that, one question: What about using 403 Forbidden for hidden branches instead of 404 Not Found? Obviously it'd reveal their existence, but it's also more true to a setting named "serve_http = False". Sort of. Maybe. :D
Adrian Wilkins (adrian-wilkins) wrote : | # |
> What about using 403 Forbidden for hidden
> branches instead of 404 Not Found? Obviously it'd reveal their existence, but
> it's also more true to a setting named "serve_http = False". Sort of. Maybe.
> :D
I've noted that some servers go the other way, and pitch a 403 for requests to non-existent branches as well. This is either "more secure" or "of dubious helpfulness", depending on your point of view.
Matt Nordhoff (mnordhoff) wrote : | # |
> > What about using 403 Forbidden for hidden
> > branches instead of 404 Not Found? Obviously it'd reveal their existence,
> but
> > it's also more true to a setting named "serve_http = False". Sort of. Maybe.
> > :D
>
> I've noted that some servers go the other way, and pitch a 403 for requests to
> non-existent branches as well. This is either "more secure" or "of dubious
> helpfulness", depending on your point of view.
For truly non-existent branches, I vote 404.
For hidden branches, IMO 403 is correct, but I can accept 404 for the privacy aspect.
Anyway, what do you want to do here? I'm okay either way. :)
Matt Nordhoff (mnordhoff) wrote : | # |
I approve, pending the 403 vs. 404 bikeshed I started, but someone more knowledgeable than me should verify that you didn't miss any entry points.
Serving .bzr is broken -- see lp:~jelmer/loggerhead/fix-bzrserve or lp:~mnordhoff/loggerhead/380026-fix-serving-branches, but there are conflicts between them and this branch, and I'd prefer that you fix them. I could probably handle it myself, but it'd mostly be guesswork, trying random things until it stopped spewing 500 Internal Server Errors.
- 361. By Martin Albisetti
-
Fixed whitespace and other trivial bits from the review
Martin Albisetti (beuno) wrote : | # |
> * The copyright headers tend to have 2 spaces between "2009" and "Canonical".
>
> * TestEmptyBranch's docstring starts with a space.
Done.
> Aside from that, one question: What about using 403 Forbidden for hidden
> branches instead of 404 Not Found? Obviously it'd reveal their existence, but
> it's also more true to a setting named "serve_http = False". Sort of. Maybe.
> :D
I chose 404 because it gives out less about what's available. I feel that if we spit out a 403, there has to be some sort of ACL or way of viewing them, when there actually isn't.
Matt Nordhoff (mnordhoff) wrote : | # |
> I approve, pending the 403 vs. 404 bikeshed I started, but someone more
> knowledgeable than me should verify that you didn't miss any entry points.
>
> Serving .bzr is broken -- see lp:~jelmer/loggerhead/fix-bzrserve or
> lp:~mnordhoff/loggerhead/380026-fix-serving-branches, but there are conflicts
> between them and this branch, and I'd prefer that you fix them. I could
> probably handle it myself, but it'd mostly be guesswork, trying random things
> until it stopped spewing 500 Internal Server Errors.
Also: I think you should use ConfigObj's as_bool method instead of "== 'False'".
Matt Nordhoff (mnordhoff) wrote : | # |
> Serving .bzr is broken -- see lp:~jelmer/loggerhead/fix-bzrserve or
> lp:~mnordhoff/loggerhead/380026-fix-serving-branches, but there are conflicts
> between them and this branch, and I'd prefer that you fix them. I could
> probably handle it myself, but it'd mostly be guesswork, trying random things
> until it stopped spewing 500 Internal Server Errors.
Those branches have now landed, FYI.
- 362. By Martin Albisetti
-
Merge from trunk and fix conflict by refactoring the code
Martin Albisetti (beuno) wrote : | # |
Ok, merged with trunk and resolved conflicts by refactoring.
All tests pass :)
Martin Albisetti (beuno) wrote : | # |
> Also: I think you should use ConfigObj's as_bool method instead of "==
> 'False'".
Would you be so kind as to point me to an example of this?
Matt Nordhoff (mnordhoff) wrote : | # |
I just noticed that a few tabs got introduced into the code:
$ ack-grep --python '\t'
loggerhead/
111: bzrdir = BzrDir.
112: self.transport.
113: branch = bzrdir.
114: if branch.
115: raise httpexceptions.
117: return
loggerhead/
75: if b.get_config(
Matt Nordhoff (mnordhoff) wrote : | # |
Martin Albisetti wrote:
> Ok, merged with trunk and resolved conflicts by refactoring.
> All tests pass :)
I fixed the tabs and a couple other trivial things in
lp:~mnordhoff/loggerhead/serve-config.
However, now there's a different issue: When serving branches on a
remote transport, trying to access .bzr/ results in a
TooManyConcurre
BzrDir.
E.g.:
serve-config$ bzr serve --directory .. &
serve-config$ ./serve-branches bzr://localhost/ &
serve-config$ bzr log -r -1 http://
Matt Nordhoff (mnordhoff) wrote : | # |
Martin Albisetti wrote:
>> Also: I think you should use ConfigObj's as_bool method instead of "==
>> 'False'".
>
> Would you be so kind as to point me to an example of this?
Sorry, I dunno. When going through Bazaar's configuration system, I'm
not sure if it's possible to use it.
- 363. By Martin Albisetti
-
Merge in Peng's tab fixes
Michael Hudson-Doyle (mwhudson) wrote : | # |
I guess this is bb:comment... two points:
1) I'm not sure what checking in branch.py and in transport.py really gains you -- I think just checking in branch.py would be OK?
2) '== "False"' as a condition check is pretty icky, I think at least 'false' should be accepted too. It's unfortunate that bzrlib doesn't let you at the as_bool method of configobj. Perhaps we can just paste in a version of that as a standalone function? And file a bug on bzrlib.
Let's chat about these points on IRC sometime.
Martin Albisetti (beuno) wrote : | # |
> 1) I'm not sure what checking in branch.py and in transport.py really gains
> you -- I think just checking in branch.py would be OK?
Checking in transport.py blocks serving branches as well I think.
> 2) '== "False"' as a condition check is pretty icky, I think at least 'false'
> should be accepted too. It's unfortunate that bzrlib doesn't let you at the
> as_bool method of configobj. Perhaps we can just paste in a version of that
> as a standalone function? And file a bug on bzrlib.
>
> Let's chat about these points on IRC sometime.
Sure, ping me.
- 364. By Martin Albisetti
-
Merge from trunk
- 365. By Martin Albisetti
-
Add deprecation warning to start-loggerhead
Michael Hudson-Doyle (mwhudson) wrote : | # |
Land it, let's deal with the fallout later :)
Preview Diff
1 | === modified file 'NEWS' |
2 | --- NEWS 2009-05-17 11:10:38 +0000 |
3 | +++ NEWS 2009-05-27 12:51:51 +0000 |
4 | @@ -115,6 +115,10 @@ |
5 | |
6 | - Fix 'bzr serve --http' errors. (Matt Nordhoff, #377551) |
7 | |
8 | + - Added the option to hide branches by setting http_serve = False |
9 | + in locations.conf (Martin Albisetti) |
10 | + |
11 | + |
12 | 1.10 [22Dec2008] |
13 | --------------- |
14 | |
15 | |
16 | === renamed file 'README.txt' => 'README' |
17 | --- README.txt 2009-03-17 22:31:42 +0000 |
18 | +++ README 2009-05-27 12:52:49 +0000 |
19 | @@ -1,7 +1,7 @@ |
20 | LOGGERHEAD |
21 | ========== |
22 | |
23 | -[ Version 1.6 for Bazaar 1.6 ] |
24 | +[ Version 1.10 for Bazaar 1.6 ] |
25 | |
26 | Loggerhead is a web viewer for Bazaar branches. It can be used to |
27 | navigate a branch history, annotate files, perform searches... all the |
28 | @@ -63,31 +63,14 @@ |
29 | USING A CONFIG FILE |
30 | ------------------- |
31 | |
32 | -Previous versions of Loggerhead read their configuration from a config |
33 | -file. This mode of operation is still supported by the |
34 | -'start-loggerhead' script. A 'loggerhead.conf.example' file is |
35 | -included in the source which has comments explaining the various |
36 | -options. |
37 | - |
38 | -Loggerhead can then be started by running:: |
39 | - |
40 | - $ ./start-loggerhead |
41 | - |
42 | -This will run loggerhead in the background, listening on port 8080 by |
43 | -default. |
44 | - |
45 | -To stop Loggerhead, run:: |
46 | - |
47 | - $ ./stop-loggerhead |
48 | - |
49 | -In the configuration file you can configure projects, and branches per |
50 | -project. The idea is that you could be publishing several (possibly |
51 | -unrelated) projects through the same loggerhead instance, and several |
52 | -branches for the same project. See the "loggerhead.conf.example" file |
53 | -included with the source. |
54 | - |
55 | -A debug and access log are stored in the logs/ folder, relative to |
56 | -the location of the start-loggerhead script. |
57 | +To hide branches from being displayed, add to ``~/.bazaar/locations.conf``, |
58 | +under the branch's section: |
59 | + |
60 | + [/path/to/branch] |
61 | + http_serve = False |
62 | + |
63 | + |
64 | +More configuration options to come soon. |
65 | |
66 | |
67 | SERVING LOGGERHEAD FROM BEHIND APACHE |
68 | @@ -107,13 +90,14 @@ |
69 | run behind a proxy at the root of a site, but if you're running it at |
70 | some path into the site, you'll need to specify is using '--prefix=/some_path'. |
71 | |
72 | -FILES CHANGED CACHE |
73 | -------------------- |
74 | - |
75 | -To speed up the display of the changelog view for large trees, |
76 | -loggerhead can be configured to cache the files changes between |
77 | -revisions. Set the 'cachepath' value in the config file. |
78 | - |
79 | + |
80 | +SEARCH |
81 | +------ |
82 | + |
83 | +Search is currently supported by using the bzr-search plugin (available |
84 | +at: ``https://launchpad.net/bzr-search`` |
85 | +You need to have the plugin installed and each branch indexed to allow |
86 | +searching on branches. |
87 | |
88 | SUPPORT |
89 | ------- |
90 | |
91 | === modified file 'loggerhead/apps/branch.py' |
92 | --- loggerhead/apps/branch.py 2009-05-13 11:34:09 +0000 |
93 | +++ loggerhead/apps/branch.py 2009-05-27 09:55:41 +0000 |
94 | @@ -1,3 +1,19 @@ |
95 | +# Copyright (C) 2008, 2009 Canonical Ltd. |
96 | +# |
97 | +# This program is free software; you can redistribute it and/or modify |
98 | +# it under the terms of the GNU General Public License as published by |
99 | +# the Free Software Foundation; either version 2 of the License, or |
100 | +# (at your option) any later version. |
101 | +# |
102 | +# This program is distributed in the hope that it will be useful, |
103 | +# but WITHOUT ANY WARRANTY; without even the implied warranty of |
104 | +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the |
105 | +# GNU General Public License for more details. |
106 | +# |
107 | +# You should have received a copy of the GNU General Public License |
108 | +# along with this program; if not, write to the Free Software |
109 | +# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA |
110 | +# |
111 | """The WSGI application for serving a Bazaar branch.""" |
112 | |
113 | import logging |
114 | @@ -113,6 +129,10 @@ |
115 | return self.branch.get_config().get_user_option('public_branch') |
116 | |
117 | def app(self, environ, start_response): |
118 | + # Check again if the branch is blocked from being served, this is |
119 | + # mostly for tests. It's already checked in apps/transport.py |
120 | + if self.branch.get_config().get_user_option('http_serve') == 'False': |
121 | + raise httpexceptions.HTTPNotFound() |
122 | self._url_base = environ['SCRIPT_NAME'] |
123 | self._static_url_base = environ.get('loggerhead.static.url') |
124 | if self._static_url_base is None: |
125 | |
126 | === modified file 'loggerhead/apps/transport.py' |
127 | --- loggerhead/apps/transport.py 2009-05-04 20:28:50 +0000 |
128 | +++ loggerhead/apps/transport.py 2009-05-27 09:55:41 +0000 |
129 | @@ -1,6 +1,23 @@ |
130 | +# Copyright (C) 2008, 2009 Canonical Ltd. |
131 | +# |
132 | +# This program is free software; you can redistribute it and/or modify |
133 | +# it under the terms of the GNU General Public License as published by |
134 | +# the Free Software Foundation; either version 2 of the License, or |
135 | +# (at your option) any later version. |
136 | +# |
137 | +# This program is distributed in the hope that it will be useful, |
138 | +# but WITHOUT ANY WARRANTY; without even the implied warranty of |
139 | +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the |
140 | +# GNU General Public License for more details. |
141 | +# |
142 | +# You should have received a copy of the GNU General Public License |
143 | +# along with this program; if not, write to the Free Software |
144 | +# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA |
145 | +# |
146 | """Serve branches at urls that mimic a transport's file system layout.""" |
147 | |
148 | from bzrlib import branch, errors, lru_cache, urlutils |
149 | +from bzrlib.bzrdir import BzrDir |
150 | |
151 | from paste.request import path_info_pop |
152 | from paste import httpexceptions |
153 | @@ -63,7 +80,10 @@ |
154 | raise httpexceptions.HTTPNotFound() |
155 | return self.app_for_non_branch(environ)(environ, start_response) |
156 | else: |
157 | - return self.app_for_branch(b)(environ, start_response) |
158 | + if b.get_config().get_user_option('http_serve') == 'False': |
159 | + raise httpexceptions.HTTPNotFound() |
160 | + else: |
161 | + return self.app_for_branch(b)(environ, start_response) |
162 | |
163 | |
164 | class BranchesFromTransportRoot(object): |
165 | @@ -82,6 +102,14 @@ |
166 | elif environ['PATH_INFO'] == '/favicon.ico': |
167 | return favicon_app(environ, start_response) |
168 | elif '/.bzr/' in environ['PATH_INFO']: |
169 | + try: |
170 | + bzrdir = BzrDir.open_containing_from_transport( |
171 | + self.transport.clone(environ['PATH_INFO'])) |
172 | + branch = bzrdir.open_branch() |
173 | + if branch.get_config().get_user_option('http_serve') == 'False': |
174 | + raise httpexceptions.HTTPNotFound() |
175 | + except errors.NotBranchError: |
176 | + pass |
177 | app = urlparser.make_static(None, self.transport) |
178 | return app(environ, start_response) |
179 | else: |
180 | |
181 | === modified file 'loggerhead/controllers/directory_ui.py' |
182 | --- loggerhead/controllers/directory_ui.py 2009-05-05 18:37:26 +0000 |
183 | +++ loggerhead/controllers/directory_ui.py 2009-05-26 16:21:53 +0000 |
184 | @@ -72,6 +72,8 @@ |
185 | for d in listing: |
186 | try: |
187 | b = branch.Branch.open_from_transport(self.transport.clone(d)) |
188 | + if b.get_config().get_user_option('http_serve') == 'False': |
189 | + continue |
190 | except: |
191 | if not stat.S_ISDIR(self.transport.stat(d).st_mode): |
192 | continue |
193 | |
194 | === modified file 'loggerhead/tests/test_simple.py' |
195 | --- loggerhead/tests/test_simple.py 2009-02-18 10:40:22 +0000 |
196 | +++ loggerhead/tests/test_simple.py 2009-05-27 12:44:28 +0000 |
197 | @@ -1,8 +1,26 @@ |
198 | +# Copyright (C) 2008, 2009 Canonical Ltd. |
199 | +# |
200 | +# This program is free software; you can redistribute it and/or modify |
201 | +# it under the terms of the GNU General Public License as published by |
202 | +# the Free Software Foundation; either version 2 of the License, or |
203 | +# (at your option) any later version. |
204 | +# |
205 | +# This program is distributed in the hope that it will be useful, |
206 | +# but WITHOUT ANY WARRANTY; without even the implied warranty of |
207 | +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the |
208 | +# GNU General Public License for more details. |
209 | +# |
210 | +# You should have received a copy of the GNU General Public License |
211 | +# along with this program; if not, write to the Free Software |
212 | +# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA |
213 | +# |
214 | + |
215 | import cgi |
216 | import logging |
217 | |
218 | from bzrlib.tests import TestCaseWithTransport |
219 | from bzrlib.util.configobj.configobj import ConfigObj |
220 | +from bzrlib import config |
221 | |
222 | from loggerhead.apps.branch import BranchWSGIApp |
223 | from paste.fixture import TestApp |
224 | @@ -103,6 +121,7 @@ |
225 | |
226 | |
227 | class TestEmptyBranch(BasicTests): |
228 | + """ Test that an empty branch doesn't break""" |
229 | |
230 | def setUp(self): |
231 | BasicTests.setUp(self) |
232 | @@ -118,3 +137,22 @@ |
233 | res = app.get('/files') |
234 | res.mustcontain('No revisions!') |
235 | |
236 | + |
237 | +class TestHiddenBranch(BasicTests): |
238 | + """ |
239 | + Test that hidden branches aren't shown |
240 | + FIXME: not tested that it doesn't show up on listings |
241 | + """ |
242 | + |
243 | + def setUp(self): |
244 | + BasicTests.setUp(self) |
245 | + self.createBranch() |
246 | + locations = config.locations_config_filename() |
247 | + config.ensure_config_dir_exists() |
248 | + open(locations, 'wb').write('[%s]\nhttp_serve = False' |
249 | + % (self.tree.branch.base,)) |
250 | + |
251 | + def test_no_access(self): |
252 | + app = self.setUpLoggerhead() |
253 | + res = app.get('/changes', status=404) |
254 | + |
This branch does the following:
- Allows hiding branches with serve-branches
- Tests it
- Adds some mising copyright headers
- Renames README.txt to README