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.