Re: [PATCH 3/3] meson: don't install git-pack-redundant(1) docs with breaking changes

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Mar 17, 2025, 14:50 UTC
Message-ID: <d341777a-a6e5-46fe-8004-9fe885321905@gmail.com>
In-Reply-To: <Z9gqP3ng0a9Zfpqc@pks.im>

Hi Patrick

On 17/03/2025 13:57, Patrick Steinhardt wrote:

Show 15 quoted lines

> On Sun, Mar 16, 2025 at 03:19:48PM +0000, Phillip Wood wrote:
>> On 12/03/2025 13:17, Patrick Steinhardt wrote:
>>> +manpages_breaking_changes = {
>>> +  'git-pack-redundant.adoc' : 1,
>>> +}
>>> +
>>> +if not get_option('breaking_changes')
>>> +  manpages += manpages_breaking_changes
>>> +endif
>>> +
>>>    docs_backend = get_option('docs_backend')
>>>    if docs_backend == 'auto'
>>>      if find_program('asciidoc', dirs: program_path, required: false).found()
>>> @@ -479,7 +486,9 @@ endif
>>>    # Sanity check that we are not missing any tests present in 't/'. This check

I think this part of the comment must have been copied from somewhere else

Show 15 quoted lines

>>>    # only runs once at configure time and is thus best-effort, only. Furthermore,
>>>    # it only verifies man pages for the sake of simplicity.
>>> -configured_manpages = manpages.keys() + [ 'git-bisect-lk2009.adoc', 'git-tools.adoc' ]
>>> +configured_manpages = manpages.keys()
>>> +configured_manpages += manpages_breaking_changes.keys()
>>> +configured_manpages += [ 'git-bisect-lk2009.adoc', 'git-tools.adoc' ]
>>
>> I don't think we need this hunk as we add manpages_breaking_changes into
>> manpages in the hunk above.
> 
> We indeed need it: it's required in case the 'breaking_changes' option
> is enabled. In that case we still need to have the man pages here in
> this variable because we use it to check that the manpage is handled at
> all. Otherwise we would error out because Meson thinks that we forgot to
> wire up this manpage that we found in the source directory.

Oh so if we have selected breaking_changes then manpages.keys() does not include "git-pack-redundant.adoc" but that file exists and so we need to add it to the list of configured man pages. If breaking_changes is selected then don't we end up adding "git-pack-redundant.adoc" to configured_manpages twice? Does that matter when we come to do

actual_manpages = run_command(shell, '-c', 'ls git*.adoc scalar.adoc',
   check: true,
   env: script_environment,
).stdout().strip().split('\n')

if configured_manpages != actual_manpages
   ...

? Also I'm confused as to how that comparison works without sorting configured_manpages. Even if manpages.keys() sorts the keys (the documentation at [1] is silent on that) we add some out-of-order entries to the end of the list.

Best Wishes

Phillip

[1] https://mesonbuild.com/Reference-manual_elementary_dict.html#dictkeys