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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: May 19, 2022, 14:06 UTC
Message-ID: <220519.86a6bd4mft.gmgdl@evledraar.gmail.com>
In-Reply-To: <CANYiYbGpMx8JkHj2EMxYYk6KgdpiPr5=7VWEXKevp+J+e7NKcQ@mail.gmail.com>

On Thu, May 19 2022, Jiang Xin wrote:

Show 25 quoted lines

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

Show 6 quoted lines

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

Show 5 quoted lines

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

Show 15 quoted lines

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