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

From: Jeff King <peff@peff.net>
Date: Aug 4, 2025, 13:19 UTC
Message-ID: <20250804131922.GB86602@coredump.intra.peff.net>
In-Reply-To: <d03308e9474f5e26fd4a5494ec243a278e971443.1754302009.git.liu.denton@gmail.com>

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.

Show 13 quoted lines

> 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