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

[Ben Boeckel <mathstuf@gmail.com>]

On Mon, Aug 02, 2021 at 23:52:54 +0200, Johannes Schindelin wrote:
> 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).

Ah, I missed the logical merge conflict that was in effect here. I'll
add this to the commit message.

> 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.

I did as well :) ("All other global variable settings have entries
already."). I also verified the reverse, but that was going to be moot
with the other patches anyways. But having it called out as a separate
paragraph sounds better.

Thanks,

--Ben