Re: [PATCHv3 2/5] gitweb: Describe CSSMIN and JSMIN in gitweb/INSTALL

From: Jakub Narebski <jnareb@gmail.com>
Date: Jun 8, 2011, 11:32 UTC
Message-ID: <201106081332.19008.jnareb@gmail.com>
In-Reply-To: <20110606200126.GB30588@elie>

On Mon, 6 June 2011, Jonathan Nieder wrote:

Thank you very much for your review and comments.

Show 10 quoted lines

> Jakub Narebski wrote:
> 
> > The build-time configuration variables JSMIN and CSSMIN were mentioned
> > only in Makefile; add their description to gitweb/INSTALL.
> >
> > This required moving description of GITWEB_JS up, near GITWEB_CSS and
> > just introduced CSMIN and JSMIN.
> 
> Why does this require that?  Ah, to make the analogy with GITWEB_CSS
> clear and because the JSMIN description refers to it.

Yes, moving description of GITWEB_JS was required because I wanted to cover both of CSSMIN and JSMIN with one entry, which entry of course refers to both GITWEB_CSS and GITWEB_JS.

Show 26 quoted lines

> > --- a/gitweb/INSTALL
> > +++ b/gitweb/INSTALL
> > @@ -147,6 +147,19 @@ You can specify the following configuration variables when building GIT:
> [...]
> > + * CSSMIN, JSMIN
> > +   Invocation of a CSS minifier or a JavaScript minifier, respectively,
> > +   working as a filter (source on standard input, minified result on
> > +   standard output).  If set, it is used to generate a minified version of
> > +   'static/gitweb.css' or 'static/gitweb.js', respectively.  *Note* that
> > +   minified files would have *.min.css and *.min.js extension, which is
> > +   important if you also set GITWEB_CSS and/or GITWEB_JS.  [No default]
> 
> When I first read this, I thought it meant these command lines were
> going to be cooked into the gitweb script and invoked at run time.
> Maybe (sorry for the rough text):
> 
> * CSSMIN, JSMIN
>   Command for a CSS minifier or a Javascript minifier, working as a
>   filter [...]
>   These are used if defined to generate smaller, non human-readable
>   versions of 'gitweb/gitweb.css' and 'static/gitweb.js' at
>   'static/gitweb.min.css' and 'static/gitweb.min.js'.  Only the minified
>   versions are installed, which is important if you also set GITWEB_CSS
>   or GITWEB_JS.  [No default]
> 
> Aside from that, looks good.  Thanks.

Thanks. I'll incorporate those comments in next round... or as a separate improvement (it is not that bad, I think, to not allow it to be fixed "in tree").

Anyway the description could use some improvements. We need to cover the following issues:

1. CSSMIN and JSMIN are invoked during building gitweb.

2. CSSMIN and JSMIN are interpreted as shell commands, so
   * if you refer to script by full path, you need to quote spaces
     yourself, e.g.

     JSMIN="'c:/Program Files/JSMin/jsmin.exe'"

   * you can provide options, so you can e.g. use

     JSMIN="perl -MJavaScript::Minifier=minify -we 'minify(input => *STDIN, output => *STDOUT);'"

3. CSSMIN and JSMIN must accept source on standard input, and print
   minified version to standard output, i.e. they are invoked as

     $(CSSMIN) <$< >$@

4. Minified files will be named static/gitweb.min.css and
   static/gitweb.min.js, respectively

5. If you do not set GITWEB_CSS and GITWEB_JS, but use CSS and/or JSMIN,
   they would be set to appropriate values automatically

6. The "install" target in gitweb Makefile, and "install-gitweb" target
   in main Makefile, would install minified versions (if any).

7. If you set both CSSMIN and GITWEB_CSS, you have to adjust GITWEB_CSS
   by yourself.  Same for JSMIN and GITWEB_JS.

Help with wording those would be much appreciated, in more compact form. Thanks in advance.

-- 
Jakub Narebski
Poland (not native English speaker)