[PATCH 1/4] Fix maybe-uninitialized warning with GCC at -O3

[PATCH 1/4] Fix maybe-uninitialized warning with GCC at -O3

[Mike Hommey]

```
In file included from parse-options.c:1:
git-compat-util.h: In function ‘get_value’:
git-compat-util.h:489:21: error: ‘arg’ may be used uninitialized [-Werror=maybe-uninitialized]
  489 | #define error(...) (error(__VA_ARGS__), const_error())
      |                     ^~~~~
parse-options.c:76:21: note: ‘arg’ was declared here
   76 |         const char *arg;
      |                     ^~~
```

Signed-off-by: Mike Hommey <mh@glandium.org>
---
 parse-options.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/parse-options.c b/parse-options.c
index a9a39ecaef..cf79805bc0 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -73,7 +73,7 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
 					  enum opt_parsed flags,
 					  const char **argp)
 {
-	const char *arg;
+	const char *arg = NULL;
 	const int unset = flags & OPT_UNSET;
 	int err;
 
-- 
2.50.0.rc1.593.g042f21cb9b

[PATCH 2/4] Fix use-after-free warning with GCC at -O3

[Mike Hommey]

```
reftable/basics.c: In function ‘parse_names’:
reftable/basics.c:233:17: error: pointer ‘names’ may be used after ‘free’ [-Werror=use-after-free]
  233 |                 reftable_free(names[i]);
      |                 ^~~~~~~~~~~~~~~~~~~~~~~
In function ‘reftable_free’,
    inlined from ‘reftable_realloc’ at reftable/basics.c:30:3,
    inlined from ‘reftable_realloc’ at reftable/basics.c:27:7,
    inlined from ‘reftable_alloc_grow’ at reftable/basics.h:228:10,
    inlined from ‘parse_names’ at reftable/basics.c:214:8:
reftable/basics.c:44:17: note: call to ‘free’ here
   44 |                 free(p);
      |                 ^~~~~~~
```

Signed-off-by: Mike Hommey <mh@glandium.org>
---
 reftable/basics.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/reftable/basics.c b/reftable/basics.c
index 9988ebd635..de21fe6ef7 100644
--- a/reftable/basics.c
+++ b/reftable/basics.c
@@ -229,9 +229,11 @@ char **parse_names(char *buf, int size)
 	return names;
 
 err:
-	for (size_t i = 0; i < names_len; i++)
-		reftable_free(names[i]);
-	reftable_free(names);
+	if (names) {
+		for (size_t i = 0; i < names_len; i++)
+			reftable_free(names[i]);
+		reftable_free(names);
+	}
 	return NULL;
 }
 
-- 
2.50.0.rc1.593.g042f21cb9b

Re: [PATCH 2/4] Fix use-after-free warning with GCC at -O3

[Junio C Hamano]

Mike Hommey <mh@glandium.org> writes:

> Cc: gitster@pobox.com, Mike Hommey <mh@glandium.org>

Not to me, but to the designated area expert.

Thanks.

> ```
> reftable/basics.c: In function ‘parse_names’:
> reftable/basics.c:233:17: error: pointer ‘names’ may be used after ‘free’ [-Werror=use-after-free]
>   233 |                 reftable_free(names[i]);
>       |                 ^~~~~~~~~~~~~~~~~~~~~~~
> In function ‘reftable_free’,
>     inlined from ‘reftable_realloc’ at reftable/basics.c:30:3,
>     inlined from ‘reftable_realloc’ at reftable/basics.c:27:7,
>     inlined from ‘reftable_alloc_grow’ at reftable/basics.h:228:10,
>     inlined from ‘parse_names’ at reftable/basics.c:214:8:
> reftable/basics.c:44:17: note: call to ‘free’ here
>    44 |                 free(p);
>       |                 ^~~~~~~
> ```
>
> Signed-off-by: Mike Hommey <mh@glandium.org>
> ---
>  reftable/basics.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/reftable/basics.c b/reftable/basics.c
> index 9988ebd635..de21fe6ef7 100644
> --- a/reftable/basics.c
> +++ b/reftable/basics.c
> @@ -229,9 +229,11 @@ char **parse_names(char *buf, int size)
>  	return names;
>  
>  err:
> -	for (size_t i = 0; i < names_len; i++)
> -		reftable_free(names[i]);
> -	reftable_free(names);
> +	if (names) {
> +		for (size_t i = 0; i < names_len; i++)
> +			reftable_free(names[i]);
> +		reftable_free(names);
> +	}
>  	return NULL;
>  }

Re: [PATCH 2/4] Fix use-after-free warning with GCC at -O3

[Patrick Steinhardt]

On Wed, Jun 04, 2025 at 08:06:44AM +0900, Mike Hommey wrote:
> ```
> reftable/basics.c: In function ‘parse_names’:
> reftable/basics.c:233:17: error: pointer ‘names’ may be used after ‘free’ [-Werror=use-after-free]
>   233 |                 reftable_free(names[i]);
>       |                 ^~~~~~~~~~~~~~~~~~~~~~~
> In function ‘reftable_free’,
>     inlined from ‘reftable_realloc’ at reftable/basics.c:30:3,
>     inlined from ‘reftable_realloc’ at reftable/basics.c:27:7,
>     inlined from ‘reftable_alloc_grow’ at reftable/basics.h:228:10,
>     inlined from ‘parse_names’ at reftable/basics.c:214:8:
> reftable/basics.c:44:17: note: call to ‘free’ here
>    44 |                 free(p);
>       |                 ^~~~~~~
> ```

Same here, only posting the warning isn't sufficient to explain what's
going on.

> diff --git a/reftable/basics.c b/reftable/basics.c
> index 9988ebd635..de21fe6ef7 100644
> --- a/reftable/basics.c
> +++ b/reftable/basics.c
> @@ -229,9 +229,11 @@ char **parse_names(char *buf, int size)
>  	return names;
>  
>  err:
> -	for (size_t i = 0; i < names_len; i++)
> -		reftable_free(names[i]);
> -	reftable_free(names);
> +	if (names) {
> +		for (size_t i = 0; i < names_len; i++)
> +			reftable_free(names[i]);
> +		reftable_free(names);
> +	}
>  	return NULL;
>  }

This change shouldn't be needed in theory: `names_len` has a positive
value if and only if `names` is non-NULL. So the warning is a false
positive.

That being said I'm not opposed to squelching this warning. But details
like this should be explained in the commit message.

Patrick

[PATCH 3/4] Fix comma warnings with clang on Windows

[Mike Hommey]

e.g.
```
  compat/mingw.c:723:24: error: possible misuse of comma operator here [-Werror,-Wcomma]
    723 |                 return errno = ENOSYS, -1;
        |                                      ^
  compat/mingw.c:723:10: note: cast expression to void to silence warning
    723 |                 return errno = ENOSYS, -1;
        |                        ^~~~~~~~~~~~~~
        |                        (void)(       )
  /usr/x86_64-w64-mingw32/include/stdlib.h:155:15: note: expanded from macro 'errno'
    155 | #define errno (*_errno())
        |               ^
```

Signed-off-by: Mike Hommey <mh@glandium.org>
---
 compat/mingw.c | 48 ++++++++++++++++++++++++++++--------------------
 1 file changed, 28 insertions(+), 20 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 8a9972a1ca..cd54937ebd 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -501,8 +501,10 @@ static int mingw_open_append(wchar_t const *wfilename, int oflags, ...)
 	DWORD create = (oflags & O_CREAT) ? OPEN_ALWAYS : OPEN_EXISTING;
 
 	/* only these flags are supported */
-	if ((oflags & ~O_CREAT) != (O_WRONLY | O_APPEND))
-		return errno = ENOSYS, -1;
+	if ((oflags & ~O_CREAT) != (O_WRONLY | O_APPEND)) {
+		errno = ENOSYS;
+		return -1;
+	}
 
 	/*
 	 * FILE_SHARE_WRITE is required to permit child processes
@@ -2497,12 +2499,14 @@ static int start_timer_thread(void)
 	timer_event = CreateEvent(NULL, FALSE, FALSE, NULL);
 	if (timer_event) {
 		timer_thread = (HANDLE) _beginthreadex(NULL, 0, ticktack, NULL, 0, NULL);
-		if (!timer_thread )
-			return errno = ENOMEM,
-				error("cannot start timer thread");
-	} else
-		return errno = ENOMEM,
-			error("cannot allocate resources for timer");
+		if (!timer_thread) {
+			errno = ENOMEM;
+			return error("cannot start timer thread");
+		}
+	} else {
+		errno = ENOMEM;
+		return error("cannot allocate resources for timer");
+	}
 	return 0;
 }
 
@@ -2535,13 +2539,15 @@ int setitimer(int type UNUSED, struct itimerval *in, struct itimerval *out)
 	static const struct timeval zero;
 	static int atexit_done;
 
-	if (out)
-		return errno = EINVAL,
-			error("setitimer param 3 != NULL not implemented");
+	if (out) {
+		errno = EINVAL;
+		return error("setitimer param 3 != NULL not implemented");
+	}
 	if (!is_timeval_eq(&in->it_interval, &zero) &&
-	    !is_timeval_eq(&in->it_interval, &in->it_value))
-		return errno = EINVAL,
-			error("setitimer: it_interval must be zero or eq it_value");
+	    !is_timeval_eq(&in->it_interval, &in->it_value)) {
+		errno = EINVAL;
+		return error("setitimer: it_interval must be zero or eq it_value");
+	}
 
 	if (timer_thread)
 		stop_timer_thread();
@@ -2561,12 +2567,14 @@ int setitimer(int type UNUSED, struct itimerval *in, struct itimerval *out)
 
 int sigaction(int sig, struct sigaction *in, struct sigaction *out)
 {
-	if (sig != SIGALRM)
-		return errno = EINVAL,
-			error("sigaction only implemented for SIGALRM");
-	if (out)
-		return errno = EINVAL,
-			error("sigaction: param 3 != NULL not implemented");
+	if (sig != SIGALRM) {
+		errno = EINVAL;
+		return error("sigaction only implemented for SIGALRM");
+	}
+	if (out) {
+		errno = EINVAL;
+		return error("sigaction: param 3 != NULL not implemented");
+	}
 
 	timer_fn = in->sa_handler;
 	return 0;
-- 
2.50.0.rc1.593.g042f21cb9b

Re: [PATCH 3/4] Fix comma warnings with clang on Windows

[Junio C Hamano]

Mike Hommey <mh@glandium.org> writes:

> Subject: Re: [PATCH 3/4] Fix comma warnings with clang on Windows

Common to all four patches, as "Fix" does not quite tell the story,
please choose more appropriate verb.  For example, judging from how
this was rewritten ...

> -	if ((oflags & ~O_CREAT) != (O_WRONLY | O_APPEND))
> -		return errno = ENOSYS, -1;
> +	if ((oflags & ~O_CREAT) != (O_WRONLY | O_APPEND)) {
> +		errno = ENOSYS;
> +		return -1;
> +	}

... the warning is a false positive (i.e. the code does what the
author intended it to do), so this is more like "squelching" the
warning.

I obviously like the style after this patch.  I just thought that
the proposed log message, especially its title, were not clear
enough to tell which ones are real fixes and which ones are
workarounds.

Thanks.

[PATCH 4/4] Fix unreachable-code warning with clang on Windows

[Mike Hommey]

```
refs/files-backend.c:3187:5: error: code will never be executed [-Werror,-Wunreachable-code]
   3187 |                                 continue;
        |                                 ^~~~~~~~
  1 error generated.
```

Signed-off-by: Mike Hommey <mh@glandium.org>
---
 refs/files-backend.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index bf6f89b1d1..af21eb80a9 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3183,7 +3183,7 @@ static int files_transaction_finish(struct ref_store *ref_store,
 		 * next update. If not, we try and create a regular symref.
 		 */
 		if (update->new_target && refs->prefer_symlink_refs)
-			if (!create_ref_symlink(lock, update->new_target))
+			if (NOT_CONSTANT(!create_ref_symlink(lock, update->new_target)))
 				continue;
 
 		if (update->flags & REF_NEEDS_COMMIT) {
-- 
2.50.0.rc1.593.g042f21cb9b

Re: [PATCH 4/4] Fix unreachable-code warning with clang on Windows

[Patrick Steinhardt]

On Wed, Jun 04, 2025 at 08:06:46AM +0900, Mike Hommey wrote:
> ```
> refs/files-backend.c:3187:5: error: code will never be executed [-Werror,-Wunreachable-code]
>    3187 |                                 continue;
>         |                                 ^~~~~~~~
>   1 error generated.
> ```
> 
> Signed-off-by: Mike Hommey <mh@glandium.org>
> ---
>  refs/files-backend.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index bf6f89b1d1..af21eb80a9 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -3183,7 +3183,7 @@ static int files_transaction_finish(struct ref_store *ref_store,
>  		 * next update. If not, we try and create a regular symref.
>  		 */
>  		if (update->new_target && refs->prefer_symlink_refs)
> -			if (!create_ref_symlink(lock, update->new_target))
> +			if (NOT_CONSTANT(!create_ref_symlink(lock, update->new_target)))
>  				continue;

So the story here is that there are two implementations of
`create_ref_symlink()`:

  - One macro that is defined to `(-1)` which is set when
    NO_SYMLINK_HEAD is defined.

  - A function that creates the ref symlink if NO_SYMLINK_HEAD is not
    defined.

The function won't cause the error, but the macro will. So wouldn't it
make more sense to wrap the macro itself in `NOT_CONSTANT`, like this:

    #define create_ref_symlink(a, b) NOT_CONSTANT(-1)

Patrick

Re: [PATCH 4/4] Fix unreachable-code warning with clang on Windows

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> The function won't cause the error, but the macro will. So wouldn't it
> make more sense to wrap the macro itself in `NOT_CONSTANT`, like this:
>
>     #define create_ref_symlink(a, b) NOT_CONSTANT(-1)

That's clever ;-).


We cannot unfortunatel do the same at the site that the macro
NOT_CONSTANT() was invented for, though.

Re: [PATCH 1/4] Fix maybe-uninitialized warning with GCC at -O3

[Patrick Steinhardt]

On Wed, Jun 04, 2025 at 08:06:43AM +0900, Mike Hommey wrote:
> ```
> In file included from parse-options.c:1:
> git-compat-util.h: In function ‘get_value’:
> git-compat-util.h:489:21: error: ‘arg’ may be used uninitialized [-Werror=maybe-uninitialized]
>   489 | #define error(...) (error(__VA_ARGS__), const_error())
>       |                     ^~~~~
> parse-options.c:76:21: note: ‘arg’ was declared here
>    76 |         const char *arg;
>       |                     ^~~
> ```

A bit more explanation whether this warning is a false positive or
whether this may be an actual issue would be welcome.

Patrick

Re: [PATCH 1/4] Fix maybe-uninitialized warning with GCC at -O3

[Jeff King]

On Wed, Jun 04, 2025 at 09:36:03AM +0200, Patrick Steinhardt wrote:

> On Wed, Jun 04, 2025 at 08:06:43AM +0900, Mike Hommey wrote:
> > ```
> > In file included from parse-options.c:1:
> > git-compat-util.h: In function ‘get_value’:
> > git-compat-util.h:489:21: error: ‘arg’ may be used uninitialized [-Werror=maybe-uninitialized]
> >   489 | #define error(...) (error(__VA_ARGS__), const_error())
> >       |                     ^~~~~
> > parse-options.c:76:21: note: ‘arg’ was declared here
> >    76 |         const char *arg;
> >       |                     ^~~
> > ```
> 
> A bit more explanation whether this warning is a false positive or
> whether this may be an actual issue would be welcome.

I thought at first it was a false positive, as we'd always fill in "arg"
via get_arg(), or return an error. But I suspect the culprit is the
earlier part of this if/else chain:


                  if (unset) {
                          value = 0;
                  } else if (opt->flags & PARSE_OPT_OPTARG && !p->opt) {
                          value = opt->defval;
                  } else if (get_arg(p, opt, flags, &arg)) {
                          return -1;
		  } ...

So if "unset" is true, we set "value" but not "arg". The code the
compiler is complaining about is here:

                  if (value < lower_bound)
                          return error(_("value %s for %s not in range [%"PRIdMAX",%"PRIdMAX"]"),
                                       arg, optname(opt, flags), (intmax_t)lower_bound, (intmax_t)upper_bound);

In the case of "unset", I think we could never trigger this, since our
lower bound will never be above 0.

But what about opt->defval? We don't know anything about it here.
Probably it would be a programming error to pass in a value that is
outside the bounds, but this code doesn't know that. So something like
this (it was actually hard to find an integer option with OPTARG!):

diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index 3b6aac2cf7..630403611b 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -28,7 +28,7 @@ int cmd_fmt_merge_msg(int argc,
 			.argh = N_("n"),
 			.help = N_("populate log with at most <n> entries from shortlog"),
 			.flags = PARSE_OPT_OPTARG,
-			.defval = DEFAULT_MERGE_LOG_LEN,
+			.defval = -2147483649,
 		},
 		{
 			.type = OPTION_INTEGER,

would cause:

  git fmt-merge-msg --log

to print uninitialized memory. But it is equally wrong with Mike's
patch; we'd just segfault!

I think this is unlikely to happen in practice, but it does feel like we
should be able to solve it in a better way. I came up with two options.

One is to BUG() on a bogus default value, like this:

diff --git a/parse-options.c b/parse-options.c
index a9a39ecaef..7ddf213d0c 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -73,7 +73,7 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
 					  enum opt_parsed flags,
 					  const char **argp)
 {
-	const char *arg;
+	const char *arg = NULL;
 	const int unset = flags & OPT_UNSET;
 	int err;
 
@@ -195,9 +195,13 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
 				     optname(opt, flags));
 		}
 
-		if (value < lower_bound)
+		if (value < lower_bound) {
+			if (!arg)
+				BUG("default option value for '%s' is not in range",
+				    optname(opt, flags));
 			return error(_("value %s for %s not in range [%"PRIdMAX",%"PRIdMAX"]"),
 				     arg, optname(opt, flags), (intmax_t)lower_bound, (intmax_t)upper_bound);
+		}
 
 		switch (opt->precision) {
 		case 1:

The other is to not bother checking the bounds on the default values at
all, by pushing this bounds check into the if/else chain after we know
we have something to parse, like this (extended context to make it more
obvious how this fits into the chain):

diff --git a/parse-options.c b/parse-options.c
index a9a39ecaef..d36e56da15 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -179,39 +179,38 @@ static enum parse_opt_result do_get_value(struct parse_opt_ctx_t *p,
 
 		if (unset) {
 			value = 0;
 		} else if (opt->flags & PARSE_OPT_OPTARG && !p->opt) {
 			value = opt->defval;
 		} else if (get_arg(p, opt, flags, &arg)) {
 			return -1;
 		} else if (!*arg) {
 			return error(_("%s expects a numerical value"),
 				     optname(opt, flags));
 		} else if (!git_parse_signed(arg, &value, upper_bound)) {
 			if (errno == ERANGE)
 				return error(_("value %s for %s not in range [%"PRIdMAX",%"PRIdMAX"]"),
 					     arg, optname(opt, flags), lower_bound, upper_bound);
 
 			return error(_("%s expects an integer value with an optional k/m/g suffix"),
 				     optname(opt, flags));
-		}
-
-		if (value < lower_bound)
+		} else if (value < lower_bound) {
 			return error(_("value %s for %s not in range [%"PRIdMAX",%"PRIdMAX"]"),
 				     arg, optname(opt, flags), (intmax_t)lower_bound, (intmax_t)upper_bound);
+		}
 
 		switch (opt->precision) {
 		case 1:
 			*(int8_t *)opt->value = value;
 			return 0;
 		case 2:
 			*(int16_t *)opt->value = value;
 			return 0;
 		case 4:
 			*(int32_t *)opt->value = value;
 			return 0;
 		case 8:
 			*(int64_t *)opt->value = value;
 			return 0;
 		default:
 			BUG("invalid precision for option %s",
 			    optname(opt, flags));

-Peff