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

[Mark Rada <marada@uwaterloo.ca>]

>> 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?
 
 
>> export prefix bindir sharedir sysconfdir
>>
>> CC = gcc
>> diff --git a/configure.ac b/configure.ac
>> index 914ae57..bf36c72 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -179,6 +179,26 @@ fi],
>>    AC_MSG_NOTICE([Will try -pthread then -lpthread to enable
>> POSIX Threads.])
>> ])
>>
>> +# Define option to enable JavaScript minification
>> +AC_ARG_ENABLE([jsmin],
>> + [AS_HELP_STRING([--enable-jsmin=ARG],
>> +   [ARG is the value to pass to make to enable
>> JavaScript minification.])],
>> + [
>> +   JSMIN=$enableval;
>> +   AC_MSG_NOTICE([Setting JSMIN to '$JSMIN' to enable
>> JavaScript minifying])
>> +   GIT_CONF_APPEND_LINE(JSMIN=$enableval);
>> + ])
>> +
>> +# 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).
>
> 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...

Thank you for the input.

—
Mark Rada
marada@uwaterloo.ca