Re: [PATCH v1 2/4] advice: add enum variants for missing advice variables

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Aug 2, 2021, 21:52 UTC
Message-ID: <nycvar.QRO.7.76.6.2108022333360.55@tvgsbejvaqbjf.bet>
In-Reply-To: <20210731022504.1912702-3-mathstuf@gmail.com>

Hi Ben,

On Fri, 30 Jul 2021, Ben Boeckel wrote:

> These were missed in their addition in 887a0fd573 (add: change advice
> config variables used by the add API, 2020-02-06). All other global
> variable settings have entries already.

It took quite a bit of reading and looking through the `git log` history to piece together what is going on here, and I wish the commit message would have explained this better.

A big puzzlement came from the claim that "These were missed" is not only missing a noun that clarifies what "These" are meant to be, but also from the fact that `git grep advice_setting 887a0fd573` comes up empty. Which suggests to me that nothing was missed there, but the problem lies with `hw/advise-ng`, merged via c4a09cc9ccb (Merge branch 'hw/advise-ng', 2020-03-25), is based on v2.25.0, but was only merged after v2.26.0, which contains daef1b300b0 (Merge branch 'hw/advice-add-nothing', 2020-02-14).

In other words, the addition of the two entries `addEmptyPathspec` and `addIgnoredFile` happened in a diverging branch from the addition of the `advice_setting` array, and the problem lies with the merge of the latter into a branch that already had merged the former.

It would have helped me to read something along these lines:

	In daef1b300b0 (Merge branch 'hw/advice-add-nothing', 2020-02-14),
	two advice settings were introduced into the `advice_config`
	array.

	Subsequently, c4a09cc9ccb (Merge branch 'hw/advise-ng',
	2020-03-25) started to deprecate `advice_config` in favor of a new
	array, `advice_setting`.

	However, the latter branch did not include the former branch, and
	therefore `advice_setting` is missing the two entries added by the
	`hw/advice-add-nothing` branch.

	These are currently the only entries in `advice_config` missing
	from `advice_setting`.

FWIW I manually verified that last paragraph's claim.

The diff itself looks correct to me.

Ciao, Dscho