Re: [PATCHv5 2/6] Gitweb: add support for minifying gitweb.css

From: Jakub Narebski <jnareb@gmail.com>
Date: Apr 13, 2010, 22:30 UTC
Message-ID: <201004140030.47222.jnareb@gmail.com>
In-Reply-To: <4BC4D3F0.5020107@hashpling.org>

On Tue, 13 April 2010, Charles Bailey wrote:

Show 33 quoted lines

> On 01/04/2010 06:36, Mark Rada wrote:
> > @@ -84,13 +92,14 @@ endif
> >
> >   all:: gitweb.cgi
> >
> > +FILES = gitweb.cgi
> >   ifdef JSMIN
> > -FILES=gitweb.cgi gitweb.min.js
> > -gitweb.cgi: gitweb.perl gitweb.min.js
> > -else # !JSMIN
> > -FILES=gitweb.cgi
> > -gitweb.cgi: gitweb.perl
> > -endif # JSMIN
> > +FILES += gitweb.min.js
> > +endif
> > +ifdef CSSMIN
> > +FILES += gitweb.min.css
> > +endif
> > +gitweb.cgi: gitweb.perl $(GITWEB_JS) $(GITWEB_CSS)
> >
> 
> I have a question about this last line of the patch. Are GITWEB_JS and 
> GITWEB_CSS supposed to be a source path or a URI?
> 
> The documentation for install (and my previous assumption) was that they 
> represented the path on the target web server. I'm used to overriding 
> them so that gitweb.cgi can live in my /cgi-bin directory, but the 
> static files are served from /gitweb which is readable but not executable.
> 
> After this patch I had to removed $(GITWEB_JS) and $(GITWEB_CSS) from 
> the list of dependencies for gitweb.cgi otherwise make failed.
> 
> Have I got the wrong end of the stick?

Thanks a lot for noticing this bug.

GITWEB_JS and GITWEB_CSS were originally meant to be URI to file with gitweb JavaScript code and default gitweb stylesheet,... but during work on minification of JavaScript code and CSS file it somehow got confused to mean source path.

If I remember correctly the original patch, before adding required support for minified gitweb.js and gitweb.css to git-instaweb script, and before support for CSS minification had

   ifdef JSMIN
   gitweb.cgi: gitweb.perl gitweb.min.js
   else
   gitweb.cgi: gitweb.perl
   endif

which should probably be replaced in current situation by

   ifdef JSMIN
   gitweb.cgi : gitweb.min.js
   endif
   ifdef CSSMIN
   gitweb.cgi : gitweb.min.css
   endif

just adding prerequisites to gitweb.css target in gitweb/Makefile

I guess that support for adding minifiction support to git-instaweb would need to be more complicated. Perhaps

  $(notdir $(GITWEB_JS))   # Makefile function

or

  $(basename $GITWEB_JS)   # shell command

But I guess that it wouldn't work for all cases...

-- 
Jakub Narebski
Poland