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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 11, 2012, 19:52 UTC
Message-ID: <7vd31sgww4.fsf@alter.siamese.dyndns.org>
In-Reply-To: <504F824F.3050903@gmail.com>

Stefano Lattarini <stefano.lattarini@gmail.com> writes:

Show 23 quoted lines

> On 09/11/2012 07:27 PM, Junio C Hamano wrote:
>> 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 :-)

No, I was happy to see many lines go ;-)

Show 7 quoted lines

>> 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.

If this were other parts of the system, my preference would be to use tabs, but because I do not help very much in the autoconf part myself, I do not have a particular preference. If it is more common to indent the configure.ac script with spaces, that would be more familiar to the folks who work on it, and I do not have much against choosing and sticking to space indented configure.ac file if that is the policy.

But if this patch is not about cleaning up the style to make it conform to a policy (whichever it is), I would have preferred to see a clean-up patch as a separate step, not mixed together with this.

That's all; either way, no big deal.