Re: [PATCH 2/5] Documentation/Makefile: make $(wildcard <doc deps>) a var

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 28, 2021, 06:28 UTC
Message-ID: <xmqq7dlrokqx.fsf@gitster.g>
In-Reply-To: <patch-2.6-fdc55a86cf4-20210326T103454Z-avarab@gmail.com>

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

Show 39 quoted lines

> Refactor the wildcard we'll scan for "include" directives into a
> single INCLUDE_TARGETS_TXT variable for readability, consistency.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  Documentation/Makefile | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/Makefile b/Documentation/Makefile
> index 0ba7564be93..7313956d73f 100644
> --- a/Documentation/Makefile
> +++ b/Documentation/Makefile
> @@ -3,6 +3,7 @@ MAN1_TXT =
>  MAN5_TXT =
>  MAN7_TXT =
>  HOWTO_TXT =
> +INCLUDE_TARGETS_TXT =
>  TECH_DOCS =
>  ARTICLES =
>  SP_ARTICLES =
> @@ -45,6 +46,9 @@ MAN7_TXT += gitworkflows.txt
>  
>  HOWTO_TXT += $(wildcard howto/*.txt)
>  
> +INCLUDE_TARGETS_TXT += $(wildcard *.txt)
> +INCLUDE_TARGETS_TXT += $(wildcard config/*.txt)
> +
>  ifdef MAN_FILTER
>  MAN_TXT = $(filter $(MAN_FILTER),$(MAN1_TXT) $(MAN5_TXT) $(MAN7_TXT))
>  else
> @@ -287,7 +291,7 @@ docdep_prereqs = \
>  	mergetools-list.made $(mergetools_txt) \
>  	cmd-list.made $(cmds_txt)
>  
> -doc.dep : $(docdep_prereqs) $(wildcard *.txt) $(wildcard config/*.txt) build-docdep.perl
> +doc.dep : $(docdep_prereqs) $(INCLUDE_TARGETS_TXT) build-docdep.perl
>  	$(QUIET_GEN)$(RM) $@+ $@ && \
>  	$(PERL_PATH) ./build-docdep.perl >$@+ $(QUIET_STDERR) && \
>  	mv $@+ $@

While this may not break anything per-se, I find the name of the new macro not quite intuitive.

The build-docdep script opens $(wildcard *.txt) itself (without getting them fed from Makefile) to find what other files are used via "include::", and then it recursively scans more files that are used via "include::" to build dependencies. Makefile assumes the "include::" chain would pull in $(wildcard config/*.txt) files, but this assumption may not prevent other *.txt files from getting pulled in (not a new problem introduced by this patch).

I am not sure what these *.txt files involved in the process should be called, but "include targets" sounds something else. Naïvely, I would say "we rebuild documentation dependency any time any source text file changes", so if this were called "ALL_TXT" or something like that, I wouldn't have such a reaction.

Thanks.