Re: [PATCH] Makefile: dedup git-ls-files output to prevent duplicate targets

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>,
	"Jiang Xin" <zhiyou.jx@alibaba-inc.com>,
	"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>
Subject: Re: [PATCH] Makefile: dedup git-ls-files output to prevent duplicate targets
Date: Thu, 26 May 2022 12:00:04 +0200	[thread overview]
Message-ID: <220526.86tu9c625s.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <CANYiYbGn08N_9bOw+ss6L4U_iTomc-08_961bk40eq1BnEstiw@mail.gmail.com>

On Thu, May 26 2022, Jiang Xin wrote:

> On Thu, May 26, 2022 at 2:23 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Jiang Xin <worldhello.net@gmail.com> writes:
>>
>> >>         )
>> >> -FOUND_SOURCE_FILES := $(shell $(SOURCES_CMD))
>> >> +FOUND_SOURCE_FILES := $(sort $(shell $(SOURCES_CMD)))
>> >>
>> >>  FOUND_C_SOURCES = $(filter %.c,$(FOUND_SOURCE_FILES))
>> >>  FOUND_H_SOURCES = $(filter %.h,$(FOUND_SOURCE_FILES))
>> >>
>> >
>> > If I disabled the git-ls-files command like below,
>> >
>> >     @@ -846,7 +846,7 @@ generated-hdrs: $(GENERATED_H)
>> >...
>> > This is because the three generated header files (defined in
>> > $(GENERATED_H)) are also included in the result of "SOURCES_CMD". We
>> > can fix this by sorting LOCALIZED_C:
>> >
>> >     -LOCALIZED_C = $(FOUND_C_SOURCES) $(FOUND_H_SOURCES) $(SCALAR_SOURCES) \
>> >     -             $(GENERATED_H)
>> >     +LOCALIZED_C = $(sort $(FOUND_C_SOURCES) $(FOUND_H_SOURCES)
>> > $(SCALAR_SOURCES) \
>> >     +               $(GENERATED_H))
>>
>> If you make FOUND_SOURCE_FILES unique upfront, the at least there
>> wouldn't be any duplicates there.  Do you mean that some of what is
>> in FOUND_SOURCE_FILES appear in either SCALAR_SOURCES or GENERATED_H?
>
> Yes. If find source files using "git-ls-files", the generated headers
> are not included in FOUND_SOURCE_FILES, but this is not the case for
> the find command.
>
>> If not, I think deduplicating near the source of the issue, i.e.
>>
>>   FOUND_SOURCE_FILES := $(sort $(shell $(SOURCES_CMD)))
>
> If we pass the "--deduplicate" option to git-ls-files, we can make
> sure files are unique in FOUND_SOURCE_FILES. So sorting
> FOUND_SOURCE_FILES becomes unnecessary. But FOUND_SOURCE_FILES may
> contain generated files if using find instead of git-ls-files in
> SOURCES_CMD, that means sort FOUND_SOURCE_FILES is not enough. Instead
> of sorting it, we may want to filter-out the generated files from it,
> like:
>
>     FOUND_SOURCE_FILES := $(filter-out $(GENERATED_H),$(shell $(SOURCES_CMD)))
>
> Exclude the several generated headers by add extra fixed options to
> find command is not good, but we may need to exclude the ".build"
> directory from the find command in SOURCES_CMD, in case we may
> duplicate C files in it in future version.
>
>
>     --- a/Makefile
>     +++ b/Makefile
>     @@ -846,7 +846,7 @@ generated-hdrs: $(GENERATED_H)
>     ## Exhaustive lists of our source files, either dynamically generated,
>     ## or hardcoded.
>     SOURCES_CMD = ( \
>     -       git ls-files \
>     +       git ls-files --deduplicate \
>                    '*.[hcS]' \
>                    '*.sh' \
>                    ':!*[tp][0-9][0-9][0-9][0-9]*' \
>     @@ -857,12 +857,13 @@ SOURCES_CMD = ( \
>                    -o \( -name '[tp][0-9][0-9][0-9][0-9]*' -prune \) \
>                    -o \( -name contrib -type d -prune \) \
>                    -o \( -name build -type d -prune \) \
>     +               -o \( -name .build -type d -prune \) \

This change is good and something we should do in any case, I think (and
this is my fault) that due to this we'd have broken other things,
e.g. included these in the TAGS file if we don't have "git ls-files".

>                    -o \( -name 'trash*' -type d -prune \) \
>                    -o \( -name '*.[hcS]' -type f -print \) \
>                    -o \( -name '*.sh' -type f -print \) \
>                    | sed -e 's|^\./||' \
>            )
>     -FOUND_SOURCE_FILES := $(shell $(SOURCES_CMD))
>     +FOUND_SOURCE_FILES := $(filter-out $(GENERATED_H),$(shell $(SOURCES_CMD)))
>
>     FOUND_C_SOURCES = $(filter %.c,$(FOUND_SOURCE_FILES))
>     FOUND_H_SOURCES = $(filter %.h,$(FOUND_SOURCE_FILES))
>
>> may be sufficient.  Deduplicating near the consumer, like
>> LOCALIZED_C, may force us to dedup all the consumers of it (e.g.
>> LOCALIZED_C is not the sole consumer of FOUND_C_SOURCES; you'd need
>> to sort the input to COCCI_SOURCES, for example).
>
> If we apply the above patch, sorting LOCALIZED_C is not necessary.

Per earlier feedback in
https://lore.kernel.org/git/220519.86tu9l6fw4.gmgdl@evledraar.gmail.com/
this all seesm a bit too complex, especially now.

I pointed out then that with --sort-by-file added we:

 * Don't group the translations by C/SH/Perl anymore
 * Change the sort order within files, to be line/sorted instead of
   line/order (i.e. first occurring translations first)

I suggested then to just use $(sort) on the respective lists.

So why not just:

 1. Switch to the $(FOUND_C_SOURCES) (good)
 2. Filter that by C/Perl/SH as before (just a simple $(filter)
 3. $(sort) that (which as noted, also de-dupes it)

Then we don't have any of the behavior change of --sort-by-file, and we
don't have to carefully curate the ls-files/find commands to not include
duplicates (although as seen here that seems to have been a useful
canary in the "find" case).