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

Documentation/gitrepository-layout.adoc +3 -0

+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',

Quoted 6 lines · Phillip Wood via GitGitGadget

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.

Quoted 9 lines · Junio C Hamano

Quoted 6 lines · Phillip Wood via GitGitGadget

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.

Quoted 5 lines

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.

Patrick

Quoted 3 lines

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/[email protected]/
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 <[email protected]>

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

Makefile +3 -1

index a9b2de0692..95ac0820e9 100644
@@ -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

Quoted 5 lines · Junio C Hamano

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.

Quoted 5 lines

Signed-off-by Junio C Hamano <[email protected]>

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

Quoted 12 lines · Phillip Wood

Quoted 10 lines · Junio C Hamano

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 <[email protected]>

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

Quoted 6 lines

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.

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

Quoted 20 lines · FPhillip Wood

Quoted 18 lines · Phillip Wood

Quoted 10 lines · Junio C Hamano

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 <[email protected]>

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

Quoted 5 lines

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

Documentation/meson.build +8 -2

index 0a0f2bfa14..fcfec63e9b 100644
@@ -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,

Quoted 6 lines · Phillip Wood

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

Quoted 6 lines

[...]

 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.

Quoted 16 lines · Phillip Wood

Quoted 6 lines · Phillip Wood

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

Quoted 6 lines

[...]

 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

Quoted 5 lines

    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!

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.

Hi Patrick

Quoted 4 lines · Patrick Steinhardt

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.