git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Jeff King <peff@peff.net>, git@vger.kernel.org
Subject: Re: [PATCH 1/4] Makefile: don't re-define PERL_DEFINES
Date: Thu, 06 May 2021 11:04:34 +0200	[thread overview]
Message-ID: <87sg30usm9.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <xmqq7dkcz20u.fsf@gitster.g>


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

> Æ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.

> 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.

  reply	other threads:[~2021-05-06  9:19 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-05 12:21 [PATCH 0/4] Makefile/perl: correctly re-generate build/* + speed up under NO_GETTEXT=Y Ævar Arnfjörð Bjarmason
2021-05-05 12:21 ` [PATCH 1/4] Makefile: don't re-define PERL_DEFINES Ævar Arnfjörð Bjarmason
2021-05-05 14:08   ` Jeff King
2021-05-06  1:05     ` Junio C Hamano
2021-05-06 16:55       ` Jeff King
2021-05-06  6:23     ` Ævar Arnfjörð Bjarmason
2021-05-06  8:42       ` Junio C Hamano
2021-05-06  9:04         ` Ævar Arnfjörð Bjarmason [this message]
2021-05-06 16:59           ` Jeff King
2021-05-07  8:42           ` Junio C Hamano
2021-05-05 12:21 ` [PATCH 2/4] Makefile: regenerate perl/build/* if GIT-PERL-DEFINES changes Ævar Arnfjörð Bjarmason
2021-05-05 12:21 ` [PATCH 3/4] Makefile: regenerate *.pm on NO_PERL_CPAN_FALLBACKS change Ævar Arnfjörð Bjarmason
2021-05-05 12:21 ` [PATCH 4/4] perl: use mock i18n functions under NO_GETTEXT=Y Ævar Arnfjörð Bjarmason
2021-05-10 10:50 ` [PATCH v2 0/6] Makefile/perl: correctly re-generate build/* + speed up " Ævar Arnfjörð Bjarmason
2021-05-10 10:50   ` [PATCH v2 1/6] Makefile: regenerate perl/build/* if GIT-PERL-DEFINES changes Ævar Arnfjörð Bjarmason
2021-05-10 10:50   ` [PATCH v2 2/6] Makefile: don't re-define PERL_DEFINES Ævar Arnfjörð Bjarmason
2021-05-10 10:50   ` [PATCH v2 3/6] Makefile: make PERL_DEFINES recursively expanded Ævar Arnfjörð Bjarmason
2021-05-10 10:50   ` [PATCH v2 4/6] Makefile: split up the deceleration of PERL_DEFINES Ævar Arnfjörð Bjarmason
2021-05-10 10:50   ` [PATCH v2 5/6] Makefile: regenerate *.pm on NO_PERL_CPAN_FALLBACKS change Ævar Arnfjörð Bjarmason
2021-05-10 10:50   ` [PATCH v2 6/6] perl: use mock i18n functions under NO_GETTEXT=Y Ævar Arnfjörð Bjarmason
2021-05-10 18:17   ` [PATCH v2 0/6] Makefile/perl: correctly re-generate build/* + speed up " Junio C Hamano
2021-05-11  6:56     ` Ævar Arnfjörð Bjarmason
2021-05-11  7:05       ` Junio C Hamano
2021-05-12  9:49         ` [PATCH] Makefile: make PERL_DEFINES recursively expanded Ævar Arnfjörð Bjarmason
2021-05-12 19:53           ` Jeff King
2021-05-12 22:45             ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87sg30usm9.fsf@evledraar.gmail.com \
    --to=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=peff@peff.net \
    --subject='Re: [PATCH 1/4] Makefile: don'\''t re-define PERL_DEFINES' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).