patchdocs: fix repository-layout when building with breaking changes

>     I copied the name from the test prerequisite as I didn't want to have
>     different names for condition used in the tests and documentation. I do
>     have some reservations about the naming though as it means we end up
>     having to use ifdef::!without-breaking-changes[] or test_expect_success
>     !WITHOUT_BREAKING_CHANGES to document and test breaking changes which is
>     a double negative.

> +ifdef::without-breaking-changes[]
>  branches::
>  	A deprecated way to store shorthands to be used
>  	to specify a URL to 'git fetch', 'git pull' and 'git push'.
> @@ -164,6 +165,7 @@ branches::
>  	"$GIT_COMMON_DIR/branches" will be used instead.
>  +
>  Git will stop reading remotes from this directory in Git 3.0.
> +endif::without-breaking-changes[]
>  
>  hooks::
>  	Hooks are customization scripts used by various Git
> @@ -231,6 +233,7 @@ info/sparse-checkout::
>  	This file stores sparse checkout patterns.
>  	See also: linkgit:git-read-tree[1].
>  
> +ifdef::without-breaking-changes[]
>  remotes::
>  	Stores shorthands for URL and default refnames for use
>  	when interacting with remote repositories via 'git fetch',
> @@ -241,6 +244,7 @@ remotes::
>  	"$GIT_COMMON_DIR/remotes" will be used instead.
>  +
>  Git will stop reading remotes from this directory in Git 3.0.
> +endif::without-breaking-changes[]
>  
>  logs::
>  	Records of changes made to refs are stored in this directory.

> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
> >     I copied the name from the test prerequisite as I didn't want to have
> >     different names for condition used in the tests and documentation. I do
> >     have some reservations about the naming though as it means we end up
> >     having to use ifdef::!without-breaking-changes[] or test_expect_success
> >     !WITHOUT_BREAKING_CHANGES to document and test breaking changes which is
> >     a double negative.
> 
> It was exactly the first thing that came to my mind when I saw the
> change to the Makefile in the patch.  Unless our breaking changes
> are all removals, which is not likely to be the case in the longer
> term, "without-breaking-changes" would be an invitation for
> confusing double negatives.

> On Mon, Mar 03, 2025 at 10:18:05AM -0800, Junio C Hamano wrote:
>> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>
>>>      I copied the name from the test prerequisite as I didn't want to have
>>>      different names for condition used in the tests and documentation. I do
>>>      have some reservations about the naming though as it means we end up
>>>      having to use ifdef::!without-breaking-changes[] or test_expect_success
>>>      !WITHOUT_BREAKING_CHANGES to document and test breaking changes which is
>>>      a double negative.
>>
>> It was exactly the first thing that came to my mind when I saw the
>> change to the Makefile in the patch.  Unless our breaking changes
>> are all removals, which is not likely to be the case in the longer
>> term, "without-breaking-changes" would be an invitation for
>> confusing double negatives.
> 
> I remember not quite being happy with the double-negation myself. I
> don't mind renaming the prerequisite we have in our test suite for
> consistency, as well, if you want to do that.

>> I remember not quite being happy with the double-negation myself. I
>> don't mind renaming the prerequisite we have in our test suite for
>> consistency, as well, if you want to do that.
>
> Yes, I can do that when I re-roll the patches at
> https://lore.kernel.org/git/pull.1863.git.1740149837.gitgitgadget@gmail.com/
> to use WITH_BREAKING_CHANGES

> 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(-)
> 
> 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 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 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

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

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

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

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

> Hi Patrick
> 
> On 11/03/2025 14:40, Patrick Steinhardt wrote:
> > On Mon, Mar 10, 2025 at 07:42:25AM +0100, Patrick Steinhardt wrote:
> > 
> > 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.
> 
> If you're happy to pick this up that would be great. I'm unlikely to have
> time for git related things for the next week or so and you're also much
> more familiar with meson than me.