[PATCH] strbuf: add compound literal test balloon

[PATCH] strbuf: add compound literal test balloon

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

A C99 compound literal creates an unnamed object whose value is given by
an initializer list. This allows us to simplify code where we cannot use
a designated initalizer because the values of some members of the object
need to be calculated first. For example this code from builtin/rebase.c

	struct strbuf branch_reflog = STRBUF_INIT, head_reflog = STRBUF_INIT;
	struct reset_head_opts ropts = { 0 };
	int ret;

	strbuf_addf(&branch_reflog, "%s (finish): %s onto %s",
		    opts->reflog_action,
		    opts->head_name, oid_to_hex(&opts->onto->object.oid));
	strbuf_addf(&head_reflog, "%s (finish): returning to %s",
		    opts->reflog_action, opts->head_name);
	ropts.branch = opts->head_name;
	ropts.flags = RESET_HEAD_REFS_ONLY;
	ropts.branch_msg = branch_reflog.buf;
	ropts.head_msg = head_reflog.buf;
	ret = reset_head(the_repository, &ropts);

can be be simplified to

	struct strbuf branch_reflog = STRBUF_INIT, head_reflog = STRBUF_INIT;
	int ret;

	strbuf_addf(&branch_reflog, "%s (finish): %s onto %s",
		    opts->reflog_action,
		    opts->head_name, oid_to_hex(&opts->onto->object.oid));
	strbuf_addf(&head_reflog, "%s (finish): returning to %s",
		    opts->reflog_action, opts->head_name);
        ret = reset_head(the_repository, &(struct reset_head_opts) {
                .branch = opts->head_name,
        	.flags = RESET_HEAD_REFS_ONLY,
        	.branch_msg = branch_reflog.buf,
        	.head_msg = head_reflog.buf,
        });

The result is more readable as one can see the value of each member
of the object being passed to the function at the call site rather
than building the object piecemeal in the preceding lines.

A common pattern in our code base is to define a struct together
with an initializer macro to initialize automatic variables and an
initializer function to initialize dynamically allocated instances
of the struct. Typically the initializer function for "struct foo"
looks like

        void foo_init(struct foo *f)
        {
                struct foo blank = FOO_INIT;
                memcpy(f, &blank, sizeof(*f));
        }

which enables us to reuse the initializer macro FOO_INIT to initalize
dynamically allocated objects. By using a compound literal we can
simplify this to

        void foo_init(struct foo *f)
        {
                *f = (struct foo) FOO_INIT;
        }

Let's add a test balloon to check for compiler support by changing
strbuf_init() to use a compound literal in the hope of using this
feature more widely in the future.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
Base-Commit: a30f80fde927d70950b3b4d1820813480968fb0d
Published-As: https://github.com/phillipwood/git/releases/tag/pw%2Fcompound-literal-test-balloon%2Fv1
View-Changes-At: https://github.com/phillipwood/git/compare/a30f80fde...7ac55a509
Fetch-It-Via: git fetch https://github.com/phillipwood/git pw/compound-literal-test-balloon/v1

 strbuf.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/strbuf.c b/strbuf.c
index f30fdc69843..c93c1208c93 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -68,8 +68,7 @@ char strbuf_slopbuf[1];
 
 void strbuf_init(struct strbuf *sb, size_t hint)
 {
-	struct strbuf blank = STRBUF_INIT;
-	memcpy(sb, &blank, sizeof(*sb));
+	*sb = (struct strbuf) STRBUF_INIT;
 	if (hint)
 		strbuf_grow(sb, hint);
 }
-- 
2.49.0.897.gfad3eb7d210

Re: [PATCH] strbuf: add compound literal test balloon

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> A C99 compound literal creates an unnamed object whose value is given by
> an initializer list. This allows us to simplify code where we cannot use
> a designated initalizer because the values of some members of the object
> need to be calculated first. For example this code from builtin/rebase.c
>
> 	struct strbuf branch_reflog = STRBUF_INIT, head_reflog = STRBUF_INIT;
> 	struct reset_head_opts ropts = { 0 };
> 	int ret;
>
> 	strbuf_addf(&branch_reflog, "%s (finish): %s onto %s",
> 		    opts->reflog_action,
> 		    opts->head_name, oid_to_hex(&opts->onto->object.oid));
> 	strbuf_addf(&head_reflog, "%s (finish): returning to %s",
> 		    opts->reflog_action, opts->head_name);
> 	ropts.branch = opts->head_name;
> 	ropts.flags = RESET_HEAD_REFS_ONLY;
> 	ropts.branch_msg = branch_reflog.buf;
> 	ropts.head_msg = head_reflog.buf;
> 	ret = reset_head(the_repository, &ropts);
>
> can be be simplified to
>
> 	struct strbuf branch_reflog = STRBUF_INIT, head_reflog = STRBUF_INIT;
> 	int ret;
>
> 	strbuf_addf(&branch_reflog, "%s (finish): %s onto %s",
> 		    opts->reflog_action,
> 		    opts->head_name, oid_to_hex(&opts->onto->object.oid));
> 	strbuf_addf(&head_reflog, "%s (finish): returning to %s",
> 		    opts->reflog_action, opts->head_name);
>         ret = reset_head(the_repository, &(struct reset_head_opts) {
>                 .branch = opts->head_name,
>         	.flags = RESET_HEAD_REFS_ONLY,
>         	.branch_msg = branch_reflog.buf,
>         	.head_msg = head_reflog.buf,
>         });
>
> The result is more readable as one can see the value of each member
> of the object being passed to the function at the call site rather
> than building the object piecemeal in the preceding lines.

Hmph, is it simpler, I have to wonder, in other words, I doubt you
simplified the code.

One thing the above rewrite did is to make it clear to readers that
the struct instance is used just once and then immediately got
discarded.  As long as the object that gets passed this way does not
hold resources that need to be discarded itself (in other words,
does not require a call to reset_head_opts_release()), it makes the
code easier to follow.

But once the struct gains members that need to be released, I am not
sure if this construct does not make it harder to spot leaks.
Somebody who adds a member to _release() to the struct presumably
audits and find all places that need to call _release(), but among
them they find this place---now what?  They need to first convert it
to the old fashioned way and then call _release() after the
reset_head() call returns, I guess.

I am not arguing against all uses of literals---I am just
anticipating future fallouts of encouraging overuse of this pattern,
and preparing what we would say when somebody adds a new use to
inappropriate places.  Simple cases like the initialier should be
fine.

>  strbuf.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/strbuf.c b/strbuf.c
> index f30fdc69843..c93c1208c93 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -68,8 +68,7 @@ char strbuf_slopbuf[1];
>  
>  void strbuf_init(struct strbuf *sb, size_t hint)
>  {
> -	struct strbuf blank = STRBUF_INIT;
> -	memcpy(sb, &blank, sizeof(*sb));

We should mark this clearly as a weather-balloon so that people do
not copy and paste it to elsewhere not just yet, shouldn't we?  If
it proliferates everywhere and then later we discover that some
vintage of compiler on a platform we still care about miscompiles
the construct, we want to keep it simpler to revert.

> +	*sb = (struct strbuf) STRBUF_INIT;
>  	if (hint)
>  		strbuf_grow(sb, hint);
>  }

Other than that, thanks for taking an initiative.

Re: [PATCH] strbuf: add compound literal test balloon

[Patrick Steinhardt]

On Mon, Jul 14, 2025 at 07:26:55AM -0700, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
> > From: Phillip Wood <phillip.wood@dunelm.org.uk>
> >
> > A C99 compound literal creates an unnamed object whose value is given by
> > an initializer list. This allows us to simplify code where we cannot use
> > a designated initalizer because the values of some members of the object
> > need to be calculated first. For example this code from builtin/rebase.c
> >
> > 	struct strbuf branch_reflog = STRBUF_INIT, head_reflog = STRBUF_INIT;
> > 	struct reset_head_opts ropts = { 0 };
> > 	int ret;
> >
> > 	strbuf_addf(&branch_reflog, "%s (finish): %s onto %s",
> > 		    opts->reflog_action,
> > 		    opts->head_name, oid_to_hex(&opts->onto->object.oid));
> > 	strbuf_addf(&head_reflog, "%s (finish): returning to %s",
> > 		    opts->reflog_action, opts->head_name);
> > 	ropts.branch = opts->head_name;
> > 	ropts.flags = RESET_HEAD_REFS_ONLY;
> > 	ropts.branch_msg = branch_reflog.buf;
> > 	ropts.head_msg = head_reflog.buf;
> > 	ret = reset_head(the_repository, &ropts);
> >
> > can be be simplified to
> >
> > 	struct strbuf branch_reflog = STRBUF_INIT, head_reflog = STRBUF_INIT;
> > 	int ret;
> >
> > 	strbuf_addf(&branch_reflog, "%s (finish): %s onto %s",
> > 		    opts->reflog_action,
> > 		    opts->head_name, oid_to_hex(&opts->onto->object.oid));
> > 	strbuf_addf(&head_reflog, "%s (finish): returning to %s",
> > 		    opts->reflog_action, opts->head_name);
> >         ret = reset_head(the_repository, &(struct reset_head_opts) {
> >                 .branch = opts->head_name,
> >         	.flags = RESET_HEAD_REFS_ONLY,
> >         	.branch_msg = branch_reflog.buf,
> >         	.head_msg = head_reflog.buf,
> >         });
> >
> > The result is more readable as one can see the value of each member
> > of the object being passed to the function at the call site rather
> > than building the object piecemeal in the preceding lines.
> 
> Hmph, is it simpler, I have to wonder, in other words, I doubt you
> simplified the code.
> 
> One thing the above rewrite did is to make it clear to readers that
> the struct instance is used just once and then immediately got
> discarded.  As long as the object that gets passed this way does not
> hold resources that need to be discarded itself (in other words,
> does not require a call to reset_head_opts_release()), it makes the
> code easier to follow.
> 
> But once the struct gains members that need to be released, I am not
> sure if this construct does not make it harder to spot leaks.
> Somebody who adds a member to _release() to the struct presumably
> audits and find all places that need to call _release(), but among
> them they find this place---now what?  They need to first convert it
> to the old fashioned way and then call _release() after the
> reset_head() call returns, I guess.
> 
> I am not arguing against all uses of literals---I am just
> anticipating future fallouts of encouraging overuse of this pattern,
> and preparing what we would say when somebody adds a new use to
> inappropriate places.  Simple cases like the initialier should be
> fine.

We already have a two test balloons, both defined in
"reftable/system.h":

    #define REFTABLE_FLOCK_INIT ((struct reftable_flock){ .fd = -1, })

    #define REFTABLE_TMPFILE_INIT ((struct reftable_tmpfile) { .fd = -1, })

Both of those are getting used in a way that'd break if those weren't
properly supported in "reftable/stack.c":

	for (i = 0; i < last - first + 1; i++)
		table_locks[i] = REFTABLE_FLOCK_INIT;

	tab_file = REFTABLE_TMPFILE_INIT;

Those are rather recent additions though, released with Git 2.50. I also
totally missed that we didn't have any test balloons for this syntax.
Should we maybe retroactively mark them as test balloons instead of
converting and marking some new sites?

Patrick

Re: [PATCH] strbuf: add compound literal test balloon

[Phillip Wood]

Hi Patrick

On 15/07/2025 09:53, Patrick Steinhardt wrote:
> 
> We already have a two test balloons, both defined in
> "reftable/system.h":
> 
>      #define REFTABLE_FLOCK_INIT ((struct reftable_flock){ .fd = -1, })
> 
>      #define REFTABLE_TMPFILE_INIT ((struct reftable_tmpfile) { .fd = -1, })
> 
> Both of those are getting used in a way that'd break if those weren't
> properly supported in "reftable/stack.c":
> 
> 	for (i = 0; i < last - first + 1; i++)
> 		table_locks[i] = REFTABLE_FLOCK_INIT;
> 
> 	tab_file = REFTABLE_TMPFILE_INIT;
> 
> Those are rather recent additions though, released with Git 2.50. I also
> totally missed that we didn't have any test balloons for this syntax.
> Should we maybe retroactively mark them as test balloons instead of
> converting and marking some new sites?

It would definitely be worth marking them as test balloons. So long as 
it is not possible to build git without the reftable library then they 
should suffice without adding any more. I think it would be worth adding 
a section to CodingGuildlines.adoc about the balloons we current have as 
well (I didn't add that here as I wanted to wait until we knew the 
commit id that introduced the balloon).

Thanks

Phillip

Re: [PATCH] strbuf: add compound literal test balloon

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> We already have a two test balloons, both defined in
> "reftable/system.h":
>
>     #define REFTABLE_FLOCK_INIT ((struct reftable_flock){ .fd = -1, })
>
>     #define REFTABLE_TMPFILE_INIT ((struct reftable_tmpfile) { .fd = -1, })
>
> Both of those are getting used in a way that'd break if those weren't
> properly supported in "reftable/stack.c":
>
> 	for (i = 0; i < last - first + 1; i++)
> 		table_locks[i] = REFTABLE_FLOCK_INIT;
>
> 	tab_file = REFTABLE_TMPFILE_INIT;
>
> Those are rather recent additions though, released with Git 2.50. I also
> totally missed that we didn't have any test balloons for this syntax.
> Should we maybe retroactively mark them as test balloons instead of
> converting and marking some new sites?

That sounds good.  I was wondering if it is easier to keep track of
things to add a new section to the CodingGuildlines document,
perhaps like this?

 Documentation/CodingGuidelines | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git c/Documentation/CodingGuidelines w/Documentation/CodingGuidelines
index 6350949f2e..dd3dbb9c57 100644
--- c/Documentation/CodingGuidelines
+++ w/Documentation/CodingGuidelines
@@ -298,6 +298,14 @@ For C programs:
    . since late 2021 with 44ba10d6, we have had variables declared in
      the for loop "for (int i = 0; i < 10; i++)".
 
+   C99 features we have test balloons for:
+
+   . since late 2024 with v2.48.0-rc0~20, we have test balloons for
+     compound literal syntax, e.g., (struct foo){ .member = value };
+     our hope is that no platforms we care about have trouble using
+     them, and officially adopt its wider use in mid 2026.  Do not add
+     more use of the syntax until that happens.
+
    New C99 features that we cannot use yet:
 
    . %z and %zu as a printf() argument for a size_t (the %z being for

Re: [PATCH] strbuf: add compound literal test balloon

[Phillip Wood]

On 15/07/2025 17:24, Junio C Hamano wrote:
> 
> That sounds good.  I was wondering if it is easier to keep track of
> things to add a new section to the CodingGuildlines document,
> perhaps like this?

That's a good idea

Thanks

Phillip

>   Documentation/CodingGuidelines | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git c/Documentation/CodingGuidelines w/Documentation/CodingGuidelines
> index 6350949f2e..dd3dbb9c57 100644
> --- c/Documentation/CodingGuidelines
> +++ w/Documentation/CodingGuidelines
> @@ -298,6 +298,14 @@ For C programs:
>      . since late 2021 with 44ba10d6, we have had variables declared in
>        the for loop "for (int i = 0; i < 10; i++)".
>   
> +   C99 features we have test balloons for:
> +
> +   . since late 2024 with v2.48.0-rc0~20, we have test balloons for
> +     compound literal syntax, e.g., (struct foo){ .member = value };
> +     our hope is that no platforms we care about have trouble using
> +     them, and officially adopt its wider use in mid 2026.  Do not add
> +     more use of the syntax until that happens.
> +
>      New C99 features that we cannot use yet:
>   
>      . %z and %zu as a printf() argument for a size_t (the %z being for

Re: [PATCH] strbuf: add compound literal test balloon

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> On 15/07/2025 17:24, Junio C Hamano wrote:
>> That sounds good.  I was wondering if it is easier to keep track of
>> things to add a new section to the CodingGuildlines document,
>> perhaps like this?
>
> That's a good idea
>
> Thanks
>
> Phillip

I guess I dropped the ball after this exchange.  So our tentative
conclusion was to drop your new test balloon, and replace it with
the documentation patch you are responding to, which I obviously
am OK with.

Let me make it happen.

Thanks.


>
>>   Documentation/CodingGuidelines | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>> diff --git c/Documentation/CodingGuidelines
>> w/Documentation/CodingGuidelines
>> index 6350949f2e..dd3dbb9c57 100644
>> --- c/Documentation/CodingGuidelines
>> +++ w/Documentation/CodingGuidelines
>> @@ -298,6 +298,14 @@ For C programs:
>>      . since late 2021 with 44ba10d6, we have had variables declared in
>>        the for loop "for (int i = 0; i < 10; i++)".
>>   +   C99 features we have test balloons for:
>> +
>> +   . since late 2024 with v2.48.0-rc0~20, we have test balloons for
>> +     compound literal syntax, e.g., (struct foo){ .member = value };
>> +     our hope is that no platforms we care about have trouble using
>> +     them, and officially adopt its wider use in mid 2026.  Do not add
>> +     more use of the syntax until that happens.
>> +
>>      New C99 features that we cannot use yet:
>>        . %z and %zu as a printf() argument for a size_t (the %z
>> being for

[PATCH] CodingGuidelines: document test balloons in flight

[Junio C Hamano]

Due to portability concerns, we do not blindly say "It is in [[this
standard]], so we will make liberal use of it" for many features,
and use of C99 language features follow this same principle.  When
we contemplate adopting a language feature that we haven't used in
our codebase, we typically first raise a test balloon, which

 - is a piece of code that exercises the language feature we are
   trying to see if it is OK to adopt

 - is in a small section of code that we know everybody who cares
   about having a working Git must be compiling

 - is in a fairly stable part of the code, to allow reverting it
   easily if some platforms do not understand it yet.

After a few years, with no breakage report from the community, we'd
declare that the feature is now safe to use in our codebase.  Before
that, we forbid the use of the language construct except for the
designated test balloon code site.

The CodingGuidelines document lists these selected features that we
already have determined that they are safe, and also those features
that we know some platforms had trouble with.

Let's also start listing ongoing test balloons and expected timeline
for adoption.

Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/CodingGuidelines | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
index c1046abfb7..0776d15a95 100644
--- a/Documentation/CodingGuidelines
+++ b/Documentation/CodingGuidelines
@@ -298,6 +298,14 @@ For C programs:
    . since late 2021 with 44ba10d6, we have had variables declared in
      the for loop "for (int i = 0; i < 10; i++)".
 
+   C99 features we have test balloons for:
+
+   . since late 2024 with v2.48.0-rc0~20, we have test balloons for
+     compound literal syntax, e.g., (struct foo){ .member = value };
+     our hope is that no platforms we care about have trouble using
+     them, and officially adopt its wider use in mid 2026.  Do not add
+     more use of the syntax until that happens.
+
    New C99 features that we cannot use yet:
 
    . %z and %zu as a printf() argument for a size_t (the %z being for
-- 
2.50.1-521-gf11ee0bd80

Re: [PATCH] CodingGuidelines: document test balloons in flight

[Patrick Steinhardt]

On Wed, Jul 23, 2025 at 12:31:26PM -0700, Junio C Hamano wrote:
> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
> index c1046abfb7..0776d15a95 100644
> --- a/Documentation/CodingGuidelines
> +++ b/Documentation/CodingGuidelines
> @@ -298,6 +298,14 @@ For C programs:
>     . since late 2021 with 44ba10d6, we have had variables declared in
>       the for loop "for (int i = 0; i < 10; i++)".
>  
> +   C99 features we have test balloons for:
> +
> +   . since late 2024 with v2.48.0-rc0~20, we have test balloons for
> +     compound literal syntax, e.g., (struct foo){ .member = value };
> +     our hope is that no platforms we care about have trouble using
> +     them, and officially adopt its wider use in mid 2026.  Do not add
> +     more use of the syntax until that happens.

Nice. I like that we now have an explicit deadline for people to
complain about this feature not being supported on their platform.

So this looks good to me, thanks!

Patrick

Re: [PATCH] CodingGuidelines: document test balloons in flight

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> On Wed, Jul 23, 2025 at 12:31:26PM -0700, Junio C Hamano wrote:
>> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
>> index c1046abfb7..0776d15a95 100644
>> --- a/Documentation/CodingGuidelines
>> +++ b/Documentation/CodingGuidelines
>> @@ -298,6 +298,14 @@ For C programs:
>>     . since late 2021 with 44ba10d6, we have had variables declared in
>>       the for loop "for (int i = 0; i < 10; i++)".
>>  
>> +   C99 features we have test balloons for:
>> +
>> +   . since late 2024 with v2.48.0-rc0~20, we have test balloons for
>> +     compound literal syntax, e.g., (struct foo){ .member = value };
>> +     our hope is that no platforms we care about have trouble using
>> +     them, and officially adopt its wider use in mid 2026.  Do not add
>> +     more use of the syntax until that happens.
>
> Nice. I like that we now have an explicit deadline for people to
> complain about this feature not being supported on their platform.

I do not think the firm deadline has much practical effect.  Test
balloons are designed to be placed in a stable and non-optional part
of the codebase that is exercised by everybody, so even if your
update cycle from your upstream is once a year, you'd have four or
five major releases to try building and noticing that your platform
is unhappy about them.

So the only effect it would have is to smoke out truly slow platform
maintainers; if their users are happy enough with such slow upgrade,
they have lived and they can live with versions of Git that are
years stale that we no longer care about.

Re: [PATCH] CodingGuidelines: document test balloons in flight

[Collin Funk]

Junio C Hamano <gitster@pobox.com> writes:

> Patrick Steinhardt <ps@pks.im> writes:
>
>> On Wed, Jul 23, 2025 at 12:31:26PM -0700, Junio C Hamano wrote:
>>> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
>>> index c1046abfb7..0776d15a95 100644
>>> --- a/Documentation/CodingGuidelines
>>> +++ b/Documentation/CodingGuidelines
>>> @@ -298,6 +298,14 @@ For C programs:
>>>     . since late 2021 with 44ba10d6, we have had variables declared in
>>>       the for loop "for (int i = 0; i < 10; i++)".
>>>  
>>> +   C99 features we have test balloons for:
>>> +
>>> +   . since late 2024 with v2.48.0-rc0~20, we have test balloons for
>>> +     compound literal syntax, e.g., (struct foo){ .member = value };
>>> +     our hope is that no platforms we care about have trouble using
>>> +     them, and officially adopt its wider use in mid 2026.  Do not add
>>> +     more use of the syntax until that happens.
>>
>> Nice. I like that we now have an explicit deadline for people to
>> complain about this feature not being supported on their platform.
>
> I do not think the firm deadline has much practical effect.  Test
> balloons are designed to be placed in a stable and non-optional part
> of the codebase that is exercised by everybody, so even if your
> update cycle from your upstream is once a year, you'd have four or
> five major releases to try building and noticing that your platform
> is unhappy about them.
>
> So the only effect it would have is to smoke out truly slow platform
> maintainers; if their users are happy enough with such slow upgrade,
> they have lived and they can live with versions of Git that are
> years stale that we no longer care about.


For what it is worth, Gnulib and threfore Coreutils, among others, began
using compound literals in 2017 and it seems to have not caused any
problems [1]. Even 'pcc' supports them.

Collin

[1] https://github.com/coreutils/gnulib/commit/3a8af1e38bc026a9efb3b47c4686e4e54b633436

Re: [PATCH] CodingGuidelines: document test balloons in flight

[Junio C Hamano]

Collin Funk <collin.funk1@gmail.com> writes:

>> So the only effect it would have is to smoke out truly slow platform
>> maintainers; if their users are happy enough with such slow upgrade,
>> they have lived and they can live with versions of Git that are
>> years stale that we no longer care about.
>
>
> For what it is worth, Gnulib and threfore Coreutils, among others, began
> using compound literals in 2017 and it seems to have not caused any
> problems [1]. Even 'pcc' supports them.

Thanks.  Our worry however includes platforms outside the Open
Source and/or Free Software ecosystem, which would not be helped
very much.

Re: [PATCH] CodingGuidelines: document test balloons in flight

[Phillip Wood]

Hi Junio

On 23/07/2025 20:31, Junio C Hamano wrote:
> Due to portability concerns, we do not blindly say "It is in [[this
> standard]], so we will make liberal use of it" for many features,
> and use of C99 language features follow this same principle.  When
> we contemplate adopting a language feature that we haven't used in
> our codebase, we typically first raise a test balloon, which
> 
>   - is a piece of code that exercises the language feature we are
>     trying to see if it is OK to adopt
> 
>   - is in a small section of code that we know everybody who cares
>     about having a working Git must be compiling
> 
>   - is in a fairly stable part of the code, to allow reverting it
>     easily if some platforms do not understand it yet.
> 
> After a few years, with no breakage report from the community, we'd
> declare that the feature is now safe to use in our codebase.  Before
> that, we forbid the use of the language construct except for the
> designated test balloon code site.
> 
> The CodingGuidelines document lists these selected features that we
> already have determined that they are safe, and also those features
> that we know some platforms had trouble with.
> 
> Let's also start listing ongoing test balloons and expected timeline
> for adoption.
> 
> Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>

I'm not sure what I've done to deserve that - it was Patrick that 
pointed out the test balloon already existed. The patch and the commit 
message look good to me.

Thanks

Phillip

> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>   Documentation/CodingGuidelines | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
> index c1046abfb7..0776d15a95 100644
> --- a/Documentation/CodingGuidelines
> +++ b/Documentation/CodingGuidelines
> @@ -298,6 +298,14 @@ For C programs:
>      . since late 2021 with 44ba10d6, we have had variables declared in
>        the for loop "for (int i = 0; i < 10; i++)".
>   
> +   C99 features we have test balloons for:
> +
> +   . since late 2024 with v2.48.0-rc0~20, we have test balloons for
> +     compound literal syntax, e.g., (struct foo){ .member = value };
> +     our hope is that no platforms we care about have trouble using
> +     them, and officially adopt its wider use in mid 2026.  Do not add
> +     more use of the syntax until that happens.
> +
>      New C99 features that we cannot use yet:
>   
>      . %z and %zu as a printf() argument for a size_t (the %z being for

Re: [PATCH] CodingGuidelines: document test balloons in flight

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> Hi Junio
>
> On 23/07/2025 20:31, Junio C Hamano wrote:
>> Due to portability concerns, we do not blindly say "It is in [[this
>> standard]], so we will make liberal use of it" for many features,
>> and use of C99 language features follow this same principle.  When
>> we contemplate adopting a language feature that we haven't used in
>> our codebase, we typically first raise a test balloon, which
>>   - is a piece of code that exercises the language feature we are
>>     trying to see if it is OK to adopt
>>   - is in a small section of code that we know everybody who cares
>>     about having a working Git must be compiling
>>   - is in a fairly stable part of the code, to allow reverting it
>>     easily if some platforms do not understand it yet.
>> After a few years, with no breakage report from the community, we'd
>> declare that the feature is now safe to use in our codebase.  Before
>> that, we forbid the use of the language construct except for the
>> designated test balloon code site.
>> The CodingGuidelines document lists these selected features that we
>> already have determined that they are safe, and also those features
>> that we know some platforms had trouble with.
>> Let's also start listing ongoing test balloons and expected timeline
>> for adoption.
>> Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> I'm not sure what I've done to deserve that - it was Patrick that
> pointed out the test balloon already existed. The patch and the commit
> message look good to me.

Let me assign equal credit to Patrick.  Thanks.

>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> ---
>>   Documentation/CodingGuidelines | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>> diff --git a/Documentation/CodingGuidelines
>> b/Documentation/CodingGuidelines
>> index c1046abfb7..0776d15a95 100644
>> --- a/Documentation/CodingGuidelines
>> +++ b/Documentation/CodingGuidelines
>> @@ -298,6 +298,14 @@ For C programs:
>>      . since late 2021 with 44ba10d6, we have had variables declared in
>>        the for loop "for (int i = 0; i < 10; i++)".
>>   +   C99 features we have test balloons for:
>> +
>> +   . since late 2024 with v2.48.0-rc0~20, we have test balloons for
>> +     compound literal syntax, e.g., (struct foo){ .member = value };
>> +     our hope is that no platforms we care about have trouble using
>> +     them, and officially adopt its wider use in mid 2026.  Do not add
>> +     more use of the syntax until that happens.
>> +
>>      New C99 features that we cannot use yet:
>>        . %z and %zu as a printf() argument for a size_t (the %z
>> being for

Re: [PATCH] strbuf: add compound literal test balloon

[Phillip Wood]

On 14/07/2025 15:26, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> A C99 compound literal creates an unnamed object whose value is given by
>> an initializer list. This allows us to simplify code where we cannot use
>> a designated initalizer because the values of some members of the object
>> need to be calculated first. For example this code from builtin/rebase.c
>>
>> 	struct strbuf branch_reflog = STRBUF_INIT, head_reflog = STRBUF_INIT;
>> 	struct reset_head_opts ropts = { 0 };
>> 	int ret;
>>
>> 	strbuf_addf(&branch_reflog, "%s (finish): %s onto %s",
>> 		    opts->reflog_action,
>> 		    opts->head_name, oid_to_hex(&opts->onto->object.oid));
>> 	strbuf_addf(&head_reflog, "%s (finish): returning to %s",
>> 		    opts->reflog_action, opts->head_name);
>> 	ropts.branch = opts->head_name;
>> 	ropts.flags = RESET_HEAD_REFS_ONLY;
>> 	ropts.branch_msg = branch_reflog.buf;
>> 	ropts.head_msg = head_reflog.buf;
>> 	ret = reset_head(the_repository, &ropts);
>>
>> can be be simplified to
>>
>> 	struct strbuf branch_reflog = STRBUF_INIT, head_reflog = STRBUF_INIT;
>> 	int ret;
>>
>> 	strbuf_addf(&branch_reflog, "%s (finish): %s onto %s",
>> 		    opts->reflog_action,
>> 		    opts->head_name, oid_to_hex(&opts->onto->object.oid));
>> 	strbuf_addf(&head_reflog, "%s (finish): returning to %s",
>> 		    opts->reflog_action, opts->head_name);
>>          ret = reset_head(the_repository, &(struct reset_head_opts) {
>>                  .branch = opts->head_name,
>>          	.flags = RESET_HEAD_REFS_ONLY,
>>          	.branch_msg = branch_reflog.buf,
>>          	.head_msg = head_reflog.buf,
>>          });
>>
> > One thing the above rewrite did is to make it clear to readers that
> the struct instance is used just once and then immediately got
> discarded.  As long as the object that gets passed this way does not
> hold resources that need to be discarded itself (in other words,
> does not require a call to reset_head_opts_release()), it makes the
> code easier to follow.

That's a good point - this example would not work if reset_head_opts() 
took ownership of `branch_msg` and `head_msg`.

> But once the struct gains members that need to be released, I am not
> sure if this construct does not make it harder to spot leaks.
> Somebody who adds a member to _release() to the struct presumably
> audits and find all places that need to call _release(), but among
> them they find this place---now what?  They need to first convert it
> to the old fashioned way and then call _release() after the
> reset_head() call returns, I guess.
Another possibility is to do something like

	struct reset_head_opts ropts;

	/* ... */

	ropts = (struct replay_head_opts) {
		...
	};

	ret = reset_head(the_repository, &ropts);

	reset_head_opts_release(&ropts);

which initializes all the members of `ropts` in one place though I'm not 
sure if it is really better in practice.
> I am not arguing against all uses of literals---I am just
> anticipating future fallouts of encouraging overuse of this pattern,
> and preparing what we would say when somebody adds a new use to
> inappropriate places.  Simple cases like the initialier should be
> fine.

Yes, we'd want to be careful where we use them. I like the way we use 
designated initializers and this gives us the opportunity to have a 
similar style of initialization in a few more places.

Thanks

Phillip