Re: [PATCH] Makefile: make NO_ICONV really mean "no iconv"

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jun 15, 2018, 07:43 UTC
Message-ID: <CAPig+cQL8rTg+GASp2tSng7PPPYkfeeV2SNyi0D+6-Ep7JKaGg@mail.gmail.com>
In-Reply-To: <20180615065805.GA15146@ruderich.org>

On Fri, Jun 15, 2018 at 2:58 AM Simon Ruderich <simon@ruderich.org> wrote:

Show 8 quoted lines

> On Thu, Jun 14, 2018 at 10:25:03PM -0400, Eric Sunshine wrote:
> > This patch is extra noisy due to the indentation change. Viewing it with
> > "git diff -w" helps. An alternative to re-indenting would have been to
> > "undefine NEEDS_LIBICONV", however, 'undefine' was added to GNU make in
> > 3.82 but MacOS is stuck on 3.81 (from 2006) so 'undefine' was avoided.
>
> Should we put the part about MacOS's make into the commit
> message? Seems like relevant information for future readers.

No. The bit of commentary mentioning MacOS's very old 'make' was just talking about a possible alternate way of implementing the change. That alternative was not chosen, so talking about old 'make' in the commit message would be confusing for readers. More importantly, although that alternative would have made a less noisy patch, the actual result would have made the Makefile itself noisier and uglier, particularly for people just reading the Makefile in the future, people who did not read the patch. Specifically, these alternatives were considered:

    ifdef NO_ICONV
        undefine NEEDS_LIBICONV
    endif
    ifdef NEEDS_LIBICONV
        ...as before...
    endif

and:

    ifdef NO_ICONV
        NEEDS_LIBICONV=
    endif
    ifeq ($(NEEDS_LIBICONV),)
        ...as before...
    endif

Both of which are uglier for a future reader of Makefile than the end-result actually implemented by this patch:

    ifndef NO_ICONV
        ifdef NEEDS_LIBICONV
            ...as before...
        endif
    endif