[PATCH] fix -Wmaybe-uninitialized with -Og

[PATCH] fix -Wmaybe-uninitialized with -Og

[Denton Liu]

When building with -Og on gcc 15.1.1, the build produces two warnings.
Even though in practice, these codepaths can't actually be hit while the
variables are uninitialized, satisfy the compiler by initializing the
variables.

This also acts as defensive programming since these codepaths are a
little bit spaghetti. If someone in the future makes a mistake and
causes the branch with the uninitialized variable to be hit, at least we
won't experience undefined behaviour.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 builtin/remote.c         | 2 +-
 t/unit-tests/clar/clar.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/remote.c b/builtin/remote.c
index 5dd6cbbaee..cc462677e1 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -1463,7 +1463,7 @@ static int set_head(int argc, const char **argv, const char *prefix,
 		b_local_head = STRBUF_INIT;
 	char *head_name = NULL;
 	struct ref_store *refs = get_main_ref_store(the_repository);
-	struct remote *remote;
+	struct remote *remote = NULL;
 
 	struct option options[] = {
 		OPT_BOOL('a', "auto", &opt_a,
diff --git a/t/unit-tests/clar/clar.c b/t/unit-tests/clar/clar.c
index d54e455367..03a3aa8e87 100644
--- a/t/unit-tests/clar/clar.c
+++ b/t/unit-tests/clar/clar.c
@@ -350,7 +350,7 @@ static void
 clar_run_suite(const struct clar_suite *suite, const char *filter)
 {
 	const struct clar_func *test = suite->tests;
-	size_t i, matchlen;
+	size_t i, matchlen = 0;
 	struct clar_report *report;
 	int exact = 0;
 
-- 
2.50.1

Re: [PATCH] fix -Wmaybe-uninitialized with -Og

[Jeff King]

On Mon, Aug 04, 2025 at 03:07:15AM -0700, Denton Liu wrote:

> When building with -Og on gcc 15.1.1, the build produces two warnings.
> Even though in practice, these codepaths can't actually be hit while the
> variables are uninitialized, satisfy the compiler by initializing the
> variables.

I see these on gcc 14.2.0, too.

> diff --git a/builtin/remote.c b/builtin/remote.c
> index 5dd6cbbaee..cc462677e1 100644
> --- a/builtin/remote.c
> +++ b/builtin/remote.c
> @@ -1463,7 +1463,7 @@ static int set_head(int argc, const char **argv, const char *prefix,
>  		b_local_head = STRBUF_INIT;
>  	char *head_name = NULL;
>  	struct ref_store *refs = get_main_ref_store(the_repository);
> -	struct remote *remote;
> +	struct remote *remote = NULL;
>  
>  	struct option options[] = {
>  		OPT_BOOL('a', "auto", &opt_a,

I think you're right that this can't be triggered, but maybe a bit of
reordering would make that more obvious both to the compiler and to
humans. The issue is that we do this:

          if (argc) {
                  strbuf_addf(&b_head, "refs/remotes/%s/HEAD", argv[0]);
                  remote = remote_get(argv[0]);
          }

and then follow it up with various sanity checks about how the value of
argc. But we always require that argc is at least 1 or we bail with a
usage message.

This comes from 012bc566ba (remote set-head: set followRemoteHEAD to
"warn" if "always", 2024-12-05). If we revert out that change and
instead add it later, like so:

diff --git a/builtin/remote.c b/builtin/remote.c
index 5dd6cbbaee..8ba02d1854 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -1474,10 +1474,8 @@ static int set_head(int argc, const char **argv, const char *prefix,
 	};
 	argc = parse_options(argc, argv, prefix, options,
 			     builtin_remote_sethead_usage, 0);
-	if (argc) {
+	if (argc)
 		strbuf_addf(&b_head, "refs/remotes/%s/HEAD", argv[0]);
-		remote = remote_get(argv[0]);
-	}
 
 	if (!opt_a && !opt_d && argc == 2) {
 		head_name = xstrdup(argv[1]);
@@ -1501,6 +1499,8 @@ static int set_head(int argc, const char **argv, const char *prefix,
 	} else
 		usage_with_options(builtin_remote_sethead_usage, options);
 
+	remote = remote_get(argv[0]);
+
 	if (!head_name)
 		goto cleanup;
 	strbuf_addf(&b_remote_head, "refs/remotes/%s/%s", argv[0], head_name);

then the compiler is happy. Though I still find the whole b_head thing
to be total spaghetti (it must be set before our argc if/else cascade
because deletion mode expects it there, but we can't just set it inside
there because other modes expect it to be set later on).

So I wonder if this would be much more obvious (again, to both humans
and compilers):

diff --git a/builtin/remote.c b/builtin/remote.c
index 5dd6cbbaee..f0e49a5681 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -1474,10 +1474,13 @@ static int set_head(int argc, const char **argv, const char *prefix,
 	};
 	argc = parse_options(argc, argv, prefix, options,
 			     builtin_remote_sethead_usage, 0);
-	if (argc) {
-		strbuf_addf(&b_head, "refs/remotes/%s/HEAD", argv[0]);
-		remote = remote_get(argv[0]);
-	}
+
+	/* All modes require at least a remote name. */
+	if (!argc)
+		usage_with_options(builtin_remote_sethead_usage, options);
+
+	strbuf_addf(&b_head, "refs/remotes/%s/HEAD", argv[0]);
+	remote = remote_get(argv[0]);
 
 	if (!opt_a && !opt_d && argc == 2) {
 		head_name = xstrdup(argv[1]);

> diff --git a/t/unit-tests/clar/clar.c b/t/unit-tests/clar/clar.c
> index d54e455367..03a3aa8e87 100644
> --- a/t/unit-tests/clar/clar.c
> +++ b/t/unit-tests/clar/clar.c
> @@ -350,7 +350,7 @@ static void
>  clar_run_suite(const struct clar_suite *suite, const char *filter)
>  {
>  	const struct clar_func *test = suite->tests;
> -	size_t i, matchlen;
> +	size_t i, matchlen = 0;
>  	struct clar_report *report;
>  	int exact = 0;

For this one I don't see any real alternative. We set matchlen if and
only if "filter" is set:

  if (filter) {
	...
	matchlen = strlen(filter);
	...
  }

and the line it complains about is:

  if (filter && strncmp(test[i].name, filter, matchlen))

so the short-circuit protects us. So it's only a problem if "filter"
changes between those two lines. It is in a loop, but I don't see how
"filter" could ever be changed within the loop. So I'm not sure if this
is just a compiler bug, or if there's some really subtle alias thing
where filter _could_ be changed in a sub-function or something.

At any rate I agree that "0" is the appropriate value here, and
assigning it to shut up the compiler is the best approach.

-Peff

Re: [PATCH] fix -Wmaybe-uninitialized with -Og

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> So I wonder if this would be much more obvious (again, to both humans
> and compilers):
>
> diff --git a/builtin/remote.c b/builtin/remote.c
> index 5dd6cbbaee..f0e49a5681 100644
> --- a/builtin/remote.c
> +++ b/builtin/remote.c
> @@ -1474,10 +1474,13 @@ static int set_head(int argc, const char **argv, const char *prefix,
>  	};
>  	argc = parse_options(argc, argv, prefix, options,
>  			     builtin_remote_sethead_usage, 0);
> -	if (argc) {
> -		strbuf_addf(&b_head, "refs/remotes/%s/HEAD", argv[0]);
> -		remote = remote_get(argv[0]);
> -	}
> +
> +	/* All modes require at least a remote name. */
> +	if (!argc)
> +		usage_with_options(builtin_remote_sethead_usage, options);
> +
> +	strbuf_addf(&b_head, "refs/remotes/%s/HEAD", argv[0]);
> +	remote = remote_get(argv[0]);

I do not know about compilers, but a sample of one, to this human it
is more obvious ;-).

> and the line it complains about is:
>
>   if (filter && strncmp(test[i].name, filter, matchlen))
> ...
> At any rate I agree that "0" is the appropriate value here, and
> assigning it to shut up the compiler is the best approach.

... simply because we know the value in matchlen does not matter
when filter is NULL?  I think that would work and I would be happy
with a less noisy compilation.

But any other value like 99 would equally well work, which is a bit
disturbing ;-).

Thanks.

Re: [PATCH] fix -Wmaybe-uninitialized with -Og

[Jeff King]

On Mon, Aug 04, 2025 at 06:46:50AM -0700, Junio C Hamano wrote:

> > +	/* All modes require at least a remote name. */
> > +	if (!argc)
> > +		usage_with_options(builtin_remote_sethead_usage, options);
> > +
> > +	strbuf_addf(&b_head, "refs/remotes/%s/HEAD", argv[0]);
> > +	remote = remote_get(argv[0]);
> 
> I do not know about compilers, but a sample of one, to this human it
> is more obvious ;-).

OK, cleaned up patch is below. Hopefully I am not stealing Denton's
thunder, but this seemed trivial enough that I wanted to get it off my
plate and never think of it again. ;)

> > and the line it complains about is:
> >
> >   if (filter && strncmp(test[i].name, filter, matchlen))
> > ...
> > At any rate I agree that "0" is the appropriate value here, and
> > assigning it to shut up the compiler is the best approach.
> 
> ... simply because we know the value in matchlen does not matter
> when filter is NULL?  I think that would work and I would be happy
> with a less noisy compilation.
> 
> But any other value like 99 would equally well work, which is a bit
> disturbing ;-).

It's true that any value would work with the current code. But I think
"0" makes the most sense because it is counting bytes in "filter". If
"filter" is NULL, then we have zero matched bytes. So if anybody _did_
look at it, they'd hopefully do the right thing.

BTW, this clar code comes from libgit2. They may want to fix it
upstream, too. +cc Patrick.

-- >8 --
Subject: [PATCH] remote: bail early from set_head() if missing remote name

In "git remote set-head", we can take varying numbers of arguments
depending on whether we saw the "-d" or "-a" options. But the first
argument is always the remote name.

The current code is somewhat awkward in that it conditionally handles
the remote name up-front like this:

  if (argc)
     remote = ...from argv[0]...

and then only later decides to bail if we do not have the right number
of arguments for the options we saw.

This makes it hard to figure out if "remote" is always set when it needs
to be. Both for humans, but also for compilers; with -Og, gcc complains
that "remote" can be accessed without being initialized (although this
is not true, as we'd always die with a usage message in that case).

Let's instead enforce the presence of the remote argument up front,
which fixes the compiler warning and is easier to understand. It does
mean duplicating the code to print a usage message, but it's a single
line.

Noticed-by: Denton Liu <liu.denton@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/remote.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/builtin/remote.c b/builtin/remote.c
index 5dd6cbbaee..f0e49a5681 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -1474,10 +1474,13 @@ static int set_head(int argc, const char **argv, const char *prefix,
 	};
 	argc = parse_options(argc, argv, prefix, options,
 			     builtin_remote_sethead_usage, 0);
-	if (argc) {
-		strbuf_addf(&b_head, "refs/remotes/%s/HEAD", argv[0]);
-		remote = remote_get(argv[0]);
-	}
+
+	/* All modes require at least a remote name. */
+	if (!argc)
+		usage_with_options(builtin_remote_sethead_usage, options);
+
+	strbuf_addf(&b_head, "refs/remotes/%s/HEAD", argv[0]);
+	remote = remote_get(argv[0]);
 
 	if (!opt_a && !opt_d && argc == 2) {
 		head_name = xstrdup(argv[1]);
-- 
2.50.1.786.g492fc26cdf

[PATCH v2 0/2] fix -Wmaybe-uninitialized with -Og

[Denton Liu]

When compiled with -Og, the build emits -Wmaybe-uninitialized. Fix
these.

Denton Liu (1):
  t/unit-tests/clar: fix -Wmaybe-uninitialized with -Og

Jeff King (1):
  remote: bail early from set_head() if missing remote name

 builtin/remote.c         | 11 +++++++----
 t/unit-tests/clar/clar.c |  2 +-
 2 files changed, 8 insertions(+), 5 deletions(-)

Range-diff against v1:
1:  d03308e947 ! 1:  458ec588b7 fix -Wmaybe-uninitialized with -Og
    @@
      ## Metadata ##
    -Author: Denton Liu <liu.denton@gmail.com>
    +Author: Jeff King <peff@peff.net>
     
      ## Commit message ##
    -    fix -Wmaybe-uninitialized with -Og
    +    remote: bail early from set_head() if missing remote name
     
    -    When building with -Og on gcc 15.1.1, the build produces two warnings.
    -    Even though in practice, these codepaths can't actually be hit while the
    -    variables are uninitialized, satisfy the compiler by initializing the
    -    variables.
    +    In "git remote set-head", we can take varying numbers of arguments
    +    depending on whether we saw the "-d" or "-a" options. But the first
    +    argument is always the remote name.
     
    -    This also acts as defensive programming since these codepaths are a
    -    little bit spaghetti. If someone in the future makes a mistake and
    -    causes the branch with the uninitialized variable to be hit, at least we
    -    won't experience undefined behaviour.
    +    The current code is somewhat awkward in that it conditionally handles
    +    the remote name up-front like this:
    +
    +      if (argc)
    +         remote = ...from argv[0]...
    +
    +    and then only later decides to bail if we do not have the right number
    +    of arguments for the options we saw.
    +
    +    This makes it hard to figure out if "remote" is always set when it needs
    +    to be. Both for humans, but also for compilers; with -Og, gcc complains
    +    that "remote" can be accessed without being initialized (although this
    +    is not true, as we'd always die with a usage message in that case).
    +
    +    Let's instead enforce the presence of the remote argument up front,
    +    which fixes the compiler warning and is easier to understand. It does
    +    mean duplicating the code to print a usage message, but it's a single
    +    line.
    +
    +    Noticed-by: Denton Liu <liu.denton@gmail.com>
    +    Signed-off-by: Jeff King <peff@peff.net>
     
      ## builtin/remote.c ##
     @@ builtin/remote.c: static int set_head(int argc, const char **argv, const char *prefix,
    - 		b_local_head = STRBUF_INIT;
    - 	char *head_name = NULL;
    - 	struct ref_store *refs = get_main_ref_store(the_repository);
    --	struct remote *remote;
    -+	struct remote *remote = NULL;
    - 
    - 	struct option options[] = {
    - 		OPT_BOOL('a', "auto", &opt_a,
    -
    - ## t/unit-tests/clar/clar.c ##
    -@@ t/unit-tests/clar/clar.c: static void
    - clar_run_suite(const struct clar_suite *suite, const char *filter)
    - {
    - 	const struct clar_func *test = suite->tests;
    --	size_t i, matchlen;
    -+	size_t i, matchlen = 0;
    - 	struct clar_report *report;
    - 	int exact = 0;
    + 	};
    + 	argc = parse_options(argc, argv, prefix, options,
    + 			     builtin_remote_sethead_usage, 0);
    +-	if (argc) {
    +-		strbuf_addf(&b_head, "refs/remotes/%s/HEAD", argv[0]);
    +-		remote = remote_get(argv[0]);
    +-	}
    ++
    ++	/* All modes require at least a remote name. */
    ++	if (!argc)
    ++		usage_with_options(builtin_remote_sethead_usage, options);
    ++
    ++	strbuf_addf(&b_head, "refs/remotes/%s/HEAD", argv[0]);
    ++	remote = remote_get(argv[0]);
      
    + 	if (!opt_a && !opt_d && argc == 2) {
    + 		head_name = xstrdup(argv[1]);
-:  ---------- > 2:  8ed0ac1409 t/unit-tests/clar: fix -Wmaybe-uninitialized with -Og
-- 
2.50.1

[PATCH v2 1/2] remote: bail early from set_head() if missing remote name

[Denton Liu]

From: Jeff King <peff@peff.net>

In "git remote set-head", we can take varying numbers of arguments
depending on whether we saw the "-d" or "-a" options. But the first
argument is always the remote name.

The current code is somewhat awkward in that it conditionally handles
the remote name up-front like this:

  if (argc)
     remote = ...from argv[0]...

and then only later decides to bail if we do not have the right number
of arguments for the options we saw.

This makes it hard to figure out if "remote" is always set when it needs
to be. Both for humans, but also for compilers; with -Og, gcc complains
that "remote" can be accessed without being initialized (although this
is not true, as we'd always die with a usage message in that case).

Let's instead enforce the presence of the remote argument up front,
which fixes the compiler warning and is easier to understand. It does
mean duplicating the code to print a usage message, but it's a single
line.

Noticed-by: Denton Liu <liu.denton@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
Tested-by: Denton Liu <liu.denton@gmail.com>
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
Thanks Peff for writing this patch up

 builtin/remote.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/builtin/remote.c b/builtin/remote.c
index 5dd6cbbaee..f0e49a5681 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -1474,10 +1474,13 @@ static int set_head(int argc, const char **argv, const char *prefix,
 	};
 	argc = parse_options(argc, argv, prefix, options,
 			     builtin_remote_sethead_usage, 0);
-	if (argc) {
-		strbuf_addf(&b_head, "refs/remotes/%s/HEAD", argv[0]);
-		remote = remote_get(argv[0]);
-	}
+
+	/* All modes require at least a remote name. */
+	if (!argc)
+		usage_with_options(builtin_remote_sethead_usage, options);
+
+	strbuf_addf(&b_head, "refs/remotes/%s/HEAD", argv[0]);
+	remote = remote_get(argv[0]);
 
 	if (!opt_a && !opt_d && argc == 2) {
 		head_name = xstrdup(argv[1]);
-- 
2.50.1

[PATCH v2 2/2] t/unit-tests/clar: fix -Wmaybe-uninitialized with -Og

[Denton Liu]

When building with -Og on gcc 15.1.1, the build produces a warning. In
practice, though, this cannot be hit because `exact` acts as a guard and
that variable can only be set after `matchlen` is already initialized

Assign a default value to `matchlen` so that the warning is silenced.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/unit-tests/clar/clar.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/unit-tests/clar/clar.c b/t/unit-tests/clar/clar.c
index d54e455367..03a3aa8e87 100644
--- a/t/unit-tests/clar/clar.c
+++ b/t/unit-tests/clar/clar.c
@@ -350,7 +350,7 @@ static void
 clar_run_suite(const struct clar_suite *suite, const char *filter)
 {
 	const struct clar_func *test = suite->tests;
-	size_t i, matchlen;
+	size_t i, matchlen = 0;
 	struct clar_report *report;
 	int exact = 0;
 
-- 
2.50.1

Re: [PATCH v2 2/2] t/unit-tests/clar: fix -Wmaybe-uninitialized with -Og

[Patrick Steinhardt]

On Mon, Aug 04, 2025 at 10:31:16PM -0700, Denton Liu wrote:
> When building with -Og on gcc 15.1.1, the build produces a warning. In
> practice, though, this cannot be hit because `exact` acts as a guard and
> that variable can only be set after `matchlen` is already initialized
> 
> Assign a default value to `matchlen` so that the warning is silenced.

Would you mind creating a PR against upstream [1] so that we also have it
over there? Thanks!

Patrick

[1]: https://github.com/clar-test/clar

Re: [PATCH v2 2/2] t/unit-tests/clar: fix -Wmaybe-uninitialized with -Og

[Denton Liu]

On Fri, Aug 08, 2025 at 07:48:10AM +0200, Patrick Steinhardt wrote:
> On Mon, Aug 04, 2025 at 10:31:16PM -0700, Denton Liu wrote:
> > When building with -Og on gcc 15.1.1, the build produces a warning. In
> > practice, though, this cannot be hit because `exact` acts as a guard and
> > that variable can only be set after `matchlen` is already initialized
> > 
> > Assign a default value to `matchlen` so that the warning is silenced.
> 
> Would you mind creating a PR against upstream [1] so that we also have it
> over there? Thanks!

Good idea. PR over at [0]

-Denton

[0]: https://github.com/clar-test/clar/pull/119

Re: [PATCH v2 2/2] t/unit-tests/clar: fix -Wmaybe-uninitialized with -Og

[Patrick Steinhardt]

On Fri, Aug 08, 2025 at 12:55:24AM -0700, Denton Liu wrote:
> On Fri, Aug 08, 2025 at 07:48:10AM +0200, Patrick Steinhardt wrote:
> > On Mon, Aug 04, 2025 at 10:31:16PM -0700, Denton Liu wrote:
> > > When building with -Og on gcc 15.1.1, the build produces a warning. In
> > > practice, though, this cannot be hit because `exact` acts as a guard and
> > > that variable can only be set after `matchlen` is already initialized
> > > 
> > > Assign a default value to `matchlen` so that the warning is silenced.
> > 
> > Would you mind creating a PR against upstream [1] so that we also have it
> > over there? Thanks!
> 
> Good idea. PR over at [0]
> 
> -Denton
> 
> [0]: https://github.com/clar-test/clar/pull/119

Thanks, approved and merged now.

Patrick