This series fixes the rest of the warnings you might see on recent glibc or other C23 libc where:

  const char *in = ...;
  char *out = strchr(in, ...);

now complains instead of quietly assigning the const pointer to a non-const one. It's all textually independent of the other fixes, but if you want a clean build you'll need the others. I think Collin's fixes have hit master already, but my jk/c23-const-preserving-fixes are still slated for 'next'.

Some of my fixes are similar to what Michael posted in:

  https://lore.kernel.org/git/cover.1774537954.git.git@grubix.eu/

but for most of them I took a somewhat different approach. So this would be applied instead of those patches.

The patches are:

  [01/12]: convert: add const to fix strchr() warnings
  [02/12]: http: add const to fix strchr() warnings
  [03/12]: transport-helper: drop const to fix strchr() warnings

    These ones are obvious fixes that just match the type declarations
    to their uses.

  [04/12]: pager: explicitly cast away strchr() constness
  [05/12]: run-command: explicitly cast away constness when assigning to void

    These are ones where I think an explicit cast is the least-bad
    option.

  [06/12]: find_last_dir_sep(): convert inline function to macro

    This is the one that gets repeated a zillion times when you build
    because it's in a header file. ;) It takes a slightly different
    approach than Collin's in:

      https://lore.kernel.org/git/e6f7e2eddbc9aef1c21f661420a4b8cb9cd8e2c1.1770095829.git.collin.funk1@gmail.com/

    which I think reduces the fallout through the rest of the codebase.

  [07/12]: pseudo-merge: fix disk reads from find_pseudo_merge()

    This one is...spicy. I think there are probably actual bugs here,
    but my hope is that this takes us in the right direction (and shuts
    up the warning).

  [08/12]: skip_prefix(): check const match between in and out params

    And here is where we might get controversial. It introduces some
    macro hackery that makes it safe and easy to use skip_prefix() with
    const or non-const strings. I _think_ it should just work
    everywhere, but I won't be surprised if some compiler somewhere
    complains about the construct. Coverity does, but it is so full of
    false positives that adding more is not a big deal.

  [09/12]: pkt-line: make packet_reader.line non-const
  [10/12]: range-diff: drop const to fix strstr() warnings
  [11/12]: http: drop const to fix strstr() warning
  [12/12]: refs/files-backend: drop const to fix strchr() warning

     And then these are all obvious fixes that are only made possible by
     the skip_prefix() magic above. Well, possible without extra ugly
     casts everywhere.

 builtin/config.c    |  7 ++++---
 builtin/rev-parse.c | 40 ++++++++++++++++++++--------------------
 convert.c           |  3 ++-
 git-compat-util.h   | 23 ++++++++++++++++++-----
 http-push.c         |  2 +-
 pager.c             |  3 ++-
 pseudo-merge.c      | 32 +++++++++++++++++++-------------
 revision.c          | 25 +++++++++++++++----------
 run-command.c       |  4 ++--
 transport-helper.c  |  3 ++-
 10 files changed, 85 insertions(+), 57 deletions(-)

-Peff

C23 versions of libc (like recent glibc) may provide generic versions of strchr() that match constness between the input and return value. The idea being that the compiler can detect when it implicitly converts a const pointer into a non-const one (which then emits a warning).

There are a few cases here where the result pointer does not need to be non-const at all, and we should mark it as such. That silences the warning (and avoids any potential problems with trying to write via those pointers).

Signed-off-by: Jeff King <peff@peff.net>
---
 convert.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

The "path" field of a "struct repo" (a custom http-push struct, not to be confused with "struct repository) is a pointer into a const argv string, and is never written to.

The compiler does not traditionally complain about assigning from a const pointer because it happens via strchr(). But with some C23 libc versions (notably recent glibc), it has started to do so. Let's mark the field as const to silence the warnings.

Signed-off-by: Jeff King <peff@peff.net>
---
 http-push.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

We implicitly drop the const from our "key" variable when we do:

  char *p = strchr(key, ' ');

which causes compilation with some C23 versions of libc (notably recent glibc) to complain.

We need "p" to remain writable, since we assign NULL over the space we found. We can solve this by also making "key" writable. This works because it comes from a strbuf, which is itself a writable string.

Signed-off-by: Jeff King <peff@peff.net>
---
 transport-helper.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

When we do:

  char *cp = strchr(argv[i], '=');

it implicitly removes the constness from argv[i]. We need "cp" to remain writable (since we overwrite it with a NUL). In theory we should be able to drop the const from argv[i], because it is a sub-pointer into our duplicated pager_env variable.

But we get it from split_cmdline(), which uses the traditional "const char **" type for argv. This is overly limiting, but changing it would be awkward for all the other callers of split_cmdline().

Let's do an explicit cast with a note about why it is OK. This is enough to silence compiler warnings about the implicit const problems.

Signed-off-by: Jeff King <peff@peff.net>
---
 pager.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

We do this:

  char *equals = strchr(*e, '=');

which implicitly removes the constness from "*e" and cause the compiler to complain. We never write to "equals", but later assign it to a string_list util field, which is defined as non-const "void *".

We have to cast somewhere, but doing so at the assignment to util is the least-bad place, since that is the source of the confusion. Sadly we are still open to accidentally writing to the string via the util pointer, but that is the cost of using void pointers, which lose all type information.

Signed-off-by: Jeff King <peff@peff.net>
---
 run-command.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

The find_last_dir_sep() function is implemented as an inline function which takes in a "const char *" and returns a "char *" via strrchr(). That means that just like strrchr(), it quietly removes the const from our pointer, which could lead to accidentally writing to the resulting string.

But C23 versions of libc (including recent glibc) annotate strrchr() such that the compiler can detect when const is implicitly lost, and it now complains about the call in this inline function.

We can't just switch the return type of the function to "const char *", though. Some callers really do want a non-const string to be returned (and are OK because they are feeding a non-const string into the function).

The most general solution is for us to annotate find_last_dir_sep() in the same way that is done for strrchr(). But doing so relies on using C23 generics, which we do not otherwise require.

Since this inline function is wrapping a single call to strrchr(), we can take a shortcut. If we implement it as a macro, then the original type information is still available to strrchr(), and it does the check for us.

Note that this is just one implementation of find_last_dir_sep(). There is an alternate implementation in compat/win32/path-utils.h. It doesn't suffer from the same warning, as it does not use strrchr() and just casts away const explicitly. That's not ideal, and eventually we may want to conditionally teach it the same C23 generic trick that strrchr() uses. But it has been that way forever, and our goal here is just quieting new warnings, not improving const-checking.

Signed-off-by: Jeff King <peff@peff.net>
---
 git-compat-util.h | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

The goal of this commit was to fix a const warning when compiling with new versions of glibc, but ended up untangling a much deeper problem.

The find_pseudo_merge() function does a bsearch() on the "commits" pointer of a pseudo_merge_map. This pointer ultimately comes from memory mapped from the on-disk bitmap file, and is thus not writable.

The "commits" array is correctly marked const, but the result from bsearch() is returned directly as a non-const pseudo_merge_commit struct. Since new versions of glibc annotate bsearch() in a way that detects the implicit loss of const, the compiler now warns.

My first instinct was that we should be returning a const struct. That requires apply_pseudo_merges_for_commit() to mark its local pointer as const. But that doesn't work! If the offset field has the high-bit set, we look it up in the extended table via nth_pseudo_merge_ext(). And that function then feeds our const struct to read_pseudo_merge_commit_at(), which writes into it by byte-swapping from the on-disk mmap.

But I think this points to a larger problem with find_pseudo_merge(). It is not just that the return value is missing const, but it is missing that byte-swapping! And we know that byte-swapping is needed here, because the comparator we use for bsearch() also calls our read_pseudo_merge_commit_at() helper.

So I think the interface is all wrong here. We should not be returning a pointer to a struct which was cast from on-disk data. We should be filling in a caller-provided struct using the bytes we found, byte-swapping the values.

That of course raises the dual question: how did this ever work, and does it work now? The answer to the first part is: this code does not seem to be triggered in the test suite at all. If we insert a BUG("foo") call into apply_pseudo_merges_for_commit(), it never triggers.

So I think there is something wrong or missing from the test setup, and this bears further investigation. Sadly the answer to the second part ("does it work now") is still "no idea". I _think_ this takes us in a positive direction, but my goal here is mainly to quiet the compiler warning. Further bug-hunting on this experimental feature can be done separately.

Signed-off-by: Jeff King <peff@peff.net>
---
+cc Taylor

 pseudo-merge.c | 32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

The skip_prefix() function takes in a "const char *" string, and returns via a "const char **" out-parameter that points somewhere in that string. This is fine if you are operating on a const string, like:

  const char *in = ...;
  const char *out;
  if (skip_prefix(in, "foo", &out))
	...look at out...

It is also OK if "in" is not const but "out" is, as we add an implicit const when we pass "in" to the function. But there's another case where this is limiting. If we want both fields to be non-const, like:

  char *in = ...;
  char *out;
  if (skip_prefix(in, "foo", &out))
	*out = '\0';

it doesn't work. The compiler will complain about the type mismatch in passing "&out" to a parameter which expects "const char **". So to make this work, we have to do an explicit cast.

But such a cast is ugly, and also means that we run afoul of making this mistake:

  const char *in = ...;
  char *out;
  if (skip_prefix(in, "foo", (const char **)&out))
	*out = '\0';

which causes us to write to the memory pointed by "in", which was const.

We can imagine these four cases as:

  (1) const in, const out
  (2) non-const in, const out
  (3) non-const in, non-const out
  (4) const in, non-const out

Cases (1) and (2) work now. We would like case (3) to work but it doesn't. But we would like to catch case (4) as a compile error.

So ideally the rule is "the out-parameter must be at least as const as the in-parameter". We can do this with some macro trickery. We wrap skip_prefix() in a macro so that it has access to the real types of in/out. And then we pass those parameters through another macro which:

  1. Fails if the "at least as const" rule is not filled.

  2. Casts to match the signature of the real skip_prefix().

There are a lot of ways to implement the "fails" part. You can use __builtin_types_compatible_p() to check, and then either our BUILD_ASSERT macros or _Static_assert to fail. But that requires some conditional compilation based on compiler feature. That's probably OK (the fallback would be to just cast without catching case 4). But we can do better.

The macro I have here uses a ternary with a dead branch that tries to assign "in" to "out", which should work everywhere and lets the compiler catch the problem in the usual way. With an input like this:

  int foo(const char *x, const char **y);
  #define foo(in,out) foo((in), CONST_OUTPARAM((in), (out)))

  void ok_const(const char *x, const char **y)
  {
          foo(x, y);
  }

  void ok_nonconst(char *x, char **y)
  {
          foo(x, y);
  }

  void ok_add_const(char *x, const char **y)
  {
          foo(x, y);
  }

  void bad_drop_const(const char *x, char **y)
  {
          foo(x, y);
  }

gcc reports:

  foo.c: In function ‘bad_drop_const’:
  foo.c:2:35: error: assignment discards ‘const’ qualifier from pointer target type [-Werror=discarded-qualifiers]
      2 |     ((const char **)(0 ? ((*(out) = (in)),(out)) : (out)))
        |                                   ^
  foo.c:4:31: note: in expansion of macro ‘CONST_OUTPARAM’
      4 | #define foo(in,out) foo((in), CONST_OUTPARAM((in), (out)))
        |                               ^~~~~~~~~~~~~~
  foo.c:23:9: note: in expansion of macro ‘foo’
     23 |         foo(x, y);
        |         ^~~

It's a bit verbose, but I think makes it reasonably clear what's going on. Using BUILD_ASSERT_OR_ZERO() ends up much worse. Using _Static_assert you can be a bit more informative, but that's not something we use at all yet in our code-base (it's an old gnu-ism later standardized in C11).

Our generic macro only works for "const char **", which is something we could improve by using typeof(in). But that introduces more portability questions, and also some weird corner cases (e.g., around implicit void conversion).

This patch just introduces the concept. We'll make use of it in future patches.

Signed-off-by: Jeff King <peff@peff.net>
---
 git-compat-util.h | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

The "line" member of a packet_reader struct is marked as const. This kind of makes sense, because it's not its own allocated buffer that should be freed, and we often use const to indicate that. But it is always writable, because it points into the non-const "buffer" member.

And we rely on this writability in places like send-pack and receive-pack, where we parse incoming packet contents by writing NULs over delimiters. This has traditionally worked because we implicitly cast away the constness with strchr() like:

  const char *head;
  char *p;

  head = reader->line;
  p = strchr(head, ' ');

Since C23 libc provides a generic strchr() to detect this implicit const removal, this now generate a compiler warning on some platforms (like recent glibc).

We can fix it by marking "line" as non-const, as well as a few intermediate variables (like "head" in the above example). Note that by itself, switching to a non-const variable would cause problems with this line in send-pack.c:

  if (!skip_prefix(reader->line, "unpack ", &reader->line))

But due to our skip_prefix() magic introduced in the previous commit, this compiles fine (both the in and out-parameters are non-const, so we know it is safe).

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/receive-pack.c | 7 ++++---
 pkt-line.h             | 2 +-
 send-pack.c            | 7 ++++---
 3 files changed, 9 insertions(+), 7 deletions(-)

This is another case where we implicitly drop the "const" from a pointer by feeding it to strstr() and assigning the result to a non-const pointer. This is OK in practice, since the const pointer originally comes from a writable source (a strbuf), but C23 libc implementations have started to complain about it.

We do write to the output pointer, so it needs to remain non-const. We can just switch the input pointer to also be non-const in this case. By itself that would run into problems with calls to skip_prefix(), but since that function has now been taught to match in/out constness automatically, it just works without us doing anything further.

Signed-off-by: Jeff King <peff@peff.net>
---
 range-diff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

In redact_sensitive_header(), a C23 implementation of libc will complain that strstr() assigns the result from "const char *cookie" to "char *semicolon".

Ultimately the memory is writable. We're fed a strbuf, generate a const pointer "sensitive_header" within it using skip_iprefix(), and then assign the result to "cookie". So we can solve this by dropping the const from "cookie" and "sensitive_header".

However, this runs afoul of skip_iprefix(), which wants a "const char **" for its out-parameter. We can solve that by teaching skip_iprefix() the same "make sure out is at least as const as in" magic that we recently taught to skip_prefix().

Signed-off-by: Jeff King <peff@peff.net>
---
 git-compat-util.h | 3 +++
 http.c            | 4 ++--
 2 files changed, 5 insertions(+), 2 deletions(-)

In show_one_reflog_ent(), we're fed a writable strbuf buffer, which we parse into the various reflog components. We write a NUL over email_end to tie off one of the fields, and thus email_end must be non-const.

But with a C23 implementation of libc, strchr() will now complain when assigning the result to a non-const pointer from a const one. So we can fix this by making the source pointer non-const.

But there's a catch. We derive that source pointer by parsing the line with parse_oid_hex_algop(), which requires a const pointer for its out-parameter. We can work around that by teaching it to use our CONST_OUTPARAM() trick, just like skip_prefix(). Note that unlike skip_prefix(), the function is not inline, so we can't just wrap it using the same name (otherwise the actual definition would expand the macro, which breaks compilation). So we rename the actual function with an "_impl" suffix, and callers will all use the macro.

Signed-off-by: Jeff King <peff@peff.net>
---
 hex.c                | 6 +++---
 hex.h                | 6 ++++--
 refs/files-backend.c | 2 +-
 3 files changed, 8 insertions(+), 6 deletions(-)

On Tue, Mar 31, 2026 at 07:46:23PM -0400, Jeff King wrote:

If this is the wrong direction or if we just want to keep things minimal in this patch series, the absolute smallest fix is probably to cast away the constness explicitly in find_pseudo_merge(), along with a comment that the fix is almost certainly wrong. ;)

-Peff

Hi peff

On 01/04/2026 00:50, Jeff King wrote:

This is clever but it changes the behavior of skip_prefix() which is documented as not touching out if it returns false. That may not matter in practice but there are nearly 600 callers so auditing them all would be quite an undertaking. Elsewhere there was some discussion about using type generic macros to fix the warnings. That would be more complex as we need to check if they were supported by the compiler but it would avoid changing the behavior.

Thanks for working on fixing these warnings, your approach of trying to fix the underlying problem rather than casting away the warning is very welcome.

Phillip

On Tue, Mar 31, 2026 at 07:41:48PM -0400, Jeff King wrote:

You probably mean NUL, not NULL.

Patrick

On Tue, Mar 31, 2026 at 07:50:17PM -0400, Jeff King wrote:

Okay. In theory this would cause us to evaluate both `out` and `str` multiple times. But I guess because this is a dead branch we expect the compiler to optimize these statements away so that we don't have to worry about this.

Constructs like this always feel a bit dirty, but I guess this is going to be fine in practice.

Patrick

On Tue, Mar 31, 2026 at 07:53:41PM -0400, Jeff King wrote:

Fair. In fact, I was a bit torn with the other commits whether it's nice to reuse the same name. I guess what it buys us is that you cannot accidentally call the wrong function without the guardrails. Even though that's quite unlikely with the `_impl` suffix.

Thanks!

Patrick

On 01/04/2026 14:17, Phillip Wood wrote:

Sorry, I've just realized we always take the other branch so this does not change the behavior and is in fact a nice solution to the problem.

Thanks

Phillip

On Wed, Apr 01, 2026 at 03:04:10PM +0100, Phillip Wood wrote:

Yeah, exactly. I was curious if the dead branch would be left in place, but gcc seems to prune it even at -O0.

I also pondered whether:

  (*out = in,out)

might be a problem, but I think it is OK. The "," is a sequence point, so it is well defined (of course we would never run this code anyway, but if we have undefined behavior in the code at all, it may cause confusing effects).

For reference, this is the more complicated one I came up with:

  /*
   * Note that builtin_types_compatible_p() counts "char" and "const
   * char" as the same type. So we deref and construct our own pointer
   * with const to find out it "x" is const, and then either compare
   * x and y exactly (if it is const, they must both be) or dereferenced
   * (which lets y be either const or not).
   */
  #define CONST_COMPATIBLE(x, y) \
          (__builtin_types_compatible_p(typeof(x), const typeof(*(x)) *) ? \
           __builtin_types_compatible_p(typeof(x), typeof(y)) : \
           __builtin_types_compatible_p(typeof(*(x)), typeof(*(y))))

I also tried using a gcc statement-expression and _Static_assert to get a nicer message, like this:

  #define CONST_OUTPARAM(in, out, in_name, out_name) ({ \
          _Static_assert(CONST_COMPATIBLE((in),*(out)), \
                         in_name " is not const-compatible with " out_name); \
          (const typeof(*(in)) **)(out); \
  })
  #define skip_prefix(str, prefix, out) \
          skip_prefix(str, prefix, CONST_OUTPARAM((str), (out), #str, #out))

It does produce slightly nicer output:

  foo.c: In function ‘bad’:
  foo.c:8:9: error: static assertion failed: "my_in_var is not const-compatible with my_out_var"
      8 |         _Static_assert(CONST_COMPATIBLE((in),*(out)), \
        |         ^~~~~~~~~~~~~~
  foo.c:13:34: note: in expansion of macro ‘CONST_OUTPARAM’
     13 |         skip_prefix(str, prefix, CONST_OUTPARAM((str), (out), #str, #out))
        |                                  ^~~~~~~~~~~~~~
  foo.c:16:9: note: in expansion of macro ‘skip_prefix’
     16 |         skip_prefix(my_in_var, "foo", my_out_var);

but I don't think the extra complexity and portability headache is worth it.

-Peff

Jeff King <peff@peff.net> writes:

Yeah, it was the first thing that came to my mind that const char **argv is the source of the problem. We could cast the pointer we give to split_cmdline() and drop const from argv[] instead, which may make the in-code comment unnecessary but the patch we see here is good enough.

Jeff King <peff@peff.net> writes:

;-) Together with the BUG("foo") at the beginning of the function...

Jeff King <peff@peff.net> writes:

Yup, the part I like this the most is that this is still well defined, and the never-taken side of the ternary will not cause us trouble.

Very nicely done.

Jeff King <peff@peff.net> writes:

This is interesting. Once we go down this path, will we rethink the use of "const" as "not ours" hint (which I always found confusing)?

OK.

Patrick Steinhardt <ps@pks.im> writes:

I share the sentiment. If I were deciding the design, I'd even go forcing the _impl suffix to everything, including the inline ones, as I found the earlier "strip_prefix()" example already confusing.

On Wed, Apr 01, 2026 at 01:50:27PM -0700, Junio C Hamano wrote:

Ooh, that is a good idea. It puts the cast closer to the actual root cause. I think there are one or two other touch-ups, so I'll include that in a v2.

-Peff

On Wed, Apr 01, 2026 at 03:18:35PM -0700, Junio C Hamano wrote:

Maybe. It is already a weak signal, so I consider it more of a hint than a rule. I think it probably applies more consistently to the return value of functions (most things returning "char *" probably are passing back ownership).

-Peff

On Wed, Apr 01, 2026 at 03:22:48PM -0700, Junio C Hamano wrote:

OK. I think the _impl() is ugly, but the idea is that you never have to type it outside of the macro anyway. I'll switch to that for the others for consistency.

-Peff

On Tue, Mar 31, 2026 at 07:38:57PM -0400, Jeff King wrote:

And here's a v2 with some minor changes based on review of round 1:

  - move cast in patch 4 to split_cmdline(); note that this makes the
    patch totally different from range-diff's perspective

  - fix NULL/NUL typo

  - use _impl() suffix consistently for wrapped functions. I also
    reordered the macro definitions a bit for readability. Patch 8 also
    does not show up in range-diff (unlike patch 4, you can convince it
    to show it with "--creation-factor=80", but the result is not very
    readable anyway).

  [01/12]: convert: add const to fix strchr() warnings
  [02/12]: http: add const to fix strchr() warnings
  [03/12]: transport-helper: drop const to fix strchr() warnings
  [04/12]: pager: explicitly cast away strchr() constness
  [05/12]: run-command: explicitly cast away constness when assigning to void
  [06/12]: find_last_dir_sep(): convert inline function to macro
  [07/12]: pseudo-merge: fix disk reads from find_pseudo_merge()
  [08/12]: skip_prefix(): check const match between in and out params
  [09/12]: pkt-line: make packet_reader.line non-const
  [10/12]: range-diff: drop const to fix strstr() warnings
  [11/12]: http: drop const to fix strstr() warning
  [12/12]: refs/files-backend: drop const to fix strchr() warning

 builtin/receive-pack.c |  7 ++++---
 convert.c              |  3 ++-
 git-compat-util.h      | 33 ++++++++++++++++++++++++---------
 hex.c                  |  6 +++---
 hex.h                  |  6 ++++--
 http-push.c            |  2 +-
 http.c                 |  4 ++--
 pager.c                |  5 +++--
 pkt-line.h             |  2 +-
 pseudo-merge.c         | 32 +++++++++++++++++++-------------
 range-diff.c           |  2 +-
 refs/files-backend.c   |  2 +-
 run-command.c          |  4 ++--
 send-pack.c            |  7 ++++---
 transport-helper.c     |  3 ++-
 15 files changed, 73 insertions(+), 45 deletions(-)

 1:  4b149037ed =  1:  785b9cd5c0 convert: add const to fix strchr() warnings
 2:  65eb42692e =  2:  6e4b0b2e50 http: add const to fix strchr() warnings
 3:  659dde8201 !  3:  42ee9437c2 transport-helper: drop const to fix strchr() warnings
    @@ Commit message
         which causes compilation with some C23 versions of libc (notably recent
         glibc) to complain.
     
    -    We need "p" to remain writable, since we assign NULL over the space we
    +    We need "p" to remain writable, since we assign NUL over the space we
         found. We can solve this by also making "key" writable. This works
         because it comes from a strbuf, which is itself a writable string.
     
 4:  66018b5ccf <  -:  ---------- pager: explicitly cast away strchr() constness
 -:  ---------- >  4:  856a72ded8 pager: explicitly cast away strchr() constness
 5:  a696a8a4d9 =  5:  8181dcb941 run-command: explicitly cast away constness when assigning to void
 6:  32bba4d607 =  6:  2a0830a807 find_last_dir_sep(): convert inline function to macro
 7:  ddd0798d19 =  7:  e5b8820cf0 pseudo-merge: fix disk reads from find_pseudo_merge()
 8:  d736d1e070 <  -:  ---------- skip_prefix(): check const match between in and out params
 -:  ---------- >  8:  e87d8b70e2 skip_prefix(): check const match between in and out params
 9:  22c8d65229 =  9:  ed84dc926b pkt-line: make packet_reader.line non-const
10:  5cca7c109c = 10:  ff425ef7c3 range-diff: drop const to fix strstr() warnings
11:  30e7b35073 ! 11:  f01ba0616f http: drop const to fix strstr() warning
    @@ Commit message
         Signed-off-by: Jeff King <peff@peff.net>
     
      ## git-compat-util.h ##
    -@@ git-compat-util.h: static inline bool skip_iprefix(const char *str, const char *prefix,
    - 	return false;
    - }
    - 
    +@@ git-compat-util.h: static inline size_t xsize_t(off_t len)
    +  * is done via tolower(), so it is strictly ASCII (no multi-byte characters or
    +  * locale-specific conversions).
    +  */
    +-static inline bool skip_iprefix(const char *str, const char *prefix,
    +-			       const char **out)
     +#define skip_iprefix(str, prefix, out) \
    -+	skip_iprefix((str), (prefix), CONST_OUTPARAM((str), (out)))
    -+
    - /*
    -  * Like skip_prefix_mem, but compare case-insensitively. Note that the
    -  * comparison is done via tolower(), so it is strictly ASCII (no multi-byte
    ++	skip_iprefix_impl((str), (prefix), CONST_OUTPARAM((str), (out)))
    ++static inline bool skip_iprefix_impl(const char *str, const char *prefix,
    ++				     const char **out)
    + {
    + 	do {
    + 		if (!*prefix) {
     
      ## http.c ##
     @@ http.c: static int has_proxy_cert_password(void)
12:  317436aca5 ! 12:  d5c971cf26 refs/files-backend: drop const to fix strchr() warning
    @@ Commit message
         But there's a catch. We derive that source pointer by parsing the line
         with parse_oid_hex_algop(), which requires a const pointer for its
         out-parameter. We can work around that by teaching it to use our
    -    CONST_OUTPARAM() trick, just like skip_prefix(). Note that unlike
    -    skip_prefix(), the function is not inline, so we can't just wrap it
    -    using the same name (otherwise the actual definition would expand the
    -    macro, which breaks compilation). So we rename the actual function with
    -    an "_impl" suffix, and callers will all use the macro.
    +    CONST_OUTPARAM() trick, just like skip_prefix().
     
         Signed-off-by: Jeff King <peff@peff.net>
     
    @@ hex.h: char *oid_to_hex(const struct object_id *oid);						/* same static buffer
       */
     -int parse_oid_hex_algop(const char *hex, struct object_id *oid, const char **end,
     -			const struct git_hash_algo *algo);
    -+int parse_oid_hex_algop_impl(const char *hex, struct object_id *oid, const char **end,
    -+			     const struct git_hash_algo *algo);
     +#define parse_oid_hex_algop(hex, oid, end, algo) \
     +	parse_oid_hex_algop_impl((hex), (oid), CONST_OUTPARAM((hex), (end)), (algo))
    ++int parse_oid_hex_algop_impl(const char *hex, struct object_id *oid, const char **end,
    ++			     const struct git_hash_algo *algo);
      
      /*
       * These functions work like get_oid_hex and parse_oid_hex, but they will parse

C23 versions of libc (like recent glibc) may provide generic versions of strchr() that match constness between the input and return value. The idea being that the compiler can detect when it implicitly converts a const pointer into a non-const one (which then emits a warning).

There are a few cases here where the result pointer does not need to be non-const at all, and we should mark it as such. That silences the warning (and avoids any potential problems with trying to write via those pointers).

Signed-off-by: Jeff King <peff@peff.net>
---
 convert.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

The "path" field of a "struct repo" (a custom http-push struct, not to be confused with "struct repository) is a pointer into a const argv string, and is never written to.

The compiler does not traditionally complain about assigning from a const pointer because it happens via strchr(). But with some C23 libc versions (notably recent glibc), it has started to do so. Let's mark the field as const to silence the warnings.

Signed-off-by: Jeff King <peff@peff.net>
---
 http-push.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

We implicitly drop the const from our "key" variable when we do:

  char *p = strchr(key, ' ');

which causes compilation with some C23 versions of libc (notably recent glibc) to complain.

We need "p" to remain writable, since we assign NUL over the space we found. We can solve this by also making "key" writable. This works because it comes from a strbuf, which is itself a writable string.

Signed-off-by: Jeff King <peff@peff.net>
---
 transport-helper.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

When we do:

  char *cp = strchr(argv[i], '=');

it implicitly removes the constness from argv[i]. We need "cp" to remain writable (since we overwrite it with a NUL). In theory we should be able to drop the const from argv[i], because it is a sub-pointer into our duplicated pager_env variable.

But we get it from split_cmdline(), which uses the traditional "const char **" type for argv. This is overly limiting, but changing it would be awkward for all the other callers of split_cmdline().

Let's do an explicit cast with a note about why it is OK. This is enough to silence compiler warnings about the implicit const problems.

Signed-off-by: Jeff King <peff@peff.net>
---
 pager.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

We do this:

  char *equals = strchr(*e, '=');

which implicitly removes the constness from "*e" and cause the compiler to complain. We never write to "equals", but later assign it to a string_list util field, which is defined as non-const "void *".

We have to cast somewhere, but doing so at the assignment to util is the least-bad place, since that is the source of the confusion. Sadly we are still open to accidentally writing to the string via the util pointer, but that is the cost of using void pointers, which lose all type information.

Signed-off-by: Jeff King <peff@peff.net>
---
 run-command.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

The find_last_dir_sep() function is implemented as an inline function which takes in a "const char *" and returns a "char *" via strrchr(). That means that just like strrchr(), it quietly removes the const from our pointer, which could lead to accidentally writing to the resulting string.

But C23 versions of libc (including recent glibc) annotate strrchr() such that the compiler can detect when const is implicitly lost, and it now complains about the call in this inline function.

We can't just switch the return type of the function to "const char *", though. Some callers really do want a non-const string to be returned (and are OK because they are feeding a non-const string into the function).

The most general solution is for us to annotate find_last_dir_sep() in the same way that is done for strrchr(). But doing so relies on using C23 generics, which we do not otherwise require.

Since this inline function is wrapping a single call to strrchr(), we can take a shortcut. If we implement it as a macro, then the original type information is still available to strrchr(), and it does the check for us.

Note that this is just one implementation of find_last_dir_sep(). There is an alternate implementation in compat/win32/path-utils.h. It doesn't suffer from the same warning, as it does not use strrchr() and just casts away const explicitly. That's not ideal, and eventually we may want to conditionally teach it the same C23 generic trick that strrchr() uses. But it has been that way forever, and our goal here is just quieting new warnings, not improving const-checking.

Signed-off-by: Jeff King <peff@peff.net>
---
 git-compat-util.h | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

The goal of this commit was to fix a const warning when compiling with new versions of glibc, but ended up untangling a much deeper problem.

The find_pseudo_merge() function does a bsearch() on the "commits" pointer of a pseudo_merge_map. This pointer ultimately comes from memory mapped from the on-disk bitmap file, and is thus not writable.

The "commits" array is correctly marked const, but the result from bsearch() is returned directly as a non-const pseudo_merge_commit struct. Since new versions of glibc annotate bsearch() in a way that detects the implicit loss of const, the compiler now warns.

My first instinct was that we should be returning a const struct. That requires apply_pseudo_merges_for_commit() to mark its local pointer as const. But that doesn't work! If the offset field has the high-bit set, we look it up in the extended table via nth_pseudo_merge_ext(). And that function then feeds our const struct to read_pseudo_merge_commit_at(), which writes into it by byte-swapping from the on-disk mmap.

But I think this points to a larger problem with find_pseudo_merge(). It is not just that the return value is missing const, but it is missing that byte-swapping! And we know that byte-swapping is needed here, because the comparator we use for bsearch() also calls our read_pseudo_merge_commit_at() helper.

So I think the interface is all wrong here. We should not be returning a pointer to a struct which was cast from on-disk data. We should be filling in a caller-provided struct using the bytes we found, byte-swapping the values.

That of course raises the dual question: how did this ever work, and does it work now? The answer to the first part is: this code does not seem to be triggered in the test suite at all. If we insert a BUG("foo") call into apply_pseudo_merges_for_commit(), it never triggers.

So I think there is something wrong or missing from the test setup, and this bears further investigation. Sadly the answer to the second part ("does it work now") is still "no idea". I _think_ this takes us in a positive direction, but my goal here is mainly to quiet the compiler warning. Further bug-hunting on this experimental feature can be done separately.

Signed-off-by: Jeff King <peff@peff.net>
---
 pseudo-merge.c | 32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

The skip_prefix() function takes in a "const char *" string, and returns via a "const char **" out-parameter that points somewhere in that string. This is fine if you are operating on a const string, like:

  const char *in = ...;
  const char *out;
  if (skip_prefix(in, "foo", &out))
	...look at out...

It is also OK if "in" is not const but "out" is, as we add an implicit const when we pass "in" to the function. But there's another case where this is limiting. If we want both fields to be non-const, like:

  char *in = ...;
  char *out;
  if (skip_prefix(in, "foo", &out))
	*out = '\0';

it doesn't work. The compiler will complain about the type mismatch in passing "&out" to a parameter which expects "const char **". So to make this work, we have to do an explicit cast.

But such a cast is ugly, and also means that we run afoul of making this mistake:

  const char *in = ...;
  char *out;
  if (skip_prefix(in, "foo", (const char **)&out))
	*out = '\0';

which causes us to write to the memory pointed by "in", which was const.

We can imagine these four cases as:

  (1) const in, const out
  (2) non-const in, const out
  (3) non-const in, non-const out
  (4) const in, non-const out

Cases (1) and (2) work now. We would like case (3) to work but it doesn't. But we would like to catch case (4) as a compile error.

So ideally the rule is "the out-parameter must be at least as const as the in-parameter". We can do this with some macro trickery. We wrap skip_prefix() in a macro so that it has access to the real types of in/out. And then we pass those parameters through another macro which:

  1. Fails if the "at least as const" rule is not filled.

  2. Casts to match the signature of the real skip_prefix().

There are a lot of ways to implement the "fails" part. You can use __builtin_types_compatible_p() to check, and then either our BUILD_ASSERT macros or _Static_assert to fail. But that requires some conditional compilation based on compiler feature. That's probably OK (the fallback would be to just cast without catching case 4). But we can do better.

The macro I have here uses a ternary with a dead branch that tries to assign "in" to "out", which should work everywhere and lets the compiler catch the problem in the usual way. With an input like this:

  int foo(const char *x, const char **y);
  #define foo(in,out) foo((in), CONST_OUTPARAM((in), (out)))

  void ok_const(const char *x, const char **y)
  {
          foo(x, y);
  }

  void ok_nonconst(char *x, char **y)
  {
          foo(x, y);
  }

  void ok_add_const(char *x, const char **y)
  {
          foo(x, y);
  }

  void bad_drop_const(const char *x, char **y)
  {
          foo(x, y);
  }

gcc reports:

  foo.c: In function ‘bad_drop_const’:
  foo.c:2:35: error: assignment discards ‘const’ qualifier from pointer target type [-Werror=discarded-qualifiers]
      2 |     ((const char **)(0 ? ((*(out) = (in)),(out)) : (out)))
        |                                   ^
  foo.c:4:31: note: in expansion of macro ‘CONST_OUTPARAM’
      4 | #define foo(in,out) foo((in), CONST_OUTPARAM((in), (out)))
        |                               ^~~~~~~~~~~~~~
  foo.c:23:9: note: in expansion of macro ‘foo’
     23 |         foo(x, y);
        |         ^~~

It's a bit verbose, but I think makes it reasonably clear what's going on. Using BUILD_ASSERT_OR_ZERO() ends up much worse. Using _Static_assert you can be a bit more informative, but that's not something we use at all yet in our code-base (it's an old gnu-ism later standardized in C11).

Our generic macro only works for "const char **", which is something we could improve by using typeof(in). But that introduces more portability questions, and also some weird corner cases (e.g., around implicit void conversion).

This patch just introduces the concept. We'll make use of it in future patches.

Note that we rename skip_prefix() to skip_prefix_impl() here, to avoid expanding the macro when defining the function. That's not strictly necessary since we could just define the macro after defining the inline function. But that would not be the case for a non-inline function (and we will apply this technique to them later, and should be consistent). It also gives us more freedom about where to define the macro. I did so right above the definition here, which I think keeps the relevant bits together.

Signed-off-by: Jeff King <peff@peff.net>
---
 git-compat-util.h | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

The "line" member of a packet_reader struct is marked as const. This kind of makes sense, because it's not its own allocated buffer that should be freed, and we often use const to indicate that. But it is always writable, because it points into the non-const "buffer" member.

And we rely on this writability in places like send-pack and receive-pack, where we parse incoming packet contents by writing NULs over delimiters. This has traditionally worked because we implicitly cast away the constness with strchr() like:

  const char *head;
  char *p;

  head = reader->line;
  p = strchr(head, ' ');

Since C23 libc provides a generic strchr() to detect this implicit const removal, this now generate a compiler warning on some platforms (like recent glibc).

We can fix it by marking "line" as non-const, as well as a few intermediate variables (like "head" in the above example). Note that by itself, switching to a non-const variable would cause problems with this line in send-pack.c:

  if (!skip_prefix(reader->line, "unpack ", &reader->line))

But due to our skip_prefix() magic introduced in the previous commit, this compiles fine (both the in and out-parameters are non-const, so we know it is safe).

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/receive-pack.c | 7 ++++---
 pkt-line.h             | 2 +-
 send-pack.c            | 7 ++++---
 3 files changed, 9 insertions(+), 7 deletions(-)

This is another case where we implicitly drop the "const" from a pointer by feeding it to strstr() and assigning the result to a non-const pointer. This is OK in practice, since the const pointer originally comes from a writable source (a strbuf), but C23 libc implementations have started to complain about it.

We do write to the output pointer, so it needs to remain non-const. We can just switch the input pointer to also be non-const in this case. By itself that would run into problems with calls to skip_prefix(), but since that function has now been taught to match in/out constness automatically, it just works without us doing anything further.

Signed-off-by: Jeff King <peff@peff.net>
---
 range-diff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

In redact_sensitive_header(), a C23 implementation of libc will complain that strstr() assigns the result from "const char *cookie" to "char *semicolon".

Ultimately the memory is writable. We're fed a strbuf, generate a const pointer "sensitive_header" within it using skip_iprefix(), and then assign the result to "cookie". So we can solve this by dropping the const from "cookie" and "sensitive_header".

However, this runs afoul of skip_iprefix(), which wants a "const char **" for its out-parameter. We can solve that by teaching skip_iprefix() the same "make sure out is at least as const as in" magic that we recently taught to skip_prefix().

Signed-off-by: Jeff King <peff@peff.net>
---
 git-compat-util.h | 6 ++++--
 http.c            | 4 ++--
 2 files changed, 6 insertions(+), 4 deletions(-)

In show_one_reflog_ent(), we're fed a writable strbuf buffer, which we parse into the various reflog components. We write a NUL over email_end to tie off one of the fields, and thus email_end must be non-const.

But with a C23 implementation of libc, strchr() will now complain when assigning the result to a non-const pointer from a const one. So we can fix this by making the source pointer non-const.

But there's a catch. We derive that source pointer by parsing the line with parse_oid_hex_algop(), which requires a const pointer for its out-parameter. We can work around that by teaching it to use our CONST_OUTPARAM() trick, just like skip_prefix().

Signed-off-by: Jeff King <peff@peff.net>
---
 hex.c                | 6 +++---
 hex.h                | 6 ++++--
 refs/files-backend.c | 2 +-
 3 files changed, 8 insertions(+), 6 deletions(-)

Jeff King <peff@peff.net> writes:

Sorry for not mentioning earlier, but I couldn't quite parse the above with "that" immediately after "out-parameter". I am guessing that you wanted to say an equivalent of

    Check that an out-parameter "out" is at least as const as a
    matching in-parameter "in".

On Wed, Apr 01, 2026 at 10:11:45PM -0700, Junio C Hamano wrote:

Yes, it's just a typo. What you wrote is what I meant. Can you fix it up while applying?

-Peff

On 01/04/2026 20:24, Jeff King wrote:

I agree it's well defined and because this code is never executed we don't need to worry about evaluating "out" multiple times. While the message from using _Static_assert below is nicer I think having this which is simple and works with all compilers is a better trade off.

Thanks

Phillip

Junio C Hamano <gitster@pobox.com> writes:

What I meant was that I'd understand if that "that" immediately after the "out-parameter" is removed from the sentence.

Thanks.

Jeff King <peff@peff.net> writes:

Will do.  Thanks.

On Tue, Mar 31, 2026 at 07:56:37PM -0400, Jeff King wrote:

This approach makes the most sense to me as a band-aid fix to squelch the Coverity warnings.

I pulled on this thread a little bit over the past couple of evenings and found a fair number of pseudo-merge related bugs / oddities that I'd like to fix more comprehensively.

But this is makes sense as a first step to quiet the noise from Coverity without rushing the other fixes.

Thanks, Taylor

Jeff King <peff@peff.net> writes:

I didn't know this pattern before, but is seems it's used in other places as well. Clever.

I'm not sure it matters, but this is indented with spaces

Jeff King <peff@peff.net> writes:

Thanks for all these fixes. On top of the fixes you've made that are already in `next`, this makes my compiler and me happy.

I've posted one nit about indenting, but other than that, all looks good to me.

On Fri, Apr 03, 2026 at 01:13:11PM +0200, Toon Claes wrote:

This is in 'next', so we'd need a patch on top. Maybe not worth it on its own, but we also missed the typo-fix that Junio pointed out earlier.

So maybe this on top:

-- >8 --
Subject: [PATCH] git-compat-util: fix CONST_OUTPARAM typo and indentation

There's a typo in the comment, making it hard to understand. And the macro itself is indented with spaces rather than tab.

Signed-off-by: Jeff King <peff@peff.net>
---
 git-compat-util.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)