{"thread":{"id":"50381","subject":"[PATCH 1/1] Makefile: improve SPARSE_FLAGS customisation","startedAt":"2019-02-01T21:03:15Z","lastAt":"2019-02-04T20:50:01Z","messageCount":8,"participants":["Ramsay Jones","Junio C Hamano","Luc Van Oostenryck"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"368372","messageId":"3ccf0255-8a15-effc-ce6b-eabb61625f90@ramsayjones.plus.com","threadId":"50381","inReplyTo":null,"subject":"[PATCH 1/1] Makefile: improve SPARSE_FLAGS customisation","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2019-02-01T21:03:11Z","receivedAt":"2019-02-01T21:03:15Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\nIn order to enable greater user customisation of the SPARSE_FLAGS\nvariable, we introduce a new SP_EXTRA_FLAGS variable to use for\ntarget specific settings. Without using the new variable, setting\nthe SPARSE_FLAGS on the 'make' command-line would also override the\nvalue set by the target-specific rules in the Makefile (effectively\nmaking them useless). In addition, we initialise the SPARSE_FLAGS\nto the default (empty) value using a conditional assignment (?=).\nThis allows the SPARSE_FLAGS to be set from the environment as\nwell as from the command-line.\n\nSigned-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>\n---\n Makefile | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 6e8d017e8e..dc02825c88 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -574,7 +574,7 @@ SPATCH = spatch\n \n export TCL_PATH TCLTK_PATH\n \n-SPARSE_FLAGS =\n+SPARSE_FLAGS ?=\n SPATCH_FLAGS = --all-includes --patch .\n \n \n@@ -2369,10 +2369,10 @@ gettext.sp gettext.s gettext.o: GIT-PREFIX\n gettext.sp gettext.s gettext.o: EXTRA_CPPFLAGS = \\\n \t-DGIT_LOCALE_PATH='\"$(localedir_relative_SQ)\"'\n \n-http-push.sp http.sp http-walker.sp remote-curl.sp imap-send.sp: SPARSE_FLAGS += \\\n+http-push.sp http.sp http-walker.sp remote-curl.sp imap-send.sp: SP_EXTRA_FLAGS += \\\n \t-DCURL_DISABLE_TYPECHECK\n \n-pack-revindex.sp: SPARSE_FLAGS += -Wno-memcpy-max-count\n+pack-revindex.sp: SP_EXTRA_FLAGS += -Wno-memcpy-max-count\n \n ifdef NO_EXPAT\n http-walker.sp http-walker.s http-walker.o: EXTRA_CPPFLAGS = -DNO_EXPAT\n@@ -2386,7 +2386,7 @@ endif\n ifdef USE_NED_ALLOCATOR\n compat/nedmalloc/nedmalloc.sp compat/nedmalloc/nedmalloc.o: EXTRA_CPPFLAGS = \\\n \t-DNDEBUG -DREPLACE_SYSTEM_ALLOCATOR\n-compat/nedmalloc/nedmalloc.sp: SPARSE_FLAGS += -Wno-non-pointer-null\n+compat/nedmalloc/nedmalloc.sp: SP_EXTRA_FLAGS += -Wno-non-pointer-null\n endif\n \n git-%$X: %.o GIT-LDFLAGS $(GITLIBS)\n@@ -2710,7 +2710,7 @@ SP_OBJ = $(patsubst %.o,%.sp,$(C_OBJ))\n \n $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE\n \t$(QUIET_SP)cgcc -no-compile $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) \\\n-\t\t$(SPARSE_FLAGS) $<\n+\t\t$(SPARSE_FLAGS) $(SP_EXTRA_FLAGS) $<\n \n .PHONY: sparse $(SP_OBJ)\n sparse: $(SP_OBJ)\n-- \n2.20.0\n"},{"id":"368375","messageId":"xmqqmunfxjve.fsf@gitster-ct.c.googlers.com","threadId":"50381","inReplyTo":"3ccf0255-8a15-effc-ce6b-eabb61625f90@ramsayjones.plus.com","subject":"Re: [PATCH 1/1] Makefile: improve SPARSE_FLAGS customisation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-02-01T21:46:13Z","receivedAt":"2019-02-01T21:46:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones <ramsay@ramsayjones.plus.com> writes:\n\n> In order to enable greater user customisation of the SPARSE_FLAGS\n> variable, we introduce a new SP_EXTRA_FLAGS variable to use for\n> target specific settings. Without using the new variable, setting\n> the SPARSE_FLAGS on the 'make' command-line would also override the\n> value set by the target-specific rules in the Makefile (effectively\n> making them useless). In addition, we initialise the SPARSE_FLAGS\n> to the default (empty) value using a conditional assignment (?=).\n> This allows the SPARSE_FLAGS to be set from the environment as\n> well as from the command-line.\n\nThanks for a detailed and clear explanation here and in the cover\nletter.  I agree with the motivation and most of the things I see in\nthis patch, but one thing that stands out at me is if we still want\nto += append to SP_EXTRA_FLAGS in target specific way.  Before this\npatch, because SPARSE_FLAGS was a dual use variable, it needed +=\nappending to it in these two places, but that rationale is gone with\nthis patch.\n\nAlso, don't we want to clear SP_EXTRA_FLAGS at the beginning?\n\nThe reason I raise these is because I do not quite see a clear\nanswer to \"I want to set SP_EXTRA_FLAGS and not SPARSE_FLAGS,\nbecause ...\".\n\n> Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>\n> ---\n>  Makefile | 10 +++++-----\n>  1 file changed, 5 insertions(+), 5 deletions(-)\n>\n> diff --git a/Makefile b/Makefile\n> index 6e8d017e8e..dc02825c88 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -574,7 +574,7 @@ SPATCH = spatch\n>  \n>  export TCL_PATH TCLTK_PATH\n>  \n> -SPARSE_FLAGS =\n> +SPARSE_FLAGS ?=\n>  SPATCH_FLAGS = --all-includes --patch .\n>  \n>  \n> @@ -2369,10 +2369,10 @@ gettext.sp gettext.s gettext.o: GIT-PREFIX\n>  gettext.sp gettext.s gettext.o: EXTRA_CPPFLAGS = \\\n>  \t-DGIT_LOCALE_PATH='\"$(localedir_relative_SQ)\"'\n>  \n> -http-push.sp http.sp http-walker.sp remote-curl.sp imap-send.sp: SPARSE_FLAGS += \\\n> +http-push.sp http.sp http-walker.sp remote-curl.sp imap-send.sp: SP_EXTRA_FLAGS += \\\n>  \t-DCURL_DISABLE_TYPECHECK\n>  \n> -pack-revindex.sp: SPARSE_FLAGS += -Wno-memcpy-max-count\n> +pack-revindex.sp: SP_EXTRA_FLAGS += -Wno-memcpy-max-count\n>  \n>  ifdef NO_EXPAT\n>  http-walker.sp http-walker.s http-walker.o: EXTRA_CPPFLAGS = -DNO_EXPAT\n> @@ -2386,7 +2386,7 @@ endif\n>  ifdef USE_NED_ALLOCATOR\n>  compat/nedmalloc/nedmalloc.sp compat/nedmalloc/nedmalloc.o: EXTRA_CPPFLAGS = \\\n>  \t-DNDEBUG -DREPLACE_SYSTEM_ALLOCATOR\n> -compat/nedmalloc/nedmalloc.sp: SPARSE_FLAGS += -Wno-non-pointer-null\n> +compat/nedmalloc/nedmalloc.sp: SP_EXTRA_FLAGS += -Wno-non-pointer-null\n>  endif\n>  \n>  git-%$X: %.o GIT-LDFLAGS $(GITLIBS)\n> @@ -2710,7 +2710,7 @@ SP_OBJ = $(patsubst %.o,%.sp,$(C_OBJ))\n>  \n>  $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE\n>  \t$(QUIET_SP)cgcc -no-compile $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) \\\n> -\t\t$(SPARSE_FLAGS) $<\n> +\t\t$(SPARSE_FLAGS) $(SP_EXTRA_FLAGS) $<\n>  \n>  .PHONY: sparse $(SP_OBJ)\n>  sparse: $(SP_OBJ)\n"},{"id":"368386","messageId":"20190201224813.4vykjb4iud5j6xhz@ltop.local","threadId":"50381","inReplyTo":"xmqqmunfxjve.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/1] Makefile: improve SPARSE_FLAGS customisation","fromName":"Luc Van Oostenryck","fromEmail":"luc.vanoostenryck@gmail.com","sentAt":"2019-02-01T22:48:14Z","receivedAt":"2019-02-01T22:48:18Z","isPatch":true,"sender":{"key":"luc.vanoostenryck@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16168334?v=4"},"body":"On Fri, Feb 01, 2019 at 01:46:13PM -0800, Junio C Hamano wrote:\n> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:\n> \n> > In order to enable greater user customisation of the SPARSE_FLAGS\n> > variable, we introduce a new SP_EXTRA_FLAGS variable to use for\n> > target specific settings. Without using the new variable, setting\n> > the SPARSE_FLAGS on the 'make' command-line would also override the\n> > value set by the target-specific rules in the Makefile (effectively\n> > making them useless). In addition, we initialise the SPARSE_FLAGS\n> > to the default (empty) value using a conditional assignment (?=).\n> > This allows the SPARSE_FLAGS to be set from the environment as\n> > well as from the command-line.\n> \n> Thanks for a detailed and clear explanation here and in the cover\n> letter.  I agree with the motivation and most of the things I see in\n> this patch, but one thing that stands out at me is if we still want\n> to += append to SP_EXTRA_FLAGS in target specific way.  Before this\n> patch, because SPARSE_FLAGS was a dual use variable, it needed +=\n> appending to it in these two places, but that rationale is gone with\n> this patch.\n> \n> Also, don't we want to clear SP_EXTRA_FLAGS at the beginning?\n> \n> The reason I raise these is because I do not quite see a clear\n> answer to \"I want to set SP_EXTRA_FLAGS and not SPARSE_FLAGS,\n> because ...\".\n\nI think the intent here is to *only* use SP_SPARSE_FLAGS as\nthe internal-only variable and to use SPARSE_FLAGS *only* \nas the additional user-controlable flags.\nIf it is indeed the case, then I think it looks good but maybe\nit would be better to use another variable's name to make\nthis more explicit or add a small comment.\n\n-- Luc\n"},{"id":"368421","messageId":"4a8f6b3f-3844-b4e8-2ed2-4078b592dd4b@ramsayjones.plus.com","threadId":"50381","inReplyTo":"xmqqmunfxjve.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/1] Makefile: improve SPARSE_FLAGS customisation","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2019-02-03T01:25:22Z","receivedAt":"2019-02-03T01:25:30Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 01/02/2019 21:46, Junio C Hamano wrote:\n> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:\n> \n>> In order to enable greater user customisation of the SPARSE_FLAGS\n>> variable, we introduce a new SP_EXTRA_FLAGS variable to use for\n>> target specific settings. Without using the new variable, setting\n>> the SPARSE_FLAGS on the 'make' command-line would also override the\n>> value set by the target-specific rules in the Makefile (effectively\n>> making them useless). In addition, we initialise the SPARSE_FLAGS\n>> to the default (empty) value using a conditional assignment (?=).\n>> This allows the SPARSE_FLAGS to be set from the environment as\n>> well as from the command-line.\n> \n> Thanks for a detailed and clear explanation here and in the cover\n> letter.  I agree with the motivation and most of the things I see in\n> this patch, but one thing that stands out at me is if we still want\n> to += append to SP_EXTRA_FLAGS in target specific way.  Before this\n> patch, because SPARSE_FLAGS was a dual use variable, it needed +=\n> appending to it in these two places, but that rationale is gone with\n> this patch.\n\nAs Luc surmised, in his reply, my intention was that SP_EXTRA_FLAGS\nshould be used for any 'internal' settings (not just the target\nspecific settings), whereas SPARSE_FLAGS would now be used _only_ for\nuser customisation.\n\nThe commit message doesn't make that clear, (and the patch text adds\nto the confusion, since only target specific settings are changed) so\nI need to reword that somehow. Also, ...\n\n> Also, don't we want to clear SP_EXTRA_FLAGS at the beginning?\n\n... (Ahem) I just simply forgot to initialise the new variable! :(\n(Yes, it actually doesn't matter, but it gives a wrong impression). ;-)\n\nBTW, the first name I chose was SP_FLAGS, but while editing the second\nhunk I decided that wasn't a good name. On several other projects I have\nseen exactly this 'split' happen, where the user facing variable was\ncalled <something>_FLAGS and the 'internal' variable was then called\n<something>_EXTRA_FLAGS, so I decided to go with that instead. (Yes, I\nabbreviated SPARSE). However, I have to say that I have also seen (less\noften) the exact opposite: \"... if some idiot user wants to add extra\nflags ...\". :-D\n\nSo, yes SP_EXTRA_FLAGS could be used for other 'internal' uses; for\nexample, look back to commit 6bc8606be3 (\"config.mak.uname: remove\nSPARSE_FLAGS setting for cygwin\", 2018-02-12), which removed:\n'SPARSE_FLAGS = -isystem /usr/include/w32api -Wno-one-bit-signed-bitfield'\nfrom config.mak.uname. As you can see, although gcc could find the\nwin32 header files, sparse needed a little help. Also, the win32 system\nheader files had an instance of a 'one-bit signed bitfield', which caused\nsparse to spew many many many errors. If I needed to do something like\nthat again, then I would use SP_EXTRA_FLAGS instead.\n\n[Looking back now, I am a little shocked that it seems to have taken\nme nearly 5 years to submit that patch! :-P ]\n\nI could give quite a few examples, but ... Oh wait! ... Hmm, it seems\nthat I need to add a new patch to remove line 558 of config.mak.uname.\nThis line has a setting for SPARSE_FLAGS in the MINGW section of that\nfile. Back in around 2011, having ported sparse to MinGW (the original\nmsysgit, not MSYS2), I naturally had the same issue with the Win32\nheader files. Since I didn't upstream my sparse patches, I don't think\nanyone can be running sparse on MinGW these days.\n\nAnyway, its late, so I will look at redoing the patches soon.\n\nATB,\nRamsay Jones\n"},{"id":"368465","messageId":"xmqqwomfv2w5.fsf@gitster-ct.c.googlers.com","threadId":"50381","inReplyTo":"4a8f6b3f-3844-b4e8-2ed2-4078b592dd4b@ramsayjones.plus.com","subject":"Re: [PATCH 1/1] Makefile: improve SPARSE_FLAGS customisation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-02-04T18:12:42Z","receivedAt":"2019-02-04T18:12:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones <ramsay@ramsayjones.plus.com> writes:\n\n>> Thanks for a detailed and clear explanation here and in the cover\n>> letter.  I agree with the motivation and most of the things I see in\n>> this patch, but one thing that stands out at me is if we still want\n>> to += append to SP_EXTRA_FLAGS in target specific way.  Before this\n>> patch, because SPARSE_FLAGS was a dual use variable, it needed +=\n>> appending to it in these two places, but that rationale is gone with\n>> this patch.\n>\n> As Luc surmised, in his reply, my intention was that SP_EXTRA_FLAGS\n> should be used for any 'internal' settings (not just the target\n> specific settings), whereas SPARSE_FLAGS would now be used _only_ for\n> user customisation.\n\nOK, if that is the case, then not using \"+= append\" on SP_EXTRA_FLAGS\nand initializing it without allowing environment would be two improvements\nthat can make the intention more clear, I think.\n\n> Anyway, its late, so I will look at redoing the patches soon.\n\nThanks.\n"},{"id":"368477","messageId":"365a9539-4ce1-121a-ec8c-2e52a5828091@ramsayjones.plus.com","threadId":"50381","inReplyTo":"xmqqwomfv2w5.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/1] Makefile: improve SPARSE_FLAGS customisation","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2019-02-04T19:20:12Z","receivedAt":"2019-02-04T19:20:18Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 04/02/2019 18:12, Junio C Hamano wrote:\n> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:\n> \n>>> Thanks for a detailed and clear explanation here and in the cover\n>>> letter.  I agree with the motivation and most of the things I see in\n>>> this patch, but one thing that stands out at me is if we still want\n>>> to += append to SP_EXTRA_FLAGS in target specific way.  Before this\n>>> patch, because SPARSE_FLAGS was a dual use variable, it needed +=\n>>> appending to it in these two places, but that rationale is gone with\n>>> this patch.\n>>\n>> As Luc surmised, in his reply, my intention was that SP_EXTRA_FLAGS\n>> should be used for any 'internal' settings (not just the target\n>> specific settings), whereas SPARSE_FLAGS would now be used _only_ for\n>> user customisation.\n> \n> OK, if that is the case, then not using \"+= append\" on SP_EXTRA_FLAGS\n\nErr, no, that clearly wouldn't be an improvement! As I said above,\nthis is not just for target specific settings.\n\nAm I missing something?\n\nATB,\nRamsay Jones\n\n"},{"id":"368484","messageId":"xmqqy36vtimu.fsf@gitster-ct.c.googlers.com","threadId":"50381","inReplyTo":"365a9539-4ce1-121a-ec8c-2e52a5828091@ramsayjones.plus.com","subject":"Re: [PATCH 1/1] Makefile: improve SPARSE_FLAGS customisation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-02-04T20:15:37Z","receivedAt":"2019-02-04T20:15:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones <ramsay@ramsayjones.plus.com> writes:\n\n> On 04/02/2019 18:12, Junio C Hamano wrote:\n>> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:\n>> \n>>>> Thanks for a detailed and clear explanation here and in the cover\n>>>> letter.  I agree with the motivation and most of the things I see in\n>>>> this patch, but one thing that stands out at me is if we still want\n>>>> to += append to SP_EXTRA_FLAGS in target specific way.  Before this\n>>>> patch, because SPARSE_FLAGS was a dual use variable, it needed +=\n>>>> appending to it in these two places, but that rationale is gone with\n>>>> this patch.\n>>>\n>>> As Luc surmised, in his reply, my intention was that SP_EXTRA_FLAGS\n>>> should be used for any 'internal' settings (not just the target\n>>> specific settings), whereas SPARSE_FLAGS would now be used _only_ for\n>>> user customisation.\n>> \n>> OK, if that is the case, then not using \"+= append\" on SP_EXTRA_FLAGS\n>\n> Err, no, that clearly wouldn't be an improvement! As I said above,\n> this is not just for target specific settings.\n\nAh, do you mean that there may be globally applicable internal\nsetting?  I would have expected that such an option would be done\ndirectly on the command line, e.g.\n\n$(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE\n\t$(QUIET_SP)cgcc -no-compile $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) \\\n\t\t$(SPARSE_FLAGS) $(SP_EXTRA_FLAGS) \\\n\t\t-Wsparse-settings-for-everybody $<\n\nBut it is fine either way, as long as the purpose of the macro is\ndocumented clearly enough ;-)\n\nThanks.\n"},{"id":"368485","messageId":"a60cd464-33e8-eefb-33df-83c1dca09031@ramsayjones.plus.com","threadId":"50381","inReplyTo":"xmqqy36vtimu.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/1] Makefile: improve SPARSE_FLAGS customisation","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2019-02-04T20:49:56Z","receivedAt":"2019-02-04T20:50:01Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 04/02/2019 20:15, Junio C Hamano wrote:\n> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:\n> \n>> On 04/02/2019 18:12, Junio C Hamano wrote:\n>>> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:\n>>>\n>>>>> Thanks for a detailed and clear explanation here and in the cover\n>>>>> letter.  I agree with the motivation and most of the things I see in\n>>>>> this patch, but one thing that stands out at me is if we still want\n>>>>> to += append to SP_EXTRA_FLAGS in target specific way.  Before this\n>>>>> patch, because SPARSE_FLAGS was a dual use variable, it needed +=\n>>>>> appending to it in these two places, but that rationale is gone with\n>>>>> this patch.\n>>>>\n>>>> As Luc surmised, in his reply, my intention was that SP_EXTRA_FLAGS\n>>>> should be used for any 'internal' settings (not just the target\n>>>> specific settings), whereas SPARSE_FLAGS would now be used _only_ for\n>>>> user customisation.\n>>>\n>>> OK, if that is the case, then not using \"+= append\" on SP_EXTRA_FLAGS\n>>\n>> Err, no, that clearly wouldn't be an improvement! As I said above,\n>> this is not just for target specific settings.\n> \n> Ah, do you mean that there may be globally applicable internal\n> setting?  I would have expected that such an option would be done\n> directly on the command line, e.g.\n> \n> $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE\n> \t$(QUIET_SP)cgcc -no-compile $(ALL_CFLAGS) $(EXTRA_CPPFLAGS) \\\n> \t\t$(SPARSE_FLAGS) $(SP_EXTRA_FLAGS) \\\n> \t\t-Wsparse-settings-for-everybody $<\n\nglobal, possibly, but more likely platform variations - as I tried\n(but obviously failed) to indicate with the cygwin and MinGW examples\nin my previous email.\n\nATB,\nRamsay Jones\n\n"}]}