On Thu, May 06 2021, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 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.
> 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.
