Andreas Herrmann <aherrmann@xxxxxxx> 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.
