Re: [RFC/PATCHv3 4/5] gitweb: Starting work on a man page for /etc/gitweb.conf (WIP)

On Tue, 2011-06-07 at 08:44 -0500, Jonathan Nieder wrote:

Hi,

Drew Northup wrote:

quoted

On Mon, 2011-06-06 at 17:12 -0500, Jonathan Nieder wrote:

quoted

quoted

Micronit: a single line like

	SYNOPSIS
	--------
	$GITWEBDIR/gitweb_config.perl, /etc/gitweb.conf

might fit better with the pattern established by gitattributes(5) and
its kin.

I thought that's how I had originally put it.

I take that as a "yes, it would work better".

Agreed, it would.

To save others the trouble of digging previous rounds up for
comparison:

 - the original: http://thread.gmane.org/gmane.comp.version-control.git/173422
 - v2: http://thread.gmane.org/gmane.comp.version-control.git/174700/focus=174701
 - v3: http://thread.gmane.org/gmane.comp.version-control.git/175140/focus=175145

The original had one line but it didn't mention
$GITWEBDIR/gitweb_config.perl.  So we are slowly converging.

Thanks for looking that up (and the refs above).

[...]

quoted

quoted

quoted

+The configuration file is used to override the default settings that
+were built into gitweb at the time 'gitweb.cgi' script was generated.
+
+While one could just alter the configuration settings in the gitweb
+CGI itself, those changes would be lost upon upgrade.

What if I upgrade using RCS "merge"? :)  So maybe:

	s/one could just alter/one can alter/
	s/would be lost/could be lost/

to clarify that (1) editing the CGI script is allowed and fine but (2)
it might be a pain to keep those changes.

The point of this was originally if you were a mere mortal then upgrades
of the gitweb CGI would flush all of the settings you just spent a week
getting right down the drain. That's the point of the stronger language.
It's meant to be a solid "If you do it that way you can expect it to
hurt" message.

Sorry, I suppose I was unclear.  All I meant is that as a naïve
reader, I found myself losing trust with "those changes would be lost
upon upgrade".  Oddly enough, by contrast something like:

	While it would be possible to change the gitweb CGI directly
	instead, DON'T DO THAT!  Your changes would be lost on upgrade!

would have worked fine for me, since there are enough cues to know I
am reading hyperbole rather than an explanation of mechanism.

Anyway, it was just how I read it, and it's likely my proposed change
would have made it worse in some other way.

Your complaint is valid from the perspective you just outlined. The
point I was trying to drive home was that you can expect the upgrade
process to replace the executable because that's what upgrades do--they
replace executables. Obviously stating it that plainly to somebody
unfamiliar with how the whole install/upgrade process works at the
filesystem level isn't going to be very helpful. Perhaps a combination
of the "one can alter" and "are likely to be overwritten [upon upgrade]"
would do the trick?

quoted

quoted

quoted

Configuration
+settings might also be placed into a file in the same directory as the
+CGI script with the default name 'gitweb_config.perl' -- allowing
+one to have multiple gitweb instances with different configurations by
+the use of symlinks.

Might also in addition to what?  

This comment is nonsensical, please clarify it.

I found the sentence starting with "Configuration settings" hard to
understand because the "also" didn't have an obvious antecedent.
Therefore I proposed some alternative phrasing:

quoted

quoted

Continuing the thought from before:

	So gitweb allows settings to be placed in a separate file named
	'gitweb_config.perl' in the same directory as the CGI script.
	This also allows one to set up multiple gitweb instances with
	different configurations by using symlinks to a common gitweb.cgi
	script.

How is this any different from what I wrote?

I'm starting to sense annoyance.  Please feel free to ignore my
comments if you think they're unhelpful; they're meant as a gift, not
demands.

Anyway, the difference is in phrasing.  Saying "So" is meant to make
the relationship to the previous sentence clearer: we are explaining
the purpose of the configuration file, by contrasting it with editing
the CGI script directly.  The next sentence is a separate thought, so
I thought it might be clearer to put it in a separate sentence.

Yes, annoyance would be the right word. I was trying very hard to find
how what you were proposing was different and not finding it (other than
what appeared to me to be dealing with the reader in a disrespectful
manner). Now that I know where you were aiming I can think about an
appropriate change. (I'm not there yet, but it may very well come to
me.)
In any case, these are things that I'd need to see not in patch context,
but as part of the larger file to make much sense of. I seem to recall
that something is missing here that was prior to the words
"Configuration settings" in my original, which I'll have to check on. If
that is the case it would explain the lack antecedent clarity you noted.
If not then the text could be made more clear unto itself.

quoted

quoted

quoted

+The location of system-wide gitweb configuration file is defined at compile
+time using the configuration value `GITWEB_CONFIG_SYSTEM` and defaults to
+'/etc/gitweb.conf'.  The name of the per-instance configuration file is
+defined in gitweb by `GITWEB_CONFIG`, and defaults to 'gitweb_config.perl'
+(relative path means located in the same directory gitweb is installed).

Maybe:

	In addition to the per-instance configuration file, there can
	be a system-wide configuration file to act as a fallback when
	the per-instance configuration file does not exist.

	The system-wide configuration file is named /etc/gitweb.conf
	by default.  Filenames for the system-wide and per-instance
	configuration variables can be overridden at compile time and
	run time; see the FILES section for details.

This is the manpage for the system wide configuration file. If you'd
like to scrap this effort in favor of something else please speak up.

Huh?

If this is what you are proposing then we should be working on a
"gitweb_config.perl" manpage and not a "gitweb.conf" manpage. I know a
fair number of people around here put priority on the former and would
just as soon ignore the latter. That's what your proposed change says to
me (while I also understand that your own position is likely far more
nuanced than that).

quoted

quoted

quoted

+*NOTE:* If per-instance configuration file ('gitweb_config.perl') exists,
+then system-wide configuration file ('/etc/gitweb.conf') is _not used at
+all_!!!

Over the top. :)  I think the best way to document this is to contrast
it with /etc/gitweb-common.conf once the latter exists.

If we were to change gitweb to handle configuration files like the rest
of git (and in fact like most things we deal with daily, where settings
are overridden one by one) then this section becomes moot. Until or
unless that becomes the case it is important to loudly make note of it. 

Does using three exclamation marks and italics make it clearer?

That could probably be cut down to one, I'll grant you that. I was
trying to avoid use of the <blink> tag.

Previously there was no manpage documenting this at all, so I think
explaining it is already a big and good step.  If this needs to be
so prominent that people just skimming through will notice it, then
I'd suggest putting it in the DESCRIPTION section higher up.  But I'm
not the one writing this; others are free to disagree.

I thought seriously about doing that but decided against it as it took
this important bit of information out of the appropriate context. If I
don't present that there's more than one way to configure gitweb first
this text by itself doesn't make a whole lot of sense (and therefore
will likely be ignored).

quoted

quoted

quoted

+The syntax of the configuration files is that of Perl, as these files are

[...]

quoted

quoted

quoted

+page for more information.

Duplicates DESCRIPTION.

Perhaps it does, but not everybody is a Perl coder. It was that way in
the text I started from and I saw little point in changing it.

No, the paragraph is actually repeated from the DESCRIPTION section.

Well then, I suppose that is a problem! I don't recall doing that, but
it should be corrected. 
(As noted below and prior, I haven't been able to apply the interim
patches to my @home working copy, so if it came from there I currently
don't know it.)

[...]

quoted

quoted

quoted

+The default configuration with no configuration file at all may work
+perfectly well for some installations.  Still, a configuration file is
+useful for customizing or tweaking the behavior of gitweb in many ways, and
+some optional features will not be present unless explicitly enabled using
+the configurable `%features` variable (see also "Configuring gitweb
+features" section below).

I suppose this is based on the text

	The most notable thing that is not configurable at compile time are the
	optional features, stored in the '%features' variable.

I'd suggest removing the paragraph.

<snip>

quoted

Apparently you think that clearing these misconceptions up is a useless
exercise, or at least it sounds a lot like it.

What?  I wrote exactly what I was thinking, which was that I thought this
paragraph was based on the text I quoted from gitweb/INSTALL and that
contrasting the run-time and compile-time configuration as that
paragraph did didn't seem very important for the installed manpage
(since the reader might not have been involved in the installation at
all).

If the point was actually to say "Contrary to popular belief, you can
set %features in the configuration file instead of hard-coding it in
the CGI script itself", why not say that directly?

Ok, we can do that (or something very much like it, whatever ends up
fitting best). Remember that this was sourced prior to our on-list
discussion about what came from gitweb/INSTALL being primarily fodder
for a different document. I haven't had the chance to modify it since,
and I have the impression that Jakub has been busy working on other
parts of this project (of which he's done quite a bit). Removing the
paragraph wholesale doesn't fix the problem so far as I see it, but the
presentation of the information can certainly be better tuned.

[...]

quoted

I cannot keep up with this pace of patching right now.

No problem.  For what it's worth, I skipped a couple of rounds of
reading, too; I suppose they came quickly.  Does that mean you don't
want to be cc-ed on later incarnations of the patch, or would you like
to stay notified?

I'd like to stay cc'd, I just haven't been able to apply the previous
patch versions to my @home work tree before the next has come out,
limiting my chances to be able to effectively review them.

I apologize for being a bit snappy, but sometimes I really do have a low
tolerance for comments that just seem (to me) to be "this here is really
useless" complaints and not suggestions of an upgrade.
-- 
-Drew Northup
________________________________________________
"As opposed to vegetable or mineral error?"
-John Pescatore, SANS NewsBites Vol. 12 Num. 59