Ramsay Jones <ramsay@xxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx> > --- > 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)
