Re: [PATCH 1/4] Makefile: don't re-define PERL_DEFINES

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: May 6, 2021, 09:04 UTC
Message-ID: <87sg30usm9.fsf@evledraar.gmail.com>
In-Reply-To: <xmqq7dkcz20u.fsf@gitster.g>

On Thu, May 06 2021, Junio C Hamano wrote:

Show 23 quoted lines

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>>>> -PERL_DEFINES = $(PERL_PATH_SQ):$(PERLLIB_EXTRA_SQ):$(perllibdir_SQ)
>>>> -
>>>> -PERL_DEFINES := $(PERL_PATH_SQ) $(PERLLIB_EXTRA_SQ) $(perllibdir_SQ)
>>>> +PERL_DEFINES :=
>>>> +PERL_DEFINES += $(PERL_PATH_SQ)
>>>> +PERL_DEFINES += $(PERLLIB_EXTRA_SQ)
>>>> +PERL_DEFINES += $(perllibdir_SQ)
>>>>  PERL_DEFINES += $(RUNTIME_PREFIX)
>>>
>>> I don't think we generally use simply-expanded variables in our Makefile
>>> unless there's a reason. Do we actually need it here? Obviously not new
>>> in your patch, but just a curiosity I noticed while reading it.
>>
>> I didn't notice it at the time. I suppose it could be changed to not do
>> expansion, but per-se unrelated to the more narrorw bugfix in this
>> patch.
>
> Actually, strictly speaking there was *no* bug because assigning
> three items with := made sure the previous recursively expanded one
> to be ineffective.  In other words, there was a valid reason to use
> ":=" there in the original version.

Yes, there wasn't any bug with the the eventual value being incorrect. I.e. both of these are equivalent in a Makefile:

    FOO = abc
    FOO := def
    FOO += ghi

And:

    FOO = abc
    FOO = def
    FOO += ghi

Both will yield "def ghi". They're just different in a case like:
    
    X = Y
    FOO = abc
    FOO := $(X)
    X = Z
    FOO += ghi

Where using := will echo "Y ghi", and using = will echo "Z ghi". As a practical matter the distinction doesn't matter in this case.

Show 6 quoted lines

> Now your patch removed the recursively expanded one that was
> immediately invalidated, there no longer is a reason to use :=
> there.  So "unrelated to the more narrow bugfix" is a rather lame
> excuse to do only half a task.  If we remove that extra one (which
> is a good thing), then we should correct := into = because the
> original used := only because there was the unwanted extra one, no?

I don't see how removing the stray line changes the reason to use ":=" or "=" there. I agree it should be removed, it's just unrelated to removing the stay line. Looking at 07d90eadb50 it's clear that it's just some copy/pasting error.

Maybe the confusion is that I'm using "bug" closer to a meaning of "a thing nobody intended to be in the program", not just "a behavior-changing issue observable from the outside".

In any case. I can just submit a patch on top of this in a v2. I continue to find it hard to discover the line between superfluous while-we're-at-it fixes in your mind v.s. "we should fix this while we're at it" though :)

But regarding the "half a task" it seems to me that these are different issues; I don't think that's a point worth arguing in this case specifically (let's just fix it, and I will), but perhaps I'm missing something subtle with regards to Makefile semantics per my examples above so it really is all one issue, and I'd like to understand how they're entwined.