Re: [PATCH] configure.ac: Don't overwrite NO_EXPAT option

[Junio C Hamano <gitster@pobox.com>]

Andreas Herrmann <aherrmann@suse.de> writes:

> Subject: Re: [PATCH] configure.ac: Don't overwrite NO_EXPAT option

Downcase "D" in "Don't" (cf. "git log --oneline --no-merges
--since=2.months"), please.

> Even if 'configure --with-expat=no' was run, expat support is used,
> because library detection overwrites it. Avoid this overwrite.

As I suspect that "configure" is not used a part of any experienced
Git developers' daily development cycle, it is unfortunately very
understandable how this left unnoticed for so long, ever since
424adc50 (autoconf: Move variables which we always set to
config.mak.in, 2006-08-08), if I am reading the patch correctly, as
a part of the very beginning of autoconf support.

Thanks for spotting it, and the fix look straight-forward.

> (I think, configure should obey what the user has specified.)

Sure, but please rephrase this parenthesized sentence.

 * if you are unsure, and leaning to negative, say that below the
   three-dash line.  No need for (parentheses).

 * otherwise, lose the (parentheses), "I think", and in general be
   more assertive.

The latter is more preferrable.  The sentence with "I think" is your
proposal to make it the project policy to make sure that ./configure
script honors what the user gave it.  If other developers disagree
with the policy you propose there, they will object and give their
reason during their reviews.  If we adopt it as the project policy,
which I personally think is very sensible, it is good to have it
stated not as a mere personal opinion of the author of a commit, but
more as a general rule.

Having said all that, I have some further observation.

The NO_EXPAT support does not look *so* special.  It is split into
two parts:

 * Use AC_ARG_WITH(expat) to handle "--with-expat"

 * AC_CHECK_LIB([expat]) to auto-detect NO_EXPAT

Aren't there other symbols that share the same pattern?  For
example, how well do NO_CURL or NO_ICONV (which I picked because
AC_ARG_WITH(curl) comes before and AC_ARG_WITH(iconv) comes after
AC_ARG_WITH(expat)) work?  Do they share the same issue?

A quick "git grep AC_ARG_WITH" finds openssl, libpcre*, and tcltk
also share the same pattern, in addition to curl and iconv.  I may
be misreading the script but USE_LIBPCRE2 support looks OK.
NO_OPENSSL looks iffy, though not wrong per-se.  SHA1_Init is probed
in libcrypto and in libssl regardless of the value of NO_OPENSSL.

I do not think it is a good idea to address the same issue for some
of these other symbols, in addition to expat, in a single patch.
Let's have people review the current patch for NO_EXPAT first, and
leave the other symbols for possible follow-up patches.

Thanks.