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

From: Mark Rada <marada@uwaterloo.ca>
Date: Apr 14, 2010, 05:40 UTC
Message-ID: <4BC55558.1060608@mailservices.uwaterloo.ca>
In-Reply-To: <201004140030.47222.jnareb@gmail.com>

On 10-04-13 6:30 PM, Jakub Narebski wrote:

Show 76 quoted lines

> On Tue, 13 April 2010, Charles Bailey wrote:
>> 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...
>

Aw, frig, never thought of using gitweb like that so I made some assumptions to make things cleaner looking.

I think this can be fixed by just using different variable names? Or perhaps some nested ifdef's? I'm not sure which will be better.

I wasn't at the computer today so I'm just getting to it now, I'll try to have something when in the next day, going to bed now. Good night.

-- 
Mark Rada