[PATCH 0/3] meson: improve handling of `-Dbreaking_changes=true`

[PATCH 0/3] meson: improve handling of `-Dbreaking_changes=true`

[Patrick Steinhardt]

Hi,

this small patch series improves handling of the breaking changes option
with the Meson build system as discussed in the thread starting at [1].

Thanks!

Patrick

[1]: <56cf842a-7c1f-4354-b191-35bcc1e139bd@gmail.com>

---
Patrick Steinhardt (3):
      meson: define WITH_BREAKING_CHANGES when enabling breaking changes
      meson: don't compile git-pack-redundant(1) with breaking changes
      meson: don't install git-pack-redundant(1) docs with breaking changes

 Documentation/Makefile    |  2 +-
 Documentation/meson.build | 13 +++++++++++--
 meson.build               | 18 +++++++++++-------
 3 files changed, 23 insertions(+), 10 deletions(-)


---
base-commit: 87a0bdbf0f72b7561f3cd50636eee33dcb7dbcc3
change-id: 20250312-b4-pks-meson-breaking-changes-819afcca2e07

[PATCH 1/3] meson: define WITH_BREAKING_CHANGES when enabling breaking changes

[Patrick Steinhardt]

While Meson already supports the `-Dbreaking_changes=true` option, it
only wires up the build option that propagates into the tests. The build
option is only used for our tests to enable the `WITH_BREAKING_CHANGES`
prerequisite though, and does not influence the code that is actually
being built.

The omission went unnoticed because we only have tests right now that
get disabled when breaking changes are enabled, but not the other way
round. In other words, we don't have any tests that verify that breaking
changes behave as expected.

Fix the build issue by setting the `WITH_BREAKING_CHANGES` preprocessor
macro when breaking changes are enabled. Note that the `libgit_c_args`
array is defined after the current spot where we handle the option, so
to not have multiple sites where we handle it we instead move it after
the array has been defined.

Based-on-patch-by: Phillip Wood <phillip.wood123@gmail.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 meson.build | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/meson.build b/meson.build
index efe2871c9db..4ddc44f510d 100644
--- a/meson.build
+++ b/meson.build
@@ -672,12 +672,6 @@ build_options_config.set_quoted('GIT_TEST_UTF8_LOCALE', get_option('test_utf8_lo
 build_options_config.set_quoted('LOCALEDIR', fs.as_posix(get_option('prefix') / get_option('localedir')))
 build_options_config.set('GITWEBDIR', fs.as_posix(get_option('prefix') / get_option('datadir') / 'gitweb'))
 
-if get_option('breaking_changes')
-  build_options_config.set('WITH_BREAKING_CHANGES', 'YesPlease')
-else
-  build_options_config.set('WITH_BREAKING_CHANGES', '')
-endif
-
 if get_option('sane_tool_path').length() != 0
   sane_tool_path = (host_machine.system() == 'windows' ? ';' : ':').join(get_option('sane_tool_path'))
   build_options_config.set_quoted('BROKEN_PATH_FIX', 's|^\# @BROKEN_PATH_FIX@$|git_broken_path_fix "' + sane_tool_path + '"|')
@@ -739,6 +733,13 @@ if get_option('warning_level') in ['2','3', 'everything'] and compiler.get_argum
   endforeach
 endif
 
+if get_option('breaking_changes')
+  build_options_config.set('WITH_BREAKING_CHANGES', 'YesPlease')
+  libgit_c_args += '-DWITH_BREAKING_CHANGES'
+else
+  build_options_config.set('WITH_BREAKING_CHANGES', '')
+endif
+
 if get_option('b_sanitize').contains('address')
   build_options_config.set('SANITIZE_ADDRESS', 'YesCompiledWithIt')
 else

-- 
2.49.0.rc2.394.gf6994c5077.dirty

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

[Patrick Steinhardt]

We continue to compile the git-pack-redundant(1) builtin with Meson when
breaking changes are enabled even though we ultimately don't expose this
command at all. This is mostly harmless, but given that the intent of
the build option is to be as close as possible to the state where the
breaking change has been fully implemented this isn't optimal either.

Improve the situation by not compiling the builtin when breaking changes
are enabled.

Based-on-patch-by: Phillip Wood <phillip.wood123@gmail.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 meson.build | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/meson.build b/meson.build
index 4ddc44f510d..6b0eb6430af 100644
--- a/meson.build
+++ b/meson.build
@@ -581,7 +581,6 @@ builtin_sources = [
   'builtin/name-rev.c',
   'builtin/notes.c',
   'builtin/pack-objects.c',
-  'builtin/pack-redundant.c',
   'builtin/pack-refs.c',
   'builtin/patch-id.c',
   'builtin/prune-packed.c',
@@ -632,6 +631,10 @@ builtin_sources = [
   'builtin/write-tree.c',
 ]
 
+if not get_option('breaking_changes')
+  builtin_sources += 'builtin/pack-redundant.c'
+endif
+
 builtin_sources += custom_target(
   output: 'config-list.h',
   command: [

-- 
2.49.0.rc2.394.gf6994c5077.dirty

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

[Patrick Steinhardt]

When breaking changes are enabled we continue to install documentation
of the git-pack-redundant(1) command even though it is completely
disabled and thus inaccessible. Improve this by only installing the
documentation in case breaking changes aren't enabled.

Based-on-patch-by: Karthik Nayak <karthik.188@gmail.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 Documentation/Makefile    |  2 +-
 Documentation/meson.build | 13 +++++++++++--
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/Documentation/Makefile b/Documentation/Makefile
index 671267a8ac7..e6b20c021fd 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -509,7 +509,7 @@ lint-docs-meson:
 	awk "/^manpages = {$$/ {flag=1 ; next } /^}$$/ { flag=0 } flag { gsub(/^  \047/, \"\"); gsub(/\047 : [157],\$$/, \"\"); print }" meson.build | \
 		grep -v -e '#' -e '^$$' | \
 		sort >tmp-meson-diff/meson.adoc && \
-	ls git*.adoc scalar.adoc | grep -v -e git-bisect-lk2009.adoc -e git-tools.adoc >tmp-meson-diff/actual.adoc && \
+	ls git*.adoc scalar.adoc | grep -v -e git-bisect-lk2009.adoc -e git-pack-redundant.adoc -e git-tools.adoc >tmp-meson-diff/actual.adoc && \
 	if ! cmp tmp-meson-diff/meson.adoc tmp-meson-diff/actual.adoc; then \
 		echo "Meson man pages differ from actual man pages:"; \
 		diff -u tmp-meson-diff/meson.adoc tmp-meson-diff/actual.adoc; \
diff --git a/Documentation/meson.build b/Documentation/meson.build
index 594546d68b1..a2de85f5aad 100644
--- a/Documentation/meson.build
+++ b/Documentation/meson.build
@@ -96,7 +96,6 @@ manpages = {
   'git-notes.adoc' : 1,
   'git-p4.adoc' : 1,
   'git-pack-objects.adoc' : 1,
-  'git-pack-redundant.adoc' : 1,
   'git-pack-refs.adoc' : 1,
   'git-patch-id.adoc' : 1,
   'git-prune-packed.adoc' : 1,
@@ -205,6 +204,14 @@ manpages = {
   'gitworkflows.adoc' : 7,
 }
 
+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
 # 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' ]
 actual_manpages = run_command(shell, '-c', 'ls git*.adoc scalar.adoc',
   check: true,
   env: script_environment,

-- 
2.49.0.rc2.394.gf6994c5077.dirty

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

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 3061 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> When breaking changes are enabled we continue to install documentation
> of the git-pack-redundant(1) command even though it is completely
> disabled and thus inaccessible. Improve this by only installing the
> documentation in case breaking changes aren't enabled.
>
> Based-on-patch-by: Karthik Nayak <karthik.188@gmail.com>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  Documentation/Makefile    |  2 +-
>  Documentation/meson.build | 13 +++++++++++--
>  2 files changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/Makefile b/Documentation/Makefile
> index 671267a8ac7..e6b20c021fd 100644
> --- a/Documentation/Makefile
> +++ b/Documentation/Makefile
> @@ -509,7 +509,7 @@ lint-docs-meson:
>  	awk "/^manpages = {$$/ {flag=1 ; next } /^}$$/ { flag=0 } flag { gsub(/^  \047/, \"\"); gsub(/\047 : [157],\$$/, \"\"); print }" meson.build | \
>  		grep -v -e '#' -e '^$$' | \
>  		sort >tmp-meson-diff/meson.adoc && \
> -	ls git*.adoc scalar.adoc | grep -v -e git-bisect-lk2009.adoc -e git-tools.adoc >tmp-meson-diff/actual.adoc && \
> +	ls git*.adoc scalar.adoc | grep -v -e git-bisect-lk2009.adoc -e git-pack-redundant.adoc -e git-tools.adoc >tmp-meson-diff/actual.adoc && \
>  	if ! cmp tmp-meson-diff/meson.adoc tmp-meson-diff/actual.adoc; then \
>  		echo "Meson man pages differ from actual man pages:"; \
>  		diff -u tmp-meson-diff/meson.adoc tmp-meson-diff/actual.adoc; \

Nice, I totally missed then when I was tinkering with this issue.

> diff --git a/Documentation/meson.build b/Documentation/meson.build
> index 594546d68b1..a2de85f5aad 100644
> --- a/Documentation/meson.build
> +++ b/Documentation/meson.build
> @@ -96,7 +96,6 @@ manpages = {
>    'git-notes.adoc' : 1,
>    'git-p4.adoc' : 1,
>    'git-pack-objects.adoc' : 1,
> -  'git-pack-redundant.adoc' : 1,
>    'git-pack-refs.adoc' : 1,
>    'git-patch-id.adoc' : 1,
>    'git-prune-packed.adoc' : 1,
> @@ -205,6 +204,14 @@ manpages = {
>    'gitworkflows.adoc' : 7,
>  }
>
> +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
>  # 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' ]
>  actual_manpages = run_command(shell, '-c', 'ls git*.adoc scalar.adoc',
>    check: true,
>    env: script_environment,
>
> --
> 2.49.0.rc2.394.gf6994c5077.dirty

The rest looks good to me. Thanks!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

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

[Phillip Wood]

Hi Patrick

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

Thanks for taking this forward

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

Best Wishes

Phillip

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

[Patrick Steinhardt]

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

Patrick

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

[Phillip Wood]

Hi Patrick

On 17/03/2025 13:57, Patrick Steinhardt wrote:
> 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

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

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

[Phillip Wood]

On 17/03/2025 14:50, Phillip Wood wrote:
> 
> 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.

I think the answer is that the comparison always fails but as there are 
no missing or superfluous man pages the body of the if does not error out.

Best Wishes

Phillip

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

[Patrick Steinhardt]

On Tue, Mar 18, 2025 at 10:06:51AM +0000, Phillip Wood wrote:
> On 17/03/2025 14:50, Phillip Wood wrote:
> > 
> > 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.
> 
> I think the answer is that the comparison always fails but as there are no
> missing or superfluous man pages the body of the if does not error out.

Yeah. We indeed may have it multiple times now, but as you noticed it
ultimately still works. I didn't care too deeply to avoid the
duplication because in the end this step is only used to verify that we
have all manpages wired up in Meson.

Patrick

Re: [PATCH 0/3] meson: improve handling of `-Dbreaking_changes=true`

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 975 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> Hi,
>
> this small patch series improves handling of the breaking changes option
> with the Meson build system as discussed in the thread starting at [1].
>
> Thanks!
>
> Patrick
>
> [1]: <56cf842a-7c1f-4354-b191-35bcc1e139bd@gmail.com>
>

Thanks for picking this up. I think the series looks good, and is inline
with the discussion we had earlier.

> ---
> Patrick Steinhardt (3):
>       meson: define WITH_BREAKING_CHANGES when enabling breaking changes
>       meson: don't compile git-pack-redundant(1) with breaking changes
>       meson: don't install git-pack-redundant(1) docs with breaking changes
>
>  Documentation/Makefile    |  2 +-
>  Documentation/meson.build | 13 +++++++++++--
>  meson.build               | 18 +++++++++++-------
>  3 files changed, 23 insertions(+), 10 deletions(-)
>
>
> ---
> base-commit: 87a0bdbf0f72b7561f3cd50636eee33dcb7dbcc3
> change-id: 20250312-b4-pks-meson-breaking-changes-819afcca2e07

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]