Volume XXII, number 280Wednesday, October 7, 2026Latest message 58 minutes ago

The Git List

News and archive of git@vger.kernel.org, since April 2005

patchname-rev: fix an 'may be used uninitialized' error

8 messages between May 3, 2026 and May 5, 2026, from Ramsay Jones, Kristoffer Haugsbakk, Junio C Hamano.

Plain Markdown or JSON for tools and agents. Diffs are folded; open one to read it.

Ramsay JonesMay 3, 2026, 15:16 UTC on lore
Today's seen branch fails to build (with DEVELOPER=1), like so:
      CC builtin/name-rev.o
  builtin/name-rev.c: In function ‘cmd_format_rev’:
  builtin/name-rev.c:885:28: error: ‘commit’ may be used uninitialized [-Werror=maybe-uninitialized]
    885 |                         if (!commit) {
        |                            ^
  builtin/name-rev.c:867:40: note: ‘commit’ was declared here
    867 |                         struct commit *commit;
        |                                        ^~~~~~
  cc1: all warnings being treated as errors
  make: *** [Makefile:2932: builtin/name-rev.o] Error 1

This can be fixed in several ways; initialise the 'commit' variable to NULL (on line 867), initialise 'commit' to NULL on the line before the conditional on line 883, or (as I chose here) initialise the 'commit' variable in an else arm of the conditional.

Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
---
Hi Kristoffer,

I wrote this patch yesterday, just before I had to go out, and didn't get around to sending it to the list. Today, the problem has gone away ... (along with the 'kh/name-rev-custom-format' branch)!

Assuming you will be sending a new version soon, ... could you please squash this (or similar) into the patch corresponding to commit 5903855b1c ("format-rev: introduce builtin for on-demand pretty formatting", 2026-04-29).

Note that I don't think this particular fix is better than any other, it was just that my cursor was on that line in vim ... :)

ATB, Ramsay Jones

 builtin/name-rev.c | 2 ++
 1 file changed, 2 insertions(+)
Show changes to builtin/name-rev.c +2 −0
diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index b941e93834..5b7f7a00e5 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -882,6 +882,8 @@ int cmd_format_rev(int argc,
 			peeled = deref_tag(the_repository, object, scratch_buf.buf, 0);
 			if (peeled && peeled->type == OBJ_COMMIT)
 				commit = (struct commit *)peeled;
+			else
+				commit = NULL;
 			if (!commit) {
 				fprintf(stderr, "Could not get commit for %s. Skipping.\n",
 					*argv);

-- 
2.54.0
Kristoffer HaugsbakkMay 3, 2026, 18:52 UTC in reply to Ramsay Jones on lore

Re: [PATCH] name-rev: fix an 'may be used uninitialized' error

On Sun, May 3, 2026, at 17:16, Ramsay Jones wrote:
Show 37 quoted lines
> Today's seen branch fails to build (with DEVELOPER=1), like so:
>
>       CC builtin/name-rev.o
>   builtin/name-rev.c: In function ‘cmd_format_rev’:
>   builtin/name-rev.c:885:28: error: ‘commit’ may be used uninitialized
> [-Werror=maybe-uninitialized]
>     885 |                         if (!commit) {
>         |                            ^
>   builtin/name-rev.c:867:40: note: ‘commit’ was declared here
>     867 |                         struct commit *commit;
>         |                                        ^~~~~~
>   cc1: all warnings being treated as errors
>   make: *** [Makefile:2932: builtin/name-rev.o] Error 1
>
> This can be fixed in several ways; initialise the 'commit' variable to
> NULL (on line 867), initialise 'commit' to NULL on the line before the
> conditional on line 883, or (as I chose here) initialise the 'commit'
> variable in an else arm of the conditional.
>
> Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
> ---
>
> Hi Kristoffer,
>
> I wrote this patch yesterday, just before I had to go out, and didn't
> get around to sending it to the list. Today, the problem has gone
> away ... (along with the 'kh/name-rev-custom-format' branch)!
>
> Assuming you will be sending a new version soon, ... could you please
> squash this (or similar) into the patch corresponding to commit 5903855b1c
> ("format-rev: introduce builtin for on-demand pretty formatting", 2026-04-29).
>
> Note that I don't think this particular fix is better than any other, it
> was just that my cursor was on that line in vim ... :)
>
> ATB,
> Ramsay Jones
I’ll incorporate it. Thank you!
Show 5 quoted lines
>
>  builtin/name-rev.c | 2 ++
>  1 file changed, 2 insertions(+)
>
>[snip]
Junio C HamanoMay 4, 2026, 01:13 UTC in reply to Ramsay Jones on lore

Re: [PATCH] name-rev: fix an 'may be used uninitialized' error

Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
Show 26 quoted lines
> Today's seen branch fails to build (with DEVELOPER=1), like so:
>
>       CC builtin/name-rev.o
>   builtin/name-rev.c: In function ‘cmd_format_rev’:
>   builtin/name-rev.c:885:28: error: ‘commit’ may be used uninitialized [-Werror=maybe-uninitialized]
>     885 |                         if (!commit) {
>         |                            ^
>   builtin/name-rev.c:867:40: note: ‘commit’ was declared here
>     867 |                         struct commit *commit;
>         |                                        ^~~~~~
>   cc1: all warnings being treated as errors
>   make: *** [Makefile:2932: builtin/name-rev.o] Error 1
> ...
> diff --git a/builtin/name-rev.c b/builtin/name-rev.c
> index b941e93834..5b7f7a00e5 100644
> --- a/builtin/name-rev.c
> +++ b/builtin/name-rev.c
> @@ -882,6 +882,8 @@ int cmd_format_rev(int argc,
>  			peeled = deref_tag(the_repository, object, scratch_buf.buf, 0);
>  			if (peeled && peeled->type == OBJ_COMMIT)
>  				commit = (struct commit *)peeled;
> +			else
> +				commit = NULL;
>  			if (!commit) {
>  				fprintf(stderr, "Could not get commit for %s. Skipping.\n",
>  					*argv);
Why not
			if (peeled && peeled->type == OBJ_COMMIT) {
				commit = (struct commit *)peeled;
			} else {
				fprintf(stderr, "... skipping ...");
				continue;
			}
			get_format_rev(commit, &format_pp, &scratch);
or even
			if (!peeled || peeled->type != OBJ_COMMIT) {
				fprintf(stderr, "... skipping ...");
				continue;
			}
			get_format_rev((struct commit *)peeled->type,
					&format_pp, &scratch);
and dropping the variable "struct commit *commit" altogether?
Kristoffer HaugsbakkMay 4, 2026, 08:55 UTC in reply to Junio C Hamano on lore

Re: [PATCH] name-rev: fix an 'may be used uninitialized' error

Hi Junio
On Mon, May 4, 2026, at 03:13, Junio C Hamano wrote:
Show 51 quoted lines
> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
>
>> Today's seen branch fails to build (with DEVELOPER=1), like so:
>>
>>       CC builtin/name-rev.o
>>   builtin/name-rev.c: In function ‘cmd_format_rev’:
>>   builtin/name-rev.c:885:28: error: ‘commit’ may be used uninitialized [-Werror=maybe-uninitialized]
>>     885 |                         if (!commit) {
>>         |                            ^
>>   builtin/name-rev.c:867:40: note: ‘commit’ was declared here
>>     867 |                         struct commit *commit;
>>         |                                        ^~~~~~
>>   cc1: all warnings being treated as errors
>>   make: *** [Makefile:2932: builtin/name-rev.o] Error 1
>> ...
>> diff --git a/builtin/name-rev.c b/builtin/name-rev.c
>> index b941e93834..5b7f7a00e5 100644
>> --- a/builtin/name-rev.c
>> +++ b/builtin/name-rev.c
>> @@ -882,6 +882,8 @@ int cmd_format_rev(int argc,
>>  			peeled = deref_tag(the_repository, object, scratch_buf.buf, 0);
>>  			if (peeled && peeled->type == OBJ_COMMIT)
>>  				commit = (struct commit *)peeled;
>> +			else
>> +				commit = NULL;
>>  			if (!commit) {
>>  				fprintf(stderr, "Could not get commit for %s. Skipping.\n",
>>  					*argv);
>
> Why not
>
> 			if (peeled && peeled->type == OBJ_COMMIT) {
> 				commit = (struct commit *)peeled;
> 			} else {
> 				fprintf(stderr, "... skipping ...");
> 				continue;
> 			}
>
> 			get_format_rev(commit, &format_pp, &scratch);
>
> or even
>
> 			if (!peeled || peeled->type != OBJ_COMMIT) {
> 				fprintf(stderr, "... skipping ...");
> 				continue;
> 			}
>
> 			get_format_rev((struct commit *)peeled->type,
> 					&format_pp, &scratch);
>
> and dropping the variable "struct commit *commit" altogether?

I see that you added this as one of two “SQUASH???” commits on your kh/name-rev-custom-format branch. I will squash both of them in for the next round.

Thanks to both of you.
Ramsay JonesMay 4, 2026, 20:26 UTC in reply to Junio C Hamano on lore

Re: [PATCH] name-rev: fix an 'may be used uninitialized' error

On 04/05/2026 2:13 am, Junio C Hamano wrote:
Show 30 quoted lines
> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
> 
>> Today's seen branch fails to build (with DEVELOPER=1), like so:
>>
>>       CC builtin/name-rev.o
>>   builtin/name-rev.c: In function ‘cmd_format_rev’:
>>   builtin/name-rev.c:885:28: error: ‘commit’ may be used uninitialized [-Werror=maybe-uninitialized]
>>     885 |                         if (!commit) {
>>         |                            ^
>>   builtin/name-rev.c:867:40: note: ‘commit’ was declared here
>>     867 |                         struct commit *commit;
>>         |                                        ^~~~~~
>>   cc1: all warnings being treated as errors
>>   make: *** [Makefile:2932: builtin/name-rev.o] Error 1
>> ...
>> diff --git a/builtin/name-rev.c b/builtin/name-rev.c
>> index b941e93834..5b7f7a00e5 100644
>> --- a/builtin/name-rev.c
>> +++ b/builtin/name-rev.c
>> @@ -882,6 +882,8 @@ int cmd_format_rev(int argc,
>>  			peeled = deref_tag(the_repository, object, scratch_buf.buf, 0);
>>  			if (peeled && peeled->type == OBJ_COMMIT)
>>  				commit = (struct commit *)peeled;
>> +			else
>> +				commit = NULL;
>>  			if (!commit) {
>>  				fprintf(stderr, "Could not get commit for %s. Skipping.\n",
>>  					*argv);
> 
> Why not

Heh, you noticed that I spent all of a few seconds writing this patch, just to get the branch to build, as I was in a rush to go out. I wasn't quick enough anyway, so I didn't send it until the next day. But, as I said in the patch, I wasn't pushing this patch as _the_ fix ...

Show 21 quoted lines
> 
> 			if (peeled && peeled->type == OBJ_COMMIT) {
> 				commit = (struct commit *)peeled;
> 			} else {
> 				fprintf(stderr, "... skipping ...");
> 				continue;
> 			}
> 
> 			get_format_rev(commit, &format_pp, &scratch);
> 
> or even
> 
> 			if (!peeled || peeled->type != OBJ_COMMIT) {
> 				fprintf(stderr, "... skipping ...");
> 				continue;
> 			}
> 
> 			get_format_rev((struct commit *)peeled->type,
> 					&format_pp, &scratch);
> 
> and dropping the variable "struct commit *commit" altogether?

Having now spent some time (well at least 30 seconds :) ) looking at the surrounding code, then your final suggestion looks really good to me! ;)

However, these 'maybe-uninitialized' errors (historically have been) somewhat sensitive to the level of optimization used in the compilation and even algo used by the compiler changing frequently from one version to the next ... So, I wasn't sure if Kristoffer was actually seeing the error or had the DEVELOPER variable set (which is why I mentioned it in passing!).

Thanks!

ATB, Ramsay Jones

Kristoffer HaugsbakkMay 4, 2026, 21:56 UTC in reply to Ramsay Jones on lore

Re: [PATCH] name-rev: fix an 'may be used uninitialized' error

On Mon, May 4, 2026, at 22:26, Ramsay Jones wrote:
Show 12 quoted lines
> On 04/05/2026 2:13 am, Junio C Hamano wrote:
>> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
>>>[snip]
>
> Having now spent some time (well at least 30 seconds :) ) looking at the
> surrounding code, then your final suggestion looks really good to me! ;)
>
> However, these 'maybe-uninitialized' errors (historically have been) somewhat
> sensitive to the level of optimization used in the compilation and even algo
> used by the compiler changing frequently from one version to the next ...
> So, I wasn't sure if Kristoffer was actually seeing the error or had the
> DEVELOPER variable set (which is why I mentioned it in passing!).
This is what I had when maybe-uninit. didn’t fail for me.
    $ cat config.mak
    DEVELOPER=1
    DEBUG=1
    CC = ccache gcc
    CFLAGS+=-O0
    CFLAGS+=-ggdb3
    USE_ASCIIDOCTOR=true

I switched to the whole config.mak.dev enchilada and now it fails as it should.

Ramsay JonesMay 5, 2026, 00:41 UTC in reply to Kristoffer Haugsbakk on lore

Re: [PATCH] name-rev: fix an 'may be used uninitialized' error

On 04/05/2026 10:56 pm, Kristoffer Haugsbakk wrote:
Show 21 quoted lines
> On Mon, May 4, 2026, at 22:26, Ramsay Jones wrote:
>> On 04/05/2026 2:13 am, Junio C Hamano wrote:
>>> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
>>>> [snip]
>>
>> Having now spent some time (well at least 30 seconds :) ) looking at the
>> surrounding code, then your final suggestion looks really good to me! ;)
>>
>> However, these 'maybe-uninitialized' errors (historically have been) somewhat
>> sensitive to the level of optimization used in the compilation and even algo
>> used by the compiler changing frequently from one version to the next ...
>> So, I wasn't sure if Kristoffer was actually seeing the error or had the
>> DEVELOPER variable set (which is why I mentioned it in passing!).
> 
> This is what I had when maybe-uninit. didn’t fail for me.
> 
>     $ cat config.mak
>     DEVELOPER=1
>     DEBUG=1
>     CC = ccache gcc
>     CFLAGS+=-O0

Ah, yes -O0 will disable the warning/error. Normally CFLAGS would be set to something like 'CFLAGS = -g -O2 -Wall'. (which still produces a binary you can reasonably use with gdb).

Show 6 quoted lines
>     CFLAGS+=-ggdb3
>     USE_ASCIIDOCTOR=true
> 
> I switched to the whole config.mak.dev enchilada and now it fails
> as it should.
>
Kristoffer HaugsbakkMay 5, 2026, 19:09 UTC in reply to Ramsay Jones on lore

Re: [PATCH] name-rev: fix an 'may be used uninitialized' error

On Tue, May 5, 2026, at 02:41, Ramsay Jones wrote:
Show 15 quoted lines
> On 04/05/2026 10:56 pm, Kristoffer Haugsbakk wrote:
>> On Mon, May 4, 2026, at 22:26, Ramsay Jones wrote:
>>>[snip]
>>
>> This is what I had when maybe-uninit. didn’t fail for me.
>>
>>     $ cat config.mak
>>     DEVELOPER=1
>>     DEBUG=1
>>     CC = ccache gcc
>>     CFLAGS+=-O0
>
> Ah, yes -O0 will disable the warning/error. Normally CFLAGS would be set to
> something like 'CFLAGS = -g -O2 -Wall'. (which still produces a binary you
> can reasonably use with gdb).
Thanks :)
>
>>     CFLAGS+=-ggdb3
>>     USE_ASCIIDOCTOR=true
>>[snip]

Back to recent threads