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

Stefano Lattarini [off-list ref] writes:

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

quoted

Stefano Lattarini [off-list ref] writes:

quoted

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 <redacted>
---
 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 ;-)

quoted

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.

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

Stefano Lattarini [off-list ref] writes:

quoted

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

quoted

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,

There is no general standard about it that I know of; it's just that
GNU projects tend to prefer space-based indentation over tab-based one,
and since I mostly touch configure.ac files from GNU projects, I've
picked up the habit.

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.

Then I might send a patch that normalize indentation in configure.ac
to "spaces only", if that's OK with you.  But that's obviously for a
separated thread.

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.

The reason those few clean-ups are mixed into this patch is that the
pre-existing strange indentation style was actually making it more
difficult for me to grasp the code flow; that is, I didn't see them
as a cosmetic change, but as a way to make it easier for me and the
reader to see that my changes were correct and sensible.

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

OK.  Just let me know if you'd still prefer to have the indentation
cleanups done by a preparatory patch, and I'll send a re-roll.

Thanks,
  Stefano