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

[Artem Bityutskiy]

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!

-- 
Best Regards,
Artem Bityutskiy