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

From: Jakub Narebski <jnareb@gmail.com>
Date: Apr 14, 2010, 20:04 UTC
Message-ID: <201004142204.57323.jnareb@gmail.com>
In-Reply-To: <4BC614D8.2000208@mailservices.uwaterloo.ca>

On Wed, 14 April 2010, Mark Rada wrote:

> On 10-04-14 1:22 PM, Jakub Narebski wrote:

Show 14 quoted lines

> > For git-instaweb I think that best solution would be to introduce new
> > variables holding _source_ of gitweb JavaScript code and CSS, e.g.
> > 
> >             -e '/@@GITWEB_CSS@@/r $(GITWEB_CSS)' \
> > 
> > in place of
> > 
> >             -e '/@@GITWEB_CSS@@/r $(GITWEB_CSS_SOURCE)' \
> > 
> > ...although GITWEB_CSS might mean something different for Makefile
> > and git-instaweb than for gitweb/Makefile and gitweb itself.
> 
> Did you get those lines mixed up? I might be not understanding something
> here.

Ah, I'm sorry, I mixed up those two lines. They should be in reverse direction:

             -e '/@@GITWEB_CSS@@/r $(GITWEB_CSS_SOURCE)' \
 
  in place of
 
             -e '/@@GITWEB_CSS@@/r $(GITWEB_CSS)' \

Actually I'd like to rename @@GITWEB_CSS@@ placeholder etc. in git-instaweb.sh, as @@GITWEB_CSS@@ in git-instaweb.sh means something quite different from ++GITWEB_CSS++ in gitweb/gitweb.perl...

Show 9 quoted lines

> 
> I was actually planning something along the lines of 
> 
>              -e '/@@GITWEB_CSS@@/r $(GITWEB_CSS_NAME)' \
>              -e 's|@@GITWEB_CSS_NAME@@|$(GITWEB_CSS_NAME)|' \
> 
> where I introduce the GITWEB_CSS_NAME variable, to be consistent with the
> token in instaweb. This way we don't touch GITWEB_JS in the top level
> makefile.

Why not:

            -e '/@@GITWEB_CSS_SOURCE@@/r $(GITWEB_CSS_SOURCE)' \
            -e '/@@GITWEB_CSS_SOURCE@@/d' \
            ...
            -e 's|@@GITWEB_CSS_NAME@@|$(GITWEB_CSS)|' \

(assuming that $(GITWEB_CSS) does not include '|' in it, I guess... but see below).

> 
> Also, I should update dependancies for instaweb, since those were
> forgotten last time around. Just creating a short list of what the fix will
> need for when I get home tonight.

Something like

   git-instaweb: git-instaweb.sh gitweb/gitweb.cgi $(GITWEB_CSS_SOURCE) $(GITWEB_JS_SOURCE)

P.S. I have noticed additional complication: git-instaweb really needs gitweb compiled with *specific* values of GITWEB_CSS and GITWEB_JS, so that they point to git-instaweb's installed files.

-- 
Jakub Narebski
Poland