Merge lp://staging/~tseaver/loggerhead/sphinxify into lp://staging/loggerhead

Some small comments on part of it (I'm watching TV now...):

> 317 +oggerhead is heavily based on `bazaar-webserve

Missing an L in "Loggerhead". ;-)

> 331 +- Paste for the server. (You need version 1.2 or newer of Paste.)
> 332 +
> 333 +- Paste Deploy (optional, needed when proxying through Apache)
> 334 +
> 335 +- flup (optional, needed to use FastCGI, SCGI or AJP)

The style is rather inconsistent here -- the spacing and "(foo)" vs. "(Foo.)".

Also, what should be done with the README now?

A couple spots use "loggerhead"; I'd prefer "Loggerhead". Not a big deal, though.

> 730 + Setting this option keeps loggerhead from adding a 'readonly+' prefix
> 731 + to the base URL of is to make visible the display of instructions for
> 732 + checking out the 'public_branch' URL for the branch being browsed.

That...what? "base URL of is to make"?

If it ever includes "readonly+" in URL in the example instructions, that's a bug. Aside from that, --allow-writes isn't really related to the instructions; it's about how the server itself works.

> 810 + Override configuration file location [XXX default is
> 811 + :file:`/etc/loggerhead.conf`]

> 815 + The directory [XXX in which] to place log files

XXX comments?

Mostly this sounds really cool. Thank you. :-)

If someone who has Sphinx installed can confirm that the syntax isn't horribly wrong or something (or maybe I'll do it myself later), I'd be happy to land it, once these (minor) issues are fixed. Even if it isn't perfect, it's certainly an improvement over Loggerhead's current documentation situation. "Don't let the perfect be the enemy of the good" and all.

bb:tweak

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Matt Nordhoff wrote:
> Some small comments on part of it (I'm watching TV now...):
>
>> 317 +oggerhead is heavily based on `bazaar-webserve
>
> Missing an L in "Loggerhead". ;-)

Fixed in r408 of my branch.

>> 331 +- Paste for the server. (You need version 1.2 or newer of Paste.)
>> 332 +
>> 333 +- Paste Deploy (optional, needed when proxying through Apache)
>> 334 +
>> 335 +- flup (optional, needed to use FastCGI, SCGI or AJP)
>
> The style is rather inconsistent here -- the spacing and "(foo)" vs. "(Foo.)".

Fixed in r409 of my branch.

> Also, what should be done with the README now?

Most projects I know using Sphinx remove the bulk of the information
from the README, leaving behind a pointer to the Sphinx docs. I wasn't
sure how you would want to handle that: I can push a changeset with a
suggested diff for the README if you like.

Tres.
- --
===================================================================
Tres Seaver +1 540-429-0999 <email address hidden>
Palladion Software "Excellence by Design" http://palladion.com
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkurO1QACgkQ+gerLs4ltQ4TFACgtiYb2dawlxWfHOIoV2LlZEsr
xXIAoJDYXKUt8jhVbHxqbTP703fHrrz2
=W3GW
-----END PGP SIGNATURE-----

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Matt Nordhoff wrote:
> Review: Needs Fixing
> A couple spots use "loggerhead"; I'd prefer "Loggerhead". Not a big deal, though.

Fix included in r408 of my branch.

>> 730 + Setting this option keeps loggerhead from adding a 'readonly+' prefix
>> 731 + to the base URL of is to make visible the display of instructions for
>> 732 + checking out the 'public_branch' URL for the branch being browsed.
>
> That...what? "base URL of is to make"?

Sorry for the typo. r410 tries to clarify.

> If it ever includes "readonly+" in URL in the example instructions,
> that's a bug. Aside from that, --allow-writes isn't really related to
> the instructions; it's about how the server itself works.

I was trying to document what effect the option actualy has, by
spelunking the code. From my exploration, it appears that the option
has a somewhat misleading name: it doesn't appear to make the
Loggerhead server willing to accept any changes from clients (in fact,
in my experience, I have to serve a "raw" bzr tree from another URL to
allow even a checkout).

At any rate, I might be completely wrong about the intended (or actual)
semantics. I do know that passing '--allow-writes' on the command line
was the only way I found to get the branch UI to show the block of HTML
which gives directions for checking out the branch.

>> 810 + Override configuration file location [XXX default is
>> 811 + :file:`/etc/loggerhead.conf`]
>
>> 815 + The directory [XXX in which] to place log files
>
> XXX comments?

Fossils from my efforts to reverse engineer from the manpage source: I
have elided them in r411.

> Mostly this sounds really cool. Thank you. :-)
>
> If someone who has Sphinx installed can confirm that the syntax isn't
> horribly wrong or something (or maybe I'll do it myself later), I'd be
> happy to land it, once these (minor) issues are fixed. Even if it isn't
> perfect, it's certainly an improvement over Loggerhead's current
> documentation situation. "Don't let the perfect be the enemy of the
> good" and all.

I'm glad it helps. Sphinx has been a big win for us on the repoze project.

Tres.
- --
===================================================================
Tres Seaver +1 540-429-0999 <email address hidden>
Palladion Software "Excellence by Design" http://palladion.com
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkurPtoACgkQ+gerLs4ltQ5uvgCdHGEM+TBqNvhQEqAkWP3fiylt
wtMAoJoqpzsBBvt/JuZWmgMJMUkoKl0P
=gvDH
-----END PGP SIGNATURE-----

Tres Seaver wrote:
> I was trying to document what effect the option actualy has, by
> spelunking the code. From my exploration, it appears that the option
> has a somewhat misleading name: it doesn't appear to make the
> Loggerhead server willing to accept any changes from clients (in fact,
> in my experience, I have to serve a "raw" bzr tree from another URL to
> allow even a checkout).
>
> At any rate, I might be completely wrong about the intended (or actual)
> semantics. I do know that passing '--allow-writes' on the command line
> was the only way I found to get the branch UI to show the block of HTML
> which gives directions for checking out the branch.

--allow-writes is supposed to simply allow pushing to the smart server,
and have no impact on the "bzr branch" example.

This wouldn't be the first time it's gotten broken, though...

The "bzr branch" example should always be displayed, unless you're
serving a remote location (e.g. "serve-branches bzr+ssh://...") and the
branch has no public_branch setting. Hmm, looking at the code, I may
have already spotted an issue, though.
--
Matt Nordhoff

Matt Nordhoff wrote:
> --allow-writes is supposed to simply allow pushing to the smart server,
> and have no impact on the "bzr branch" example.

I meant to add that that makes it equivalent to "bzr serve
--allow-writes", which makes it extremely dangerous. Without some sort
of auth setup, and if Loggerhead's user has write access to the
directory tree you're serving, anyone can use bzrlib to screw with it.
--
Matt Nordhoff

There should be NEWS, too. :D (Which can be added by whoever lands this.)

Matt Nordhoff wrote:
> Tres Seaver wrote:
> > I was trying to document what effect the option actualy has, by
> > spelunking the code. From my exploration, it appears that the option
> > has a somewhat misleading name: it doesn't appear to make the
> > Loggerhead server willing to accept any changes from clients (in fact,
> > in my experience, I have to serve a "raw" bzr tree from another URL to
> > allow even a checkout).
> >
> > At any rate, I might be completely wrong about the intended (or actual)
> > semantics. I do know that passing '--allow-writes' on the command line
> > was the only way I found to get the branch UI to show the block of HTML
> > which gives directions for checking out the branch.
>
> --allow-writes is supposed to simply allow pushing to the smart server,
> and have no impact on the "bzr branch" example.
>
> This wouldn't be the first time it's gotten broken, though...
>
> The "bzr branch" example should always be displayed, unless you're
> serving a remote location (e.g. "serve-branches bzr+ssh://...") and the
> branch has no public_branch setting. Hmm, looking at the code, I may
> have already spotted an issue, though.

Following up on this, I sent a merge proposal to fix the "To get this branch" bit, and I tested --allow-writes and it's working as expected.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Matt Nordhoff wrote:
> Matt Nordhoff wrote:
>> Tres Seaver wrote:
>>> I was trying to document what effect the option actualy has, by
>>> spelunking the code. From my exploration, it appears that the option
>>> has a somewhat misleading name: it doesn't appear to make the
>>> Loggerhead server willing to accept any changes from clients (in fact,
>>> in my experience, I have to serve a "raw" bzr tree from another URL to
>>> allow even a checkout).
>>>
>>> At any rate, I might be completely wrong about the intended (or actual)
>>> semantics. I do know that passing '--allow-writes' on the command line
>>> was the only way I found to get the branch UI to show the block of HTML
>>> which gives directions for checking out the branch.
>> --allow-writes is supposed to simply allow pushing to the smart server,
>> and have no impact on the "bzr branch" example.
>>
>> This wouldn't be the first time it's gotten broken, though...
>>
>> The "bzr branch" example should always be displayed, unless you're
>> serving a remote location (e.g. "serve-branches bzr+ssh://...") and the
>> branch has no public_branch setting. Hmm, looking at the code, I may
>> have already spotted an issue, though.
>
> Following up on this, I sent a merge proposal to fix the "To get this
> branch" bit, and I tested --allow-writes and it's working as
> expected.

I was able to test your branch:

  lp:~mnordhoff/loggerhead/readonly_local_path_from_url/

and the fix for showing branchinfo HTML is working with '--allow-writes'
turned off for me. Upgrading to that branch made the "local" checkouts
work again, too (I had been running on the 1.17.0 release of loggerhead).

Tres.
- --
===================================================================
Tres Seaver +1 540-429-0999 <email address hidden>
Palladion Software "Excellence by Design" http://palladion.com
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkurl88ACgkQ+gerLs4ltQ7v2QCgm1mXOu9wf7qjab6TNU5u+Yyn
HDEAoMy8jbGvT8b2TZZAVA+Ibg+iDixp
=f3oV
-----END PGP SIGNATURE-----

Tres,

This is awesome - thanks. I'll add a NEWS entry and merge.

A question: why multiple lines in .bzrignore, as opposed to just adding ./build/?

This is merged now. I made some very minor tweaks to index.rst, updated the README and added a NEWS item while merging. If there are any problems with my tweaks, please let me know.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Ian Clatworthy wrote:

> This is awesome - thanks. I'll add a NEWS entry and merge.

Great!

> A question: why multiple lines in .bzrignore, as opposed to just adding ./build/?

Maybe my bzr-fu isn't strong enough: at least with a Subversion
project, you want the 'docs/_build' directory to be in the repository
(because the Makefile won't create it), but want to ignore anything
which is created inside it by the build process. The two lines could
probably just be 'docs/_build/*'. Or did the existing 'build' entry
already prevent anything from being noticed in that directory?

Tres.
- --
===================================================================
Tres Seaver +1 540-429-0999 <email address hidden>
Palladion Software "Excellence by Design" http://palladion.com
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkvEeVQACgkQ+gerLs4ltQ5qogCgmHuwMhCwy2g09GcITpWVzj/p
ESYAoMzUkala/ODJcB4lXmKCvhrDq2d7
=jHmY
-----END PGP SIGNATURE-----

On 14/04/10 00:03, Tres Seaver wrote:

> The two lines could
> probably just be 'docs/_build/*'.

Yes. Or ./docs/_build/

I've simplified it accordingly.

> Or did the existing 'build' entry
> already prevent anything from being noticed in that directory?

No, the existing build entry will mask out ./build/ or docs/build/ but
not docs/_build.

Ian C.