Hi, Drew Northup wrote: > On Mon, 2011-06-06 at 17:12 -0500, Jonathan Nieder wrote: >> 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". 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. [...] >>> +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. >>> 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: >> 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. >>> +DISCUSSION >>> +---------- >> >> First, as a general thought, by the time I get this far I start >> thinking, "so where's the list of possible settings?". But there are >> certainly some more basic pieces of information to cover before then >> (like the global configuration file), so I think this part is in the >> right place. It just could benefit from being a little shorter. > > I used resolv.conf(5) as the model here. I realized that without a > little introduction most of the settings and the way they are applied > didn't make a heck of a lot of sense if you've never seen them before. Yes, I think the general structure is good. >>> +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? >>> +*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? 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. >>> +The syntax of the configuration files is that of Perl, as these files are [...] >>> +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. [...] >>> +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. > > The motivation for this paragraph was actually the innumerable tutorials > out there which tell people to mangle the CGI directly because: > (1) There didn't seem to be a standard documented configuration file > (2) There wasn't an appropriate place to shove %features settings that > the author could find. > (3) It really needed to be said that you might not actually have to mess > with anything. > > 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? [...] > 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? It seems my review was not very useful; sorry about that. Thanks for some clarifications. Ciao, Jonathan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html