[PATCH 1/2] aiaiai: move most of the aiaiai-email-test-patchset options to the file

[Keller, Jacob E]

On Thu, 2014-03-06 at 14:21 +0200, Artem Bityutskiy wrote:
> On Thu, 2014-03-06 at 13:49 +0200, Artem Bityutskiy wrote:
> > From: Jacob Keller <jacob.e.keller at intel.com>
> > 
> > Currently, the options supplied by the aiaiai-email-test-patchset were
> > supplied on the command line, with no way to override them per project,
> > or even easily specify or change them. (You had to muck with the
> > validator command.) This patch adds two helper functions for reading
> > multiple configuration variables, and modifies the script to work with
> > the options from the configuration file instead of the command line.
> > 
> > This also enables the first options which have global settings
> > overridden by project settings. This enables a simple default set for
> > all projects that can be tweaked per-project if desired (which was
> > impossible before).
> > 
> > This also adds an always_cc which merges with the per-project always_cc,
> > and cleans up the use of merge_addresses, now that we can support an
> > arbitrary number of parameters.
> > 
> > Signed-off-by: Jacob Keller <jacob.e.keller at intel.com>
> 
> Overall, the change is good and very welcome, thanks. However, I have
> some comments about the implementation.
> 
> Let me summarize. We have 2 types of settings - global, and
> per-projects. Then the global settings are classified like this:
> 
> 1. "[Global]" -  these are actually mandatory global settings.
> 
> 2. "[Email]" - also mandatory global settings, but related to e-mail.
> 
> 3. "[Validator]" - optional global settings to validation, can be
> overridden by per-project settings.
> 
> To make the long story short, I feel like this is poor classification.
> And I'd better change it this way:
> 
> 1. "[Global]" - Global Aiaiai options, not related to any project.
> 2. "[Defaults]" - default values for all the per-project settings. These
> are used when the project does not define own setting.
> 
> This would be cleaner IMO. And some of the "[Global]" options could
> become per-project too.
> 
> And the way I'd approach this.
> 
> 1. A patch which merges "[Email]" with "[Global]". Should be easy. Then
> we only have "[Global]" and per-project section types.
> 
> 2. A patch which moves command-line options to per-project sections.
> This patch would not add "[Defaults]" at all.
> 
> 3. A patch which adds a global "[Defaults]" section.
> 
> > diff --git a/email/aiaiai-email-sh-functions b/email/aiaiai-email-sh-functions
> > index bb77b4d..3e3115c 100644
> > --- a/email/aiaiai-email-sh-functions
> > +++ b/email/aiaiai-email-sh-functions
> > @@ -139,7 +139,9 @@ ini_config_get_or_die()
> 
> > +	# Validator Options
> > +	cfg_defconfigdir="$(ini_config_get "$cfgfile" "validator" "defconfigdir")"
> > +	cfg_targets="$(ini_config_get "$cfgfile" "validator" "targets")"
> > +	cfg_bisectability="$(ini_config_get "$cfgfile" "validator" "bisectability")"
> > +	cfg_sparse="$(ini_config_get "$cfgfile" "validator" "sparse")"
> > +	cfg_smatch="$(ini_config_get "$cfgfile" "validator" "smatch")"
> > +	cfg_cppcheck="$(ini_config_get "$cfgfile" "validator" "cppcheck")"
> > +	cfg_coccinelle="$(ini_config_get "$cfgfile" "validator" "coccinelle")"
> > +	cfg_unwanted_keywords="$(ini_config_get "$cfgfile" "validator" "unwanted_keywords")"
> > +	cfg_kmake_opts="$(ini_config_get "$cfgfile" "validator" "kmake_opts")"
> 
> None of these variables is used outside of "aiaiai-email-sh-functions".
> And they are really not needed there at all. I'd prefix all of them with
> "__", which means that they'd be only for "aiaiai-email-sh-functions"
> own usage.
> 
> And see below.
> 
> > +	# Per-Project Validator Options
> > +	# These values take precedence over the cfg_* options
> > +	pcfg_defconfigdir="$(ini_config_get "$cfgfile" "validator" "defconfigdir")"
> > +	pcfg_bisectability="$(ini_config_get "$cfgfile" "prj_$prj" "bisectability")"
> > +	pcfg_targets="$(ini_config_get "$cfgfile" "prj_$prj" "targets")"
> > +	pcfg_sparse="$(ini_config_get "$cfgfile" "prj_$prj" "sparse")"
> > +	pcfg_smatch="$(ini_config_get "$cfgfile" "prj_$prj" "smatch")"
> > +	pcfg_cppcheck="$(ini_config_get "$cfgfile" "prj_$prj" "cppcheck")"
> > +	pcfg_coccinelle="$(ini_config_get "$cfgfile" "prj_$prj" "coccinelle")"
> 
> These are the ones interesting outside of this file. Not the defaults.
> And this is the place where I'd do something like this.
> 
> pcfg_defconfigdir="$(ini_config_get "$cfgfile" "validator" "defconfigdir")"
> pcfg_defconfigdir="${pcfg_defconfigdir:-$__cfg_defconfigdir}"
> 
> and so for every other variable.
> > +cfg_get_value()
> > +{
> > +	local var="$1"; shift
> > +	local default="$2"; shift
> > +
> > +	if [ -z $(eval "echo \$pcfg_$var") ]; then
> > +		eval "echo \$pcfg_$var"
> > +	elif [ -z $(eval "echo \$cfg_$var") ]; then
> > +		eval "echo \$cfg_$var"
> > +	else
> > +		echo "$default"
> > +	fi
> > +}
> 
> And then this function becomes unneeded. The pcfg_ parameters already
> contain the own value or the default value, or "" if they were not
> defined in the config file at all.
> 
> You also do a trich with "$default" in this function which you use, I
> think once later, I did not understand the trick, but did not try hard.
> But this means that it is not easy to understand :-) Probably the trick
> is not really needed and this function may go away?
> 
> How does this sound?
> 
> Thanks!
> 

Much better! I can work on this. I hope to resolve why my local mailer
was not sending out the patches.

I wasn't really sure how to do this, but I like this method much better,
as it removes the necessity for the cfg_get_value function, which I did
not like!

Quick question, how do you think these changes relate to adding options
for other programs, and will it work for that..?

We have some global options which I would like to add that I wasn't sure
how to implement (number of validators, for example).

Also, what about this may have to change once we use the cfg file for
all programs and not just the email settings?

Thanks,
Jake