Re: [PATCH v2 3/9] Makefile: have "make pot" not "reset --hard"

From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Jiang Xin <worldhello.net@gmail.com>
Cc: "Junio C Hamano" <gitster@pobox.com>,
	"Git List" <git@vger.kernel.org>,
	"Alexander Shopov" <ash@kambanaria.org>,
	"Jordi Mas" <jmas@softcatala.org>,
	"Matthias Rüster" <matthias.ruester@gmail.com>,
	"Jimmy Angelakos" <vyruss@hellug.gr>,
	"Christopher Díaz" <christopher.diaz.riv@gmail.com>,
	"Jean-Noël Avila" <jn.avila@free.fr>,
	"Bagas Sanjaya" <bagasdotme@gmail.com>,
	"Alessandro Menti" <alessandro.menti@alessandromenti.it>,
	"Gwan-gyeong Mun" <elongbug@gmail.com>, Arusekk <arek_koz@o2.pl>,
	"Daniel Santos" <dacs.git@brilhante.top>,
	"Dimitriy Ryazantcev" <DJm00n@mail.ru>,
	"Peter Krefting" <peter@softwolves.pp.se>,
	"Emir SARI" <bitigchi@me.com>,
	"Trần Ngọc Quân" <vnwildman@gmail.com>,
	"Fangyi Zhou" <me@fangyi.io>, "Yi-Jyun Pan" <pan93412@gmail.com>,
	"Jiang Xin" <zhiyou.jx@alibaba-inc.com>
Subject: Re: [PATCH v2 3/9] Makefile: have "make pot" not "reset --hard"
Date: Thu, 19 May 2022 16:06:42 +0200	[thread overview]
Message-ID: <220519.86a6bd4mft.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <CANYiYbGpMx8JkHj2EMxYYk6KgdpiPr5=7VWEXKevp+J+e7NKcQ@mail.gmail.com>

On Thu, May 19 2022, Jiang Xin wrote:

> On Thu, May 19, 2022 at 5:53 PM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>> In the previous discussion of kicking things around I lost track of what
>> version of mine this is picked up from, but I range-diff'd it to my
>> 6cf9c1f7022 (Makefile: have "make pot" not "reset --hard", 2022-04-02),
>> which is the latest I had in avar/Makefile-incremental-po-git-pot-rule
>> on my branch.
>>
>> A range-diff of the two follows below (yours being the RHS). Some
>> comments:
>>
>>  * There's a bug here where you're creating .build/pot/po/pretty.c.po
>>    files, not .build/pot/po/pretty.c, i.e. you add a *.po extension.
>
> In the original version of your commit, each source file has a
> duplicate version in the ".build/" directory, and this will confuse
> IDE (E.g.: VS Code) when I jump to a function declaration.
>
> Name the "pot" snippets with the ".po" extension only have the
> following side effect, nothing else:
>
>             +#. #-#-#-#-#  add-patch.c.po  #-#-#-#-#
>              #. TRANSLATORS: do not translate [y/n]
>             [...]
>             +#. #-#-#-#-#  git-add--interactive.perl.po  #-#-#-#-#

I don't think we should sacrifice correctness for such an IDE
workaround, which surely will just bite someone else in a different
way. I.e. now if I'm auto-completing .po<TAB> in a hypothetical naïve
IDE that's looking in .build/ I'll get these built files instead.

The right solution in both cases being that the IDE in question should
be ignoring things covered by .gitignore, or perhaps we could ship
something in contrib/ for specific IDEs?

There's also an easy workaround we can do on the GNU make side. Unless
you mark files as PRECIOUS it'll delete files that are only used for
intermediate targets.

We could thus make the chain e.g.:

    git.c -> .build/pot/po/git.c -> .build/pot/po/git.c.done 

Instead of:

    git.c -> .build/pot/po/git.c -> .build/pot/po/git.c.done

Where the only point of the ".build/pot/po/git.c.done" is to have make
delete the intermediate file as soon as it's done with it.

Or, since the comments all start with #. or #: we could post-munge them
with "sed" I guess.

> I add some notes in commit message:
>
>     While we could rename the "pot" snippets without the ".po" extension
>     to use more intuitive filenames in the comments, but that will
>     confuse the IDE with lots of invalid C or perl source files in
>     ".build/pot/po" directory.

Ah, I see that after you pointed out. I'd skimmed entirely over the
commit message thinking it must have been something I wrote, and didn't
notice that edit. Sorry.

>>  * We went a bit back & forth on the "if grep -q PRItime" part on the GH
>>    ticket. FWIW I still think just skipping that work is a better
>>    choice. Yes we'll have ~10MB of redundant files in .build, and it's
>
> Redundant source files (*.c, *.h, *.perl) in .build will make IDE mad.

Hopefully we can have our cake & eat it too here, per the above. 

>>    marginally slower, but "make pot" isn't a hot target, better to
>>    optimize for simplicity.
>>
>>    But if you're really set on having it I don't mind...
>>
>>  * You add a "MSGCAT_FLAGS = --sort-by-file" here, maybe worth having
>>    some "common" flags variable in the earlier commit we can use here?
>>    I.e. share --sort-by-file with xgettext.
>>
>>  * Your version is missing FORCE on po/git.pot, which is a bug. We can't
>>    omit it on any file that's checked in. We're about to "git rm" it
>>    anyway, so maybe we shouldn't worry about it though...
>
> I'm confused. Since the "po/git.pot" target has a full set of
> prerequisites, it is fine to remove FORCE from dependence.

At this point in the series po/git.pot is still a file tracked by git.

Thus:

    make pot
    git reset --hard HEAD^
    make pot

Or whatever will report that there's nothing to do, since the timestamp
of the "generated" file will be newer than that of what it depends on.

That's why my version pulled that dance with having the
.build/pot/git.pot be non-FORCE and the po/git.pot be FORCE until it was
git rm'd.