Re: [PATCH 1/1] Makefile: improve SPARSE_FLAGS customisation

[Junio C Hamano <gitster@pobox.com>]

Ramsay Jones <ramsay@ramsayjones.plus.com> writes:

> In order to enable greater user customisation of the SPARSE_FLAGS
> variable, we introduce a new SP_EXTRA_FLAGS variable to use for
> target specific settings. Without using the new variable, setting
> the SPARSE_FLAGS on the 'make' command-line would also override the
> value set by the target-specific rules in the Makefile (effectively
> making them useless). In addition, we initialise the SPARSE_FLAGS
> to the default (empty) value using a conditional assignment (?=).
> This allows the SPARSE_FLAGS to be set from the environment as
> well as from the command-line.

Thanks for a detailed and clear explanation here and in the cover
letter.  I agree with the motivation and most of the things I see in
this patch, but one thing that stands out at me is if we still want
to += append to SP_EXTRA_FLAGS in target specific way.  Before this
patch, because SPARSE_FLAGS was a dual use variable, it needed +=
appending to it in these two places, but that rationale is gone with
this patch.

Also, don't we want to clear SP_EXTRA_FLAGS at the beginning?

The reason I raise these is because I do not quite see a clear
answer to "I want to set SP_EXTRA_FLAGS and not SPARSE_FLAGS,
because ...".

> Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
> ---
>  Makefile | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 6e8d017e8e..dc02825c88 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -574,7 +574,7 @@ SPATCH = spatch
>  
>  export TCL_PATH TCLTK_PATH
>  
> -SPARSE_FLAGS =
> +SPARSE_FLAGS ?=
>  SPATCH_FLAGS = --all-includes --patch .
>  
>  
> @@ -2369,10 +2369,10 @@ gettext.sp gettext.s gettext.o: GIT-PREFIX
>  gettext.sp gettext.s gettext.o: EXTRA_CPPFLAGS = \
>  	-DGIT_LOCALE_PATH='"$(localedir_relative_SQ)"'
>  
> -http-push.sp http.sp http-walker.sp remote-curl.sp imap-send.sp: SPARSE_FLAGS += \
> +http-push.sp http.sp http-walker.sp remote-curl.sp imap-send.sp: SP_EXTRA_FLAGS += \
>  	-DCURL_DISABLE_TYPECHECK
>  
> -pack-revindex.sp: SPARSE_FLAGS += -Wno-memcpy-max-count
> +pack-revindex.sp: SP_EXTRA_FLAGS += -Wno-memcpy-max-count
>  
>  ifdef NO_EXPAT
>  http-walker.sp http-walker.s http-walker.o: EXTRA_CPPFLAGS = -DNO_EXPAT
> @@ -2386,7 +2386,7 @@ endif
>  ifdef USE_NED_ALLOCATOR
>  compat/nedmalloc/nedmalloc.sp compat/nedmalloc/nedmalloc.o: EXTRA_CPPFLAGS = \
>  	-DNDEBUG -DREPLACE_SYSTEM_ALLOCATOR
> -compat/nedmalloc/nedmalloc.sp: SPARSE_FLAGS += -Wno-non-pointer-null
> +compat/nedmalloc/nedmalloc.sp: SP_EXTRA_FLAGS += -Wno-non-pointer-null
>  endif
>  
>  git-%$X: %.o GIT-LDFLAGS $(GITLIBS)
> @@ -2710,7 +2710,7 @@ SP_OBJ = $(patsubst %.o,%.sp,$(C_OBJ))
>  
>  $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE
>  	$(QUIET_SP)cgcc -no-compile $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) \
> -		$(SPARSE_FLAGS) $<
> +		$(SPARSE_FLAGS) $(SP_EXTRA_FLAGS) $<
>  
>  .PHONY: sparse $(SP_OBJ)
>  sparse: $(SP_OBJ)