[PATCH 1/1] Makefile: improve SPARSE_FLAGS customisation

[PATCH 1/1] Makefile: improve SPARSE_FLAGS customisation

[Ramsay Jones]

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.

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)
-- 
2.20.0

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

[Junio C Hamano]

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)

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

[Luc Van Oostenryck]

On Fri, Feb 01, 2019 at 01:46:13PM -0800, Junio C Hamano wrote:
> 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 ...".

I think the intent here is to *only* use SP_SPARSE_FLAGS as
the internal-only variable and to use SPARSE_FLAGS *only* 
as the additional user-controlable flags.
If it is indeed the case, then I think it looks good but maybe
it would be better to use another variable's name to make
this more explicit or add a small comment.

-- Luc

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

[Ramsay Jones]

On 01/02/2019 21:46, Junio C Hamano wrote:
> 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.

As Luc surmised, in his reply, my intention was that SP_EXTRA_FLAGS
should be used for any 'internal' settings (not just the target
specific settings), whereas SPARSE_FLAGS would now be used _only_ for
user customisation.

The commit message doesn't make that clear, (and the patch text adds
to the confusion, since only target specific settings are changed) so
I need to reword that somehow. Also, ...

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

... (Ahem) I just simply forgot to initialise the new variable! :(
(Yes, it actually doesn't matter, but it gives a wrong impression). ;-)

BTW, the first name I chose was SP_FLAGS, but while editing the second
hunk I decided that wasn't a good name. On several other projects I have
seen exactly this 'split' happen, where the user facing variable was
called <something>_FLAGS and the 'internal' variable was then called
<something>_EXTRA_FLAGS, so I decided to go with that instead. (Yes, I
abbreviated SPARSE). However, I have to say that I have also seen (less
often) the exact opposite: "... if some idiot user wants to add extra
flags ...". :-D

So, yes SP_EXTRA_FLAGS could be used for other 'internal' uses; for
example, look back to commit 6bc8606be3 ("config.mak.uname: remove
SPARSE_FLAGS setting for cygwin", 2018-02-12), which removed:
'SPARSE_FLAGS = -isystem /usr/include/w32api -Wno-one-bit-signed-bitfield'
from config.mak.uname. As you can see, although gcc could find the
win32 header files, sparse needed a little help. Also, the win32 system
header files had an instance of a 'one-bit signed bitfield', which caused
sparse to spew many many many errors. If I needed to do something like
that again, then I would use SP_EXTRA_FLAGS instead.

[Looking back now, I am a little shocked that it seems to have taken
me nearly 5 years to submit that patch! :-P ]

I could give quite a few examples, but ... Oh wait! ... Hmm, it seems
that I need to add a new patch to remove line 558 of config.mak.uname.
This line has a setting for SPARSE_FLAGS in the MINGW section of that
file. Back in around 2011, having ported sparse to MinGW (the original
msysgit, not MSYS2), I naturally had the same issue with the Win32
header files. Since I didn't upstream my sparse patches, I don't think
anyone can be running sparse on MinGW these days.

Anyway, its late, so I will look at redoing the patches soon.

ATB,
Ramsay Jones

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

[Junio C Hamano]

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

>> 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.
>
> As Luc surmised, in his reply, my intention was that SP_EXTRA_FLAGS
> should be used for any 'internal' settings (not just the target
> specific settings), whereas SPARSE_FLAGS would now be used _only_ for
> user customisation.

OK, if that is the case, then not using "+= append" on SP_EXTRA_FLAGS
and initializing it without allowing environment would be two improvements
that can make the intention more clear, I think.

> Anyway, its late, so I will look at redoing the patches soon.

Thanks.

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

[Ramsay Jones]

On 04/02/2019 18:12, Junio C Hamano wrote:
> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
> 
>>> 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.
>>
>> As Luc surmised, in his reply, my intention was that SP_EXTRA_FLAGS
>> should be used for any 'internal' settings (not just the target
>> specific settings), whereas SPARSE_FLAGS would now be used _only_ for
>> user customisation.
> 
> OK, if that is the case, then not using "+= append" on SP_EXTRA_FLAGS

Err, no, that clearly wouldn't be an improvement! As I said above,
this is not just for target specific settings.

Am I missing something?

ATB,
Ramsay Jones

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

[Junio C Hamano]

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

> On 04/02/2019 18:12, Junio C Hamano wrote:
>> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
>> 
>>>> 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.
>>>
>>> As Luc surmised, in his reply, my intention was that SP_EXTRA_FLAGS
>>> should be used for any 'internal' settings (not just the target
>>> specific settings), whereas SPARSE_FLAGS would now be used _only_ for
>>> user customisation.
>> 
>> OK, if that is the case, then not using "+= append" on SP_EXTRA_FLAGS
>
> Err, no, that clearly wouldn't be an improvement! As I said above,
> this is not just for target specific settings.

Ah, do you mean that there may be globally applicable internal
setting?  I would have expected that such an option would be done
directly on the command line, e.g.

$(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE
	$(QUIET_SP)cgcc -no-compile $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) \
		$(SPARSE_FLAGS) $(SP_EXTRA_FLAGS) \
		-Wsparse-settings-for-everybody $<

But it is fine either way, as long as the purpose of the macro is
documented clearly enough ;-)

Thanks.

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

[Ramsay Jones]

On 04/02/2019 20:15, Junio C Hamano wrote:
> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
> 
>> On 04/02/2019 18:12, Junio C Hamano wrote:
>>> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
>>>
>>>>> 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.
>>>>
>>>> As Luc surmised, in his reply, my intention was that SP_EXTRA_FLAGS
>>>> should be used for any 'internal' settings (not just the target
>>>> specific settings), whereas SPARSE_FLAGS would now be used _only_ for
>>>> user customisation.
>>>
>>> OK, if that is the case, then not using "+= append" on SP_EXTRA_FLAGS
>>
>> Err, no, that clearly wouldn't be an improvement! As I said above,
>> this is not just for target specific settings.
> 
> Ah, do you mean that there may be globally applicable internal
> setting?  I would have expected that such an option would be done
> directly on the command line, e.g.
> 
> $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE
> 	$(QUIET_SP)cgcc -no-compile $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) \
> 		$(SPARSE_FLAGS) $(SP_EXTRA_FLAGS) \
> 		-Wsparse-settings-for-everybody $<

global, possibly, but more likely platform variations - as I tried
(but obviously failed) to indicate with the cygwin and MinGW examples
in my previous email.

ATB,
Ramsay Jones