Re: [PATCH] Procedure for making web page changes

Gabriela Gibson wrote on Wed, Feb 20, 2013 at 16:22:36 +0000:
> On 20/02/13 15:28, Gabriela Gibson wrote:
>> On 20/02/13 07:24, Daniel Shahaf wrote:
>>> <Directory /x1/www/subversion.apache.org>
>>
>> Thank you :)
>>
>> The patch and log are attached.
> Sorry, I'm just hooked on phonics at the moment (and the virus I'm still
> wrestling is probably innocent), a correction is attached.

> I added the log file again and also updated the number just so it's
> all neat.

Excellent.

> +++ publish/.htaccess (working copy)
> @@ -1,7 +1,6 @@
> # duplicated in httpd.conf in r795618
> Options +Includes
>
> -XBitHack On
> RedirectMatch ^/buildbot(.*) http://ci.apache.org/waterfall?\
> \&show=bb-openbsd\
> \&show=svn-slik-w2k3-x64-local\

You might want to commit this part separately? It isn't as much related
to the other changes as something we found while working on them.

> +++ publish/docs/community-guide/community-guide.html (working copy)
> @@ -73,6 +77,7 @@ first.</p>
> <!--#include virtual="roles.part.html"-->
> <!--#include virtual="conventions.part.html"-->
> <!--#include virtual="building.part.html"-->
> +<!--#include virtual="web.part.html"-->
> <!--#include virtual="debugging.part.html"-->

You seem to have added "web" before "debugging" here, while it's after
"debugging" everywhere else.

> +++ publish/docs/community-guide/web.part.html (revision 0)

A couple of minor comments about this file:

> +<p>Putting it all together, an example VirtualHost configuration is:</p>
> +
> +<pre>&lt;VirtualHost *:8080&gt;
> + ServerAdmin webmaster_at_localhost

Consider putting this example in a separate *.conf file, to make it
easier to reuse?

'diff -u ~/projects/svn/site/**/svnsite.conf /etc/apache2/svnsite.conf'

> +<p>then upload the resulting file to a HTML validatory, for

"validatory"?

> +<p> list the names of files modified in the patch in the log message,
> + relative to the <code>site/</code> directory and list anchors of
> + sections added or modified like this:
> + <pre>
> + * docs/community-guide/some_page.html
> + (section-name): fixed issue xyz

I usually write '(#section-name)', but a quick log inspection suggests
I'm in a minority in that.

> [[[
>
> Addition of section to Community Guide describing procedure for

Use the imperative: "Add a section"

> obtaining the source for the Subversion web site. Addition of
> navigation links to new page within community guide.
>
> Add three html files creating one extra page on web site accessible at
> /docs/community-guide/web.html. Also add README in top level directory
> providing link to new web page.
>
> * publish/docs/community-guide/web.html
> (New page): Provide the top-level html page for the new page.
>
> * publish/docs/community-guide/web.toc.html
> (New page): Provide table-of-contents for new page.
>
> * publish/docs/community-guide/web.part.html
> (New page): Provide body text for new page
> (Introduction): Provide brief overview of structure of web pages on
> subversion site.
> (web_sources): Provide instructions for accessing web sources via
> https or svn.
> (web_mirror): Provide instructions for creating mirror of Subversion
> site with Apache.
> (web_validation): Provide instructions for validating changes.
> (web_patch_creation): Describe format of commit log message for
> web changes.
>
> * README
> (New file): Provide link to new /docs/community-guide/web.html page.

You didn't include README in the patch.

Bottom line: once the points about README and .htaccess are addressed,
+1 to commit. (Everything else can be addressed post-commit if you
disagree with the review.) Thanks or the patch!
Received on 2013-02-20 19:55:09 CET