| Message ID | xmqqzfhzlbie.fsf_-_@gitster.g (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | docs: fix check-docs with WITH_BREAKING_CHANGES | expand |
On 05/03/2025 15:53, Junio C Hamano wrote: > We correctly omit builtin/pack-objects.o from BUILTIN_OBJS, but > forgot to add "git pack-redundant" on the EXCLUDED_PROGRAMS list, > which made "make check-docs" target notice that the command has been > removed but still is documented. > > Signed-off-by: Junio C Hamano <gitster@pobox.com> > --- > * The command is still listed in the resulting "git help git" > output, as cmd-list.perl does not yet know which commands on the > list are to be ignored under WITH_BREAKING_CHANGES. Good catch. It seems the meson build was also forgotten in 68f51871df8 (builtin/pack-redundant: remove subcommand with breaking changes, 2025-01-22) as we still compile builtin/pack-redundant.c and build the documentation. We should probably wrap the function declaration for cmd_pack_redundant() in builtin.h with "#ifndef WITH_BREAKING_CHANGES" as well though I don't think that is urgent. Best Wishes Phillip > Makefile | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git c/Makefile w/Makefile > index a9b2de0692..95ac0820e9 100644 > --- c/Makefile > +++ w/Makefile > @@ -1283,7 +1283,9 @@ BUILTIN_OBJS += builtin/mv.o > BUILTIN_OBJS += builtin/name-rev.o > BUILTIN_OBJS += builtin/notes.o > BUILTIN_OBJS += builtin/pack-objects.o > -ifndef WITH_BREAKING_CHANGES > +ifdef WITH_BREAKING_CHANGES > +EXCLUDED_PROGRAMS += git-pack-redundant > +else > BUILTIN_OBJS += builtin/pack-redundant.o > endif > BUILTIN_OBJS += builtin/pack-refs.o
On 07/03/2025 10:32, Phillip Wood wrote: > On 05/03/2025 15:53, Junio C Hamano wrote: >> We correctly omit builtin/pack-objects.o from BUILTIN_OBJS, but >> forgot to add "git pack-redundant" on the EXCLUDED_PROGRAMS list, >> which made "make check-docs" target notice that the command has been >> removed but still is documented. >> >> Signed-off-by: Junio C Hamano <gitster@pobox.com> >> --- >> * The command is still listed in the resulting "git help git" >> output, as cmd-list.perl does not yet know which commands on the >> list are to be ignored under WITH_BREAKING_CHANGES. > > Good catch. It seems the meson build was also forgotten in 68f51871df8 > (builtin/pack-redundant: remove subcommand with breaking changes, > 2025-01-22) as we still compile builtin/pack-redundant.c and build the > documentation. We should probably wrap the function declaration for > cmd_pack_redundant() in builtin.h with "#ifndef WITH_BREAKING_CHANGES" > as well though I don't think that is urgent. I just had a look at fixing the meson build but it seems to be tricky as the manpage sources are stored in a meson dictionary and meson dictionaries are immutable so I don't know how one is supposed to conditionally add items. I also noticed that while we store the correct value for WITH_BREAKING_CHANGES in GIT-BUILD-OPTIONS it is not defined when building the C sources and so we still build the pack-redundant builtin. The diff below stops us from building pack-redundant with -Dbreaking_changes=true but still builds the documentation. I don't intend spending any more time one this Best Wishes Phillip diff --git a/builtin.h b/builtin.h index 89928ccf92f..8483975c191 100644 --- a/builtin.h +++ b/builtin.h @@ -197,7 +197,9 @@ int cmd_mv(int argc, const char **argv, const char *prefix, struct repository *r int cmd_name_rev(int argc, const char **argv, const char *prefix, struct repository *repo); int cmd_notes(int argc, const char **argv, const char *prefix, struct repository *repo); int cmd_pack_objects(int argc, const char **argv, const char *prefix, struct repository *repo); +#ifndef WITH_BREAKING_CHANGES int cmd_pack_redundant(int argc, const char **argv, const char *prefix, struct repository *repo); +#endif int cmd_patch_id(int argc, const char **argv, const char *prefix, struct repository *repo); int cmd_prune(int argc, const char **argv, const char *prefix, struct repository *repo); int cmd_prune_packed(int argc, const char **argv, const char *prefix, struct repository *repo); diff --git a/meson.build b/meson.build index e86085b0a47..5c039fe525a 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: [ @@ -674,6 +677,7 @@ build_options_config.set('GITWEBDIR', fs.as_posix(get_option('prefix') / get_opt if get_option('breaking_changes') build_options_config.set('WITH_BREAKING_CHANGES', 'YesPlease') + add_project_arguments('-DWITH_BREAKING_CHANGES=YesPlease', language : 'c') else build_options_config.set('WITH_BREAKING_CHANGES', '') endif
Phillip Wood <phillip.wood123@gmail.com> writes: > I also noticed that while we store the correct value for > WITH_BREAKING_CHANGES in GIT-BUILD-OPTIONS it is not defined when > building the C sources and so we still build the pack-redundant builtin. > > The diff below stops us from building pack-redundant with > -Dbreaking_changes=true but still builds the documentation. I don't intend > spending any more time one this Thanks. I am afraid that Patrick's plate may already be full, but I am hoping that there are others who are interested in getting the meson build support into a usable shape. Any takers?
FPhillip Wood <phillip.wood123@gmail.com> writes: > On 07/03/2025 10:32, Phillip Wood wrote: >> On 05/03/2025 15:53, Junio C Hamano wrote: >>> We correctly omit builtin/pack-objects.o from BUILTIN_OBJS, but >>> forgot to add "git pack-redundant" on the EXCLUDED_PROGRAMS list, >>> which made "make check-docs" target notice that the command has been >>> removed but still is documented. >>> >>> Signed-off-by: Junio C Hamano <gitster@pobox.com> >>> --- >>> * The command is still listed in the resulting "git help git" >>> output, as cmd-list.perl does not yet know which commands on the >>> list are to be ignored under WITH_BREAKING_CHANGES. >> >> Good catch. It seems the meson build was also forgotten in 68f51871df8 >> (builtin/pack-redundant: remove subcommand with breaking changes, >> 2025-01-22) as we still compile builtin/pack-redundant.c and build the >> documentation. We should probably wrap the function declaration for >> cmd_pack_redundant() in builtin.h with "#ifndef WITH_BREAKING_CHANGES" >> as well though I don't think that is urgent. > > I just had a look at fixing the meson build but it seems to be tricky as > the manpage sources are stored in a meson dictionary and meson > dictionaries are immutable so I don't know how one is supposed to > conditionally add items. > But dictonaries can be combined [1]. So we could probably do something like I've added below. [1]: https://mesonbuild.com/Reference-manual_elementary_dict.html -- 8< -- diff --git a/Documentation/meson.build b/Documentation/meson.build index 0a0f2bfa14..fcfec63e9b 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() @@ -475,7 +482,7 @@ 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() + manpages_breaking_changes.keys() + [ 'git-bisect-lk2009.adoc', 'git-tools.adoc' ] actual_manpages = run_command(shell, '-c', 'ls git*.adoc scalar.adoc', check: true, env: script_environment,
On 07/03/2025 15:07, Phillip Wood wrote: > On 07/03/2025 10:32, Phillip Wood wrote: > > The diff below stops us from building pack-redundant with > -Dbreaking_changes=true but still builds the documentation. I don't intend > spending any more time one this > > [...] > > if get_option('breaking_changes') > build_options_config.set('WITH_BREAKING_CHANGES', 'YesPlease') > + add_project_arguments('-DWITH_BREAKING_CHANGES=YesPlease', language : > 'c') Looking again at this I think it should probably be libgit_c_args += '-DWITH_BREAKING_CHANGES=YesPlease' to match the rest of our meson.build. As a newcomer to meson I find it confusing that the CFLAGS for the build targets are set implicitly by their libgit dependency. Best Wishes Phillip
Hi Karthik On 07/03/2025 22:42, Karthik Nayak wrote: > FPhillip Wood <phillip.wood123@gmail.com> writes: > >> On 07/03/2025 10:32, Phillip Wood wrote: >>> On 05/03/2025 15:53, Junio C Hamano wrote: >>>> We correctly omit builtin/pack-objects.o from BUILTIN_OBJS, but >>>> forgot to add "git pack-redundant" on the EXCLUDED_PROGRAMS list, >>>> which made "make check-docs" target notice that the command has been >>>> removed but still is documented. >>>> >>>> Signed-off-by: Junio C Hamano <gitster@pobox.com> >>>> --- >>>> * The command is still listed in the resulting "git help git" >>>> output, as cmd-list.perl does not yet know which commands on the >>>> list are to be ignored under WITH_BREAKING_CHANGES. >>> >>> Good catch. It seems the meson build was also forgotten in 68f51871df8 >>> (builtin/pack-redundant: remove subcommand with breaking changes, >>> 2025-01-22) as we still compile builtin/pack-redundant.c and build the >>> documentation. We should probably wrap the function declaration for >>> cmd_pack_redundant() in builtin.h with "#ifndef WITH_BREAKING_CHANGES" >>> as well though I don't think that is urgent. >> >> I just had a look at fixing the meson build but it seems to be tricky as >> the manpage sources are stored in a meson dictionary and meson >> dictionaries are immutable so I don't know how one is supposed to >> conditionally add items. >> > > But dictonaries can be combined [1]. So we could probably do something > like I've added below. Thanks, my web search took me to a different page in the documentation [1]. Looking carefully there is an example of adding an element to a dictionary right at the end of that section but it is not mentioned anywhere in the text. I do find the meson documentation hard to use. I think it would be best if someone with more knowledge of meson than me took this forward Thanks Phillip [1] https://mesonbuild.com/Syntax.html#dictionaries > [1]: https://mesonbuild.com/Reference-manual_elementary_dict.html > > -- 8< -- > > diff --git a/Documentation/meson.build b/Documentation/meson.build > index 0a0f2bfa14..fcfec63e9b 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() > @@ -475,7 +482,7 @@ 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() + > manpages_breaking_changes.keys() + [ 'git-bisect-lk2009.adoc', > 'git-tools.adoc' ] > actual_manpages = run_command(shell, '-c', 'ls git*.adoc scalar.adoc', > check: true, > env: script_environment,
On Sun, Mar 09, 2025 at 10:52:44AM +0000, Phillip Wood wrote: > On 07/03/2025 15:07, Phillip Wood wrote: > > On 07/03/2025 10:32, Phillip Wood wrote: > > > > The diff below stops us from building pack-redundant with > > -Dbreaking_changes=true but still builds the documentation. I don't intend > > spending any more time one this > > > > [...] > > > > if get_option('breaking_changes') > > build_options_config.set('WITH_BREAKING_CHANGES', 'YesPlease') > > + add_project_arguments('-DWITH_BREAKING_CHANGES=YesPlease', language : > > 'c') > > Looking again at this I think it should probably be > > libgit_c_args += '-DWITH_BREAKING_CHANGES=YesPlease' > > to match the rest of our meson.build. As a newcomer to meson I find it > confusing that the CFLAGS for the build targets are set implicitly by their > libgit dependency. Yup, that would be preferable indeed, thanks! Patrick
On Mon, Mar 10, 2025 at 07:42:25AM +0100, Patrick Steinhardt wrote: > On Sun, Mar 09, 2025 at 10:52:44AM +0000, Phillip Wood wrote: > > On 07/03/2025 15:07, Phillip Wood wrote: > > > On 07/03/2025 10:32, Phillip Wood wrote: > > > > > > The diff below stops us from building pack-redundant with > > > -Dbreaking_changes=true but still builds the documentation. I don't intend > > > spending any more time one this > > > > > > [...] > > > > > > if get_option('breaking_changes') > > > build_options_config.set('WITH_BREAKING_CHANGES', 'YesPlease') > > > + add_project_arguments('-DWITH_BREAKING_CHANGES=YesPlease', language : > > > 'c') > > > > Looking again at this I think it should probably be > > > > libgit_c_args += '-DWITH_BREAKING_CHANGES=YesPlease' > > > > to match the rest of our meson.build. As a newcomer to meson I find it > > confusing that the CFLAGS for the build targets are set implicitly by their > > libgit dependency. > > Yup, that would be preferable indeed, thanks! To set expectations: do you have the time/intent to work on this and polish it up into a patch? Otherwise I'm happy to pick it up. Thanks! Patrick
diff --git c/Makefile w/Makefile index a9b2de0692..95ac0820e9 100644 --- c/Makefile +++ w/Makefile @@ -1283,7 +1283,9 @@ BUILTIN_OBJS += builtin/mv.o BUILTIN_OBJS += builtin/name-rev.o BUILTIN_OBJS += builtin/notes.o BUILTIN_OBJS += builtin/pack-objects.o -ifndef WITH_BREAKING_CHANGES +ifdef WITH_BREAKING_CHANGES +EXCLUDED_PROGRAMS += git-pack-redundant +else BUILTIN_OBJS += builtin/pack-redundant.o endif BUILTIN_OBJS += builtin/pack-refs.o
We correctly omit builtin/pack-objects.o from BUILTIN_OBJS, but forgot to add "git pack-redundant" on the EXCLUDED_PROGRAMS list, which made "make check-docs" target notice that the command has been removed but still is documented. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- * The command is still listed in the resulting "git help git" output, as cmd-list.perl does not yet know which commands on the list are to be ignored under WITH_BREAKING_CHANGES. Makefile | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)