Re: [PATCH 2/2] build: don't duplicate substitution of make variables

From: Stefano Lattarini <stefano.lattarini@gmail.com>
Date: Sep 11, 2012, 18:26 UTC
Message-ID: <504F824F.3050903@gmail.com>
In-Reply-To: <7vtxv4h3lh.fsf@alter.siamese.dyndns.org>

On 09/11/2012 07:27 PM, Junio C Hamano wrote:

Show 20 quoted lines

> Stefano Lattarini <stefano.lattarini@gmail.com> writes:
> 
>> Thanks to our 'GIT_CONF_SUBST' layer in configure.ac, a make variable 'VAR'
>> can be defined to a value 'VAL' at ./configure runtime in our build system
>> simply by using "GIT_CONF_SUBST([VAR], [VAL])" in configure.ac, rather than
>> having both to call "AC_SUBST([VAR], [VAL])" in configure.ac and adding the
>> 'VAR = @VAR@' definition in config.mak.in.  Less duplication, less margin
>> for error, less possibility of confusion.
>>
>> While at it, fix some formatting issues in configure.ac that unnecessarily
>> obscured the code flow.
>>
>> Signed-off-by: Stefano Lattarini <stefano.lattarini@gmail.com>
>> ---
>>  config.mak.in |  49 --------------------
>>  configure.ac  | 144 +++++++++++++++++++++++++++++++---------------------------
>>  2 files changed, 76 insertions(+), 117 deletions(-)
> 
> Whoa ;-).
>

Well, I could have converted one variable at the time, but that seemed an overkill :-)

Show 21 quoted lines

>> diff --git a/configure.ac b/configure.ac
>> index 450bbe7..da1f41f 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -267,6 +267,8 @@ AS_HELP_STRING([],           [ARG can be also prefix for libpcre library and hea
>>  	USE_LIBPCRE=YesPlease
>>  	LIBPCREDIR=$withval
>>  	AC_MSG_NOTICE([Setting LIBPCREDIR to $LIBPCREDIR])
>> +        dnl USE_LIBPCRE can still be modified below, so don't substitute
>> +        dnl it yet.
>>  	GIT_CONF_SUBST([LIBPCREDIR])
>>      fi)
>>  #
>> ...
>>  AC_CHECK_FUNC([hstrerror],
>> -	[],
>> +    [],
> 
> Is there some consistent policy regarding SP vs HT in the
> indentation you are using in this patch?
>

Basically I'm trying to follow the style of the surrounding code, while keeping in mind that in the Git codebase tabs seem to be preferred to spaces. In this case, the indentation of the following text (that was the "meat" of the expression) seemed to favour 4 spaces for indentation, so I followed suit.

> These two hunks suggest
> that you may be favoring spaces, but other places you seem to use
> tabs, so...
>

I can convert the new tabs to spaces if you prefer (that would have been my preference too, but thought trying to follow the "Git preferences" was more important). No big deal either way for me.

Thanks,
  Stefano