# [PATCH] fix -Wmaybe-uninitialized with -Og 10 messages from 2025-08-04 to 2025-08-11. Participants: Denton Liu, Jeff King, Junio C Hamano, Patrick Steinhardt. Thread: https://gitlist.dev/t/63905 ## Denton Liu, 2025-08-04 10:07 Subject: [PATCH] fix -Wmaybe-uninitialized with -Og Message-ID: URL: https://gitlist.dev/e/d03308e9474f5e26fd4a5494ec243a278e971443.1754302009.git.liu.denton%40gmail.com ``` 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 --- 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 ``` ## Jeff King, 2025-08-04 13:19 Subject: Re: [PATCH] fix -Wmaybe-uninitialized with -Og Message-ID: <20250804131922.GB86602@coredump.intra.peff.net> URL: https://gitlist.dev/e/20250804131922.GB86602%40coredump.intra.peff.net In-Reply-To: ``` 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 ``` ## Junio C Hamano, 2025-08-04 13:46 Subject: Re: [PATCH] fix -Wmaybe-uninitialized with -Og Message-ID: URL: https://gitlist.dev/e/xmqqms8f9p2t.fsf%40gitster.g In-Reply-To: <20250804131922.GB86602@coredump.intra.peff.net> ``` Jeff King 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. ``` ## Jeff King, 2025-08-04 15:53 Subject: Re: [PATCH] fix -Wmaybe-uninitialized with -Og Message-ID: <20250804155329.GD109984@coredump.intra.peff.net> URL: https://gitlist.dev/e/20250804155329.GD109984%40coredump.intra.peff.net In-Reply-To: ``` 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 Signed-off-by: Jeff King --- 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 ``` ## Denton Liu, 2025-08-05 05:31 Subject: [PATCH v2 0/2] fix -Wmaybe-uninitialized with -Og Message-ID: URL: https://gitlist.dev/e/cover.1754371649.git.liu.denton%40gmail.com In-Reply-To: ``` 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 +Author: Jeff King ## 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 + Signed-off-by: Jeff King ## 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 ``` ## Denton Liu, 2025-08-05 05:31 Subject: [PATCH v2 1/2] remote: bail early from set_head() if missing remote name Message-ID: <458ec588b7fe4d40f834d535b8f7d83684f9935b.1754371650.git.liu.denton@gmail.com> URL: https://gitlist.dev/e/458ec588b7fe4d40f834d535b8f7d83684f9935b.1754371650.git.liu.denton%40gmail.com In-Reply-To: ``` From: Jeff King 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 Signed-off-by: Jeff King Tested-by: Denton Liu Signed-off-by: Denton Liu --- 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 ``` ## Denton Liu, 2025-08-05 05:31 Subject: [PATCH v2 2/2] t/unit-tests/clar: fix -Wmaybe-uninitialized with -Og Message-ID: <8ed0ac14092e7ec979e53d2a3da84dfe884d6b3f.1754371650.git.liu.denton@gmail.com> URL: https://gitlist.dev/e/8ed0ac14092e7ec979e53d2a3da84dfe884d6b3f.1754371650.git.liu.denton%40gmail.com In-Reply-To: ``` 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 --- 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 ``` ## Patrick Steinhardt, 2025-08-08 05:48 Subject: Re: [PATCH v2 2/2] t/unit-tests/clar: fix -Wmaybe-uninitialized with -Og Message-ID: URL: https://gitlist.dev/e/aJWPmo6oGCuQvqMG%40pks.im In-Reply-To: <8ed0ac14092e7ec979e53d2a3da84dfe884d6b3f.1754371650.git.liu.denton@gmail.com> ``` 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 ``` ## Denton Liu, 2025-08-08 07:55 Subject: Re: [PATCH v2 2/2] t/unit-tests/clar: fix -Wmaybe-uninitialized with -Og Message-ID: URL: https://gitlist.dev/e/aJWtbGGBOELZN6tp%40generichostname In-Reply-To: ``` 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 ``` ## Patrick Steinhardt, 2025-08-11 09:00 Subject: Re: [PATCH v2 2/2] t/unit-tests/clar: fix -Wmaybe-uninitialized with -Og Message-ID: URL: https://gitlist.dev/e/aJmxExJgFKxeiHDf%40pks.im In-Reply-To: ``` 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 ```