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
