Re: [PATCH v4 2/3] advice: revamp advise API

[Emily Shaffer <emilyshaffer@google.com>]

On Mon, Feb 24, 2020 at 03:13:17PM +0000, Heba Waly via GitGitGadget wrote:
> From: Heba Waly <heba.waly@gmail.com>
> 
> Currently it's very easy for the advice library's callers to miss
> checking the visibility step before printing an advice. Also, it makes
> more sense for this step to be handled by the advice library.
> 
> Add a new advise_if_enabled function that checks the visibility of
> advice messages before printing.
> 
> Add a new helper advise_enabled to check the visibility of the advice
> if the caller needs to carry out complicated processing based on that
> value.
> 
> A list of config variables 'advice_config_keys' is added to be used by
> list_config_advices() instead of 'advice_config[]' because we'll get
> rid of 'advice_config[]' and the global variables once we migrate all
> the callers to use the new APIs.
> 
> Also change the advise call in tag library from advise() to
> advise_if_enabled() to construct an example of the usage of the new
> API.
> 
> Signed-off-by: Heba Waly <heba.waly@gmail.com>

I read Junio's review and agree with that too; but here are some more
thoughts.

> +static int get_config_value(enum advice_type type)
> +{
> +	int value = 1;

So we default to true if the config is unset...

> +	char *key = xstrfmt("%s.%s", "advice", advice_config_keys[type]);
> +	git_config_get_bool(key, &value);
...and per config.h, "when the configuration variable `key` is now
found, returns 1 without touching `dest`. Nice, so the default-true
works. If some problem is found when converting the value to a bool,
this function die()s, so you don't have to check the return value.

> +	free(key);
> +	return value;
> +}
> +
> +int advice_enabled(enum advice_type type)
> +{
> +	switch(type) {
> +	case PUSH_UPDATE_REJECTED:
> +		return get_config_value(PUSH_UPDATE_REJECTED) &&
> +		       get_config_value(PUSH_UPDATE_REJECTED_ALIAS);
So I can ask advice_enabled(PUSH_UPDATE_REJECTED) and still be told
'false' if earlier I set "advice.pushNonfastForward" to 0.

I wondered if this was really identical behavior to how this thing
worked before.

Before, it looks like we use the older config callback method
(git_default_advice_config()): we read each config option the user has,
in order, and then we check it against each member of advice_config, and
if it's a match, we set the appropriate value. That means that
advice_push_update_rejected is determined by whichever config is set
last, e.g. a config like so:

global: advice.pushUpdateRejected = 1
local:  advice.pushNonFastForward = 0

results in advice_push_update_rejected == 0.

Now, though, you consider the values of both. In the example above, you
have the same value; but if you reverse the values:

global: advice.pushUpdateRejected = 0
local:  advice.pushNonFastForward = 1

then your new code says advice_enabled(PUSH_UPDATE_REJECTED) == 0, and
the old codepath says advice_push_update_rejected = 1.

Although, using the config lookup methods as you are, I don't think you
can make it stay the same - I don't think there's a way to compare the
relative position of two different configs, is there?

Is this such a big deal? My gut says no; my gut says if someone had
advice.pushNonFastForward set, they aren't touching their config to
unset it, which means they aren't touching their config to fiddle with
advice.pushUpdateRejected either.

Maybe someone who was around when this alias was added can provide some
context?

 - Emily