Skip to content

Conversation

phillipwood
Copy link

@phillipwood phillipwood commented Mar 3, 2025

Thanks to Junio and Patrick for their comments on V1. I've renamed the attribute to with-breaking-changes as suggested. I'll add a patch to rename the test prerequisite to match this when I re-roll https://lore.kernel.org/git/[email protected]/ to use WITH_BREAKING_CHANGES.

Cc: Patrick Steinhardt [email protected]
Cc: Junio C Hamano [email protected]
cc: Phillip Wood [email protected]
cc: Karthik Nayak [email protected]

@phillipwood
Copy link
Author

/submit

Copy link

gitgitgadget bot commented Mar 3, 2025

Submitted as [email protected]

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1871/phillipwood/breaking-changes-documentation-v1

To fetch this version to local tag pr-1871/phillipwood/breaking-changes-documentation-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1871/phillipwood/breaking-changes-documentation-v1

Copy link

gitgitgadget bot commented Mar 3, 2025

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Phillip Wood via GitGitGadget" <[email protected]> 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.

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

The above parts of the documentation getting commented out all look
sensible to exclude in a build that omits these older mechanisms.
But can we do it with !with-breaking-changes instead?

Copy link

gitgitgadget bot commented Mar 4, 2025

On the Git mailing list, Patrick Steinhardt wrote (reply to this):

On Mon, Mar 03, 2025 at 10:18:05AM -0800, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <[email protected]> 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.

Patrick

Copy link

gitgitgadget bot commented Mar 4, 2025

On the Git mailing list, Phillip Wood wrote (reply to this):

Hi Patrick

On 04/03/2025 06:35, Patrick Steinhardt wrote:
> On Mon, Mar 03, 2025 at 10:18:05AM -0800, Junio C Hamano wrote:
>> "Phillip Wood via GitGitGadget" <[email protected]> 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.

Yes, I can do that when I re-roll the patches at https://lore.kernel.org/git/[email protected]/ to use WITH_BREAKING_CHANGES

Best Wishes

Phillip

> Patrick
> 

Copy link

gitgitgadget bot commented Mar 4, 2025

User Phillip Wood <[email protected]> has been added to the cc: list.

@phillipwood phillipwood force-pushed the breaking-changes-documentation branch from 4287701 to 5feb880 Compare March 4, 2025 14:22
Copy link

gitgitgadget bot commented Mar 4, 2025

On the Git mailing list, Junio C Hamano wrote (reply to this):

Phillip Wood <[email protected]> writes:

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

Thanks, both.

@phillipwood phillipwood force-pushed the breaking-changes-documentation branch from 5feb880 to e9e7825 Compare March 4, 2025 16:34
Since commit 8ccc75c (remote: announce removal of "branches/" and
"remotes/", 2025-01-22) enabling WITH_BREAKING_CHANGES when building git
removes support for reading branches from ".git/branches" and remotes
from ".git/remotes". However those locations are still documented in
gitrepository-layout.adoc even though the build does not support them.

Rectify this by adding a new document attribute "with-breaking-changes"
and use it to make the inclusion of those sections of the documentation
conditional. Note that the name of the attribute does not match the test
prerequisite WITHOUT_BREAKING_CHANGES added in c5bc9a7 (Makefile:
wire up build option for deprecated features, 2025-01-22). This is to
avoid the awkward double negative ifndef::without_breaking_changes for
documentation that should be included when WITH_BREAKING_CHANGES is
enabled. The test prerequisite will be renamed to match the
documentation attribute in a future patch series.

Signed-off-by: Phillip Wood <[email protected]>
@phillipwood phillipwood force-pushed the breaking-changes-documentation branch from e9e7825 to d8dcdca Compare March 4, 2025 16:50
@phillipwood
Copy link
Author

/submit

Copy link

gitgitgadget bot commented Mar 5, 2025

Submitted as [email protected]

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1871/phillipwood/breaking-changes-documentation-v2

To fetch this version to local tag pr-1871/phillipwood/breaking-changes-documentation-v2:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1871/phillipwood/breaking-changes-documentation-v2

Copy link

gitgitgadget bot commented Mar 5, 2025

On the Git mailing list, Junio C Hamano wrote (reply to this):

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

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

Copy link

gitgitgadget bot commented Mar 5, 2025

This patch series was integrated into seen via git@d23cc49.

@gitgitgadget gitgitgadget bot added the seen label Mar 5, 2025
Copy link

gitgitgadget bot commented Mar 6, 2025

This branch is now known as pw/repo-layout-doc-update.

Copy link

gitgitgadget bot commented Mar 6, 2025

This patch series was integrated into seen via git@5592ad2.

Copy link

gitgitgadget bot commented Mar 6, 2025

This patch series was integrated into next via git@2de1596.

@gitgitgadget gitgitgadget bot added the next label Mar 6, 2025
Copy link

gitgitgadget bot commented Mar 6, 2025

This patch series was integrated into seen via git@bc86ef1.

Copy link

gitgitgadget bot commented Mar 6, 2025

This patch series was integrated into master via git@bc86ef1.

Copy link

gitgitgadget bot commented Mar 6, 2025

This patch series was integrated into next via git@bc86ef1.

@gitgitgadget gitgitgadget bot added the master label Mar 6, 2025
Copy link

gitgitgadget bot commented Mar 6, 2025

Closed via bc86ef1.

@gitgitgadget gitgitgadget bot closed this Mar 6, 2025
Copy link

gitgitgadget bot commented Mar 7, 2025

On the Git mailing list, Phillip Wood wrote (reply to this):

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

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

Copy link

gitgitgadget bot commented Mar 7, 2025

On the Git mailing list, Phillip Wood wrote (reply to 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 <[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 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

Copy link

gitgitgadget bot commented Mar 7, 2025

On the Git mailing list, Junio C Hamano wrote (reply to this):

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

Copy link

gitgitgadget bot commented Mar 7, 2025

On the Git mailing list, Karthik Nayak wrote (reply to this):

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

Copy link

gitgitgadget bot commented Mar 7, 2025

User Karthik Nayak <[email protected]> has been added to the cc: list.

Copy link

gitgitgadget bot commented Mar 9, 2025

On the Git mailing list, Phillip Wood wrote (reply to 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.

Best Wishes

Phillip

Copy link

gitgitgadget bot commented Mar 9, 2025

On the Git mailing list, Phillip Wood wrote (reply to this):

Hi Karthik

On 07/03/2025 22:42, Karthik Nayak wrote:
> FPhillip Wood <[email protected]> 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 <[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 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,

Copy link

gitgitgadget bot commented Mar 10, 2025

On the Git mailing list, Patrick Steinhardt wrote (reply to this):

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

Copy link

gitgitgadget bot commented Mar 11, 2025

On the Git mailing list, Patrick Steinhardt wrote (reply to this):

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

Copy link

gitgitgadget bot commented Mar 12, 2025

On the Git mailing list, [email protected] wrote (reply to this):

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.

Thanks

Phillip

Copy link

gitgitgadget bot commented Mar 12, 2025

On the Git mailing list, Patrick Steinhardt wrote (reply to this):

On Wed, Mar 12, 2025 at 10:39:49AM +0000, [email protected] wrote:
> 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.

Okay, I've sent the patch series in [1]. Thanks!

[1]: <[email protected]>

Patrick

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant