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

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

[Ramsay Jones]

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

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

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

[Kristoffer Haugsbakk]

On Sun, May 3, 2026, at 17:16, Ramsay Jones wrote:
> 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!

>
>  builtin/name-rev.c | 2 ++
>  1 file changed, 2 insertions(+)
>
>[snip]

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

[Junio C Hamano]

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?

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

[Kristoffer Haugsbakk]

Hi Junio

On Mon, May 4, 2026, at 03:13, Junio C Hamano wrote:
> 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.

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

[Ramsay Jones]

On 04/05/2026 2:13 am, Junio C Hamano wrote:
> 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 ...

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

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

[Kristoffer Haugsbakk]

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
    CFLAGS+=-ggdb3
    USE_ASCIIDOCTOR=true

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

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

[Ramsay Jones]

On 04/05/2026 10:56 pm, Kristoffer Haugsbakk wrote:
> 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).

>     CFLAGS+=-ggdb3
>     USE_ASCIIDOCTOR=true
> 
> I switched to the whole config.mak.dev enchilada and now it fails
> as it should.
> 

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

[Kristoffer Haugsbakk]

On Tue, May 5, 2026, at 02:41, Ramsay Jones wrote:
> 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]