Re: [PATCHv5 3/6] Gitweb: add autoconfigure support for minifiers

From: Jakub Narebski <jnareb@gmail.com>
Date: Apr 2, 2010, 22:08 UTC
Message-ID: <201004030008.43513.jnareb@gmail.com>
In-Reply-To: <4BB6294A.7020800@mailservices.uwaterloo.ca>

Mark Rada wrote:

Show 12 quoted lines

>>> Makefile        |    4 ----
>>> configure.ac    |   20 ++++++++++++++++++++
>>> gitweb/Makefile |   14 ++------------
>>> 3 files changed, 22 insertions(+), 16 deletions(-)
>>
>> Why there is no change to config.mak.in?  I would thought that it
>> would contain JSMIN=@JSMIN@ etc.
>>
>> But see also below: perhaps current version is a better version.
>  
> I don't understand what you mean by this. Are you saying that the
> patch is good as is?

Yes, I think that while it could be improved a bit, it is good as it is now.

I'm sorry for causing confusion: at first I was wondering why you do not use JSMIN=@JSMIN@ etc. in config.mak.in, but then I noticed that other existing configuration uses "add to config.mak.append" trick.

[...]

Show 14 quoted lines

>>> +# Define option to enable CSS minification
>>> +AC_ARG_ENABLE([cssmin],
>>> + [AS_HELP_STRING([--enable-cssmin=ARG],
>>> +   [ARG is the value to pass to make to enable CSS minification.])],
>>> + [
>>> +   CSSMIN=$enableval;
>>> +   AC_MSG_NOTICE([Setting CSSMIN to '$CSSMIN' to enable CSS minifying])
>>> +   GIT_CONF_APPEND_LINE(CSSMIN=$enableval);
>>> + ])
>>> +
>>
>> Why not follow the code as it was done e.g. for iconv (--without-iconv
>> and --with-iconv=path); this would require JSMIN=@JSMIN@ in 
>> config.mak.in (and respectively for CSSMIN).

This is about alternate solution; please discard this question.

Show 10 quoted lines

>> Alternatively, if you decide on appending to config.mak.autogen (by the
>> way of config.mak.append) instead of filling config.mak.in, why not use
>> ready macro GIT_PARSE_WITH_SET_MAKE_VAR?
>  
> I think this is what I am really not understanding here. Are you saying
> that you think it would be better to use —with-OPT=PATH instead of
> —enable-OPT=PATH? 
> 
> Is this just a preference? I'm not seeing the problem with —enable-OPT..
> This is confusing me a bit...

Well, I haven't noticed that GIT_PARSE_WITH_SET_MAKE_VAR uses —with-OPT=PATH and not —enable-OPT=PATH.

What would be nice to have is GIT_PARSE_ENABLE_SET_MAKE_VAR macro... but for only two callsites it might be overkill.

-- 
Jakub Narebski
Poland