Web lists-archives.com

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




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

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