[PATCH 0/12] fixing the remainder of the C23 strchr warnings

[PATCH 0/12] fixing the remainder of the C23 strchr warnings

[Jeff King]

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

[PATCH 01/12] convert: add const to fix strchr() warnings

[Jeff King]

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(-)

diff --git a/convert.c b/convert.c
index a34ec6ecdc..eae36c8a59 100644
--- a/convert.c
+++ b/convert.c
@@ -1168,7 +1168,8 @@ static int ident_to_worktree(const char *src, size_t len,
 			     struct strbuf *buf, int ident)
 {
 	struct object_id oid;
-	char *to_free = NULL, *dollar, *spc;
+	char *to_free = NULL;
+	const char *dollar, *spc;
 	int cnt;
 
 	if (!ident)
-- 
2.53.0.1136.gd760fbd4a0

[PATCH 02/12] http: add const to fix strchr() warnings

[Jeff King]

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(-)

diff --git a/http-push.c b/http-push.c
index 9ae6062198..96df6344ee 100644
--- a/http-push.c
+++ b/http-push.c
@@ -99,7 +99,7 @@ static struct object_list *objects;
 
 struct repo {
 	char *url;
-	char *path;
+	const char *path;
 	int path_len;
 	int has_info_refs;
 	int can_update_info_refs;
-- 
2.53.0.1136.gd760fbd4a0

[PATCH 03/12] transport-helper: drop const to fix strchr() warnings

[Jeff King]

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(-)

diff --git a/transport-helper.c b/transport-helper.c
index 4d95d84f9e..4614036c99 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -781,7 +781,8 @@ static int push_update_ref_status(struct strbuf *buf,
 
 	if (starts_with(buf->buf, "option ")) {
 		struct object_id old_oid, new_oid;
-		const char *key, *val;
+		char *key;
+		const char *val;
 		char *p;
 
 		if (!state->hint || !(state->report || state->new_report))
-- 
2.53.0.1136.gd760fbd4a0

[PATCH 04/12] pager: explicitly cast away strchr() constness

[Jeff King]

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(-)

diff --git a/pager.c b/pager.c
index 5531fff50e..801ba392f2 100644
--- a/pager.c
+++ b/pager.c
@@ -118,7 +118,8 @@ static void setup_pager_env(struct strvec *env)
 			split_cmdline_strerror(n));
 
 	for (i = 0; i < n; i++) {
-		char *cp = strchr(argv[i], '=');
+		/* we know this is writable because it was split from pager_env */
+		char *cp = strchr((char *)argv[i], '=');
 
 		if (!cp)
 			die("malformed build-time PAGER_ENV");
-- 
2.53.0.1136.gd760fbd4a0

[PATCH 05/12] run-command: explicitly cast away constness when assigning to void

[Jeff King]

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(-)

diff --git a/run-command.c b/run-command.c
index 32c290ee6a..d6980c79b3 100644
--- a/run-command.c
+++ b/run-command.c
@@ -604,11 +604,11 @@ static void trace_add_env(struct strbuf *dst, const char *const *deltaenv)
 	/* Last one wins, see run-command.c:prep_childenv() for context */
 	for (e = deltaenv; e && *e; e++) {
 		struct strbuf key = STRBUF_INIT;
-		char *equals = strchr(*e, '=');
+		const char *equals = strchr(*e, '=');
 
 		if (equals) {
 			strbuf_add(&key, *e, equals - *e);
-			string_list_insert(&envs, key.buf)->util = equals + 1;
+			string_list_insert(&envs, key.buf)->util = (void *)(equals + 1);
 		} else {
 			string_list_insert(&envs, *e)->util = NULL;
 		}
-- 
2.53.0.1136.gd760fbd4a0

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

[Jeff King]

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(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index 4b4ea2498f..4bb59b3101 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -335,11 +335,7 @@ static inline int is_path_owned_by_current_uid(const char *path,
 #endif
 
 #ifndef find_last_dir_sep
-static inline char *git_find_last_dir_sep(const char *path)
-{
-	return strrchr(path, '/');
-}
-#define find_last_dir_sep git_find_last_dir_sep
+#define find_last_dir_sep(path) strrchr((path), '/')
 #endif
 
 #ifndef has_dir_sep
-- 
2.53.0.1136.gd760fbd4a0

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

[Jeff King]

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(-)

diff --git a/pseudo-merge.c b/pseudo-merge.c
index a2d5bd85f9..ff18b6c364 100644
--- a/pseudo-merge.c
+++ b/pseudo-merge.c
@@ -638,31 +638,37 @@ static int pseudo_merge_commit_cmp(const void *va, const void *vb)
 	return 0;
 }
 
-static struct pseudo_merge_commit *find_pseudo_merge(const struct pseudo_merge_map *pm,
-						     uint32_t pos)
+static int find_pseudo_merge(const struct pseudo_merge_map *pm, uint32_t pos,
+			     struct pseudo_merge_commit *out)
 {
+	const unsigned char *at;
+
 	if (!pm->commits_nr)
-		return NULL;
+		return 0;
 
-	return bsearch(&pos, pm->commits, pm->commits_nr,
-		       PSEUDO_MERGE_COMMIT_RAWSZ, pseudo_merge_commit_cmp);
+	at = bsearch(&pos, pm->commits, pm->commits_nr,
+		     PSEUDO_MERGE_COMMIT_RAWSZ, pseudo_merge_commit_cmp);
+	if (!at)
+		return 0;
+
+	read_pseudo_merge_commit_at(out, at);
+	return 1;
 }
 
 int apply_pseudo_merges_for_commit(const struct pseudo_merge_map *pm,
 				   struct bitmap *result,
 				   struct commit *commit, uint32_t commit_pos)
 {
 	struct pseudo_merge *merge;
-	struct pseudo_merge_commit *merge_commit;
+	struct pseudo_merge_commit merge_commit;
 	int ret = 0;
 
-	merge_commit = find_pseudo_merge(pm, commit_pos);
-	if (!merge_commit)
+	if (!find_pseudo_merge(pm, commit_pos, &merge_commit))
 		return 0;
 
-	if (merge_commit->pseudo_merge_ofs & ((uint64_t)1<<63)) {
+	if (merge_commit.pseudo_merge_ofs & ((uint64_t)1<<63)) {
 		struct pseudo_merge_commit_ext ext = { 0 };
-		off_t ofs = merge_commit->pseudo_merge_ofs & ~((uint64_t)1<<63);
+		off_t ofs = merge_commit.pseudo_merge_ofs & ~((uint64_t)1<<63);
 		uint32_t i;
 
 		if (pseudo_merge_ext_at(pm, &ext, ofs) < -1) {
@@ -673,11 +679,11 @@ int apply_pseudo_merges_for_commit(const struct pseudo_merge_map *pm,
 		}
 
 		for (i = 0; i < ext.nr; i++) {
-			if (nth_pseudo_merge_ext(pm, &ext, merge_commit, i) < 0)
+			if (nth_pseudo_merge_ext(pm, &ext, &merge_commit, i) < 0)
 				return ret;
 
 			merge = pseudo_merge_at(pm, &commit->object.oid,
-						merge_commit->pseudo_merge_ofs);
+						merge_commit.pseudo_merge_ofs);
 
 			if (!merge)
 				return ret;
@@ -687,7 +693,7 @@ int apply_pseudo_merges_for_commit(const struct pseudo_merge_map *pm,
 		}
 	} else {
 		merge = pseudo_merge_at(pm, &commit->object.oid,
-					merge_commit->pseudo_merge_ofs);
+					merge_commit.pseudo_merge_ofs);
 
 		if (!merge)
 			return ret;
-- 
2.53.0.1136.gd760fbd4a0

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

[Jeff King]

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(+)

diff --git a/git-compat-util.h b/git-compat-util.h
index 4bb59b3101..58e494e037 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -491,6 +491,23 @@ static inline bool skip_prefix(const char *str, const char *prefix,
 	return false;
 }
 
+/*
+ * Check that an out-parameter that is "at least as const as" a matching
+ * in-parameter. For example, skip_prefix() will return "out" that is a subset
+ * of "str". So:
+ *
+ *  const str, const out: ok
+ *  non-const str, const out: ok
+ *  non-const str, non-const out: ok
+ *  const str, non-const out: compile error
+ *
+ *  See the skip_prefix macro below for an example of use.
+ */
+#define CONST_OUTPARAM(in, out) \
+    ((const char **)(0 ? ((*(out) = (in)),(out)) : (out)))
+#define skip_prefix(str, prefix, out) \
+	skip_prefix((str), (prefix), CONST_OUTPARAM((str), (out)))
+
 /*
  * Like skip_prefix, but promises never to read past "len" bytes of the input
  * buffer, and returns the remaining number of bytes in "out" via "outlen".
-- 
2.53.0.1136.gd760fbd4a0

[PATCH 09/12] pkt-line: make packet_reader.line non-const

[Jeff King]

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(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index e34edff406..a6af16c4e7 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1025,8 +1025,8 @@ static int read_proc_receive_report(struct packet_reader *reader,
 
 	for (;;) {
 		struct object_id old_oid, new_oid;
-		const char *head;
-		const char *refname;
+		char *head;
+		char *refname;
 		char *p;
 		enum packet_read_status status;
 
@@ -1050,7 +1050,8 @@ static int read_proc_receive_report(struct packet_reader *reader,
 		}
 		*p++ = '\0';
 		if (!strcmp(head, "option")) {
-			const char *key, *val;
+			char *key;
+			const char *val;
 
 			if (!hint || !(report || new_report)) {
 				if (!once++)
diff --git a/pkt-line.h b/pkt-line.h
index 3b33cc64f3..e6cf85e34e 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -184,7 +184,7 @@ struct packet_reader {
 	int pktlen;
 
 	/* the last line read */
-	const char *line;
+	char *line;
 
 	/* indicates if a line has been peeked */
 	int line_peeked;
diff --git a/send-pack.c b/send-pack.c
index 07ecfae4de..b4361d5610 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -175,8 +175,8 @@ static int receive_status(struct repository *r,
 	ret = receive_unpack_status(reader);
 	while (1) {
 		struct object_id old_oid, new_oid;
-		const char *head;
-		const char *refname;
+		char *head;
+		char *refname;
 		char *p;
 		if (packet_reader_read(reader) != PACKET_READ_NORMAL)
 			break;
@@ -190,7 +190,8 @@ static int receive_status(struct repository *r,
 		*p++ = '\0';
 
 		if (!strcmp(head, "option")) {
-			const char *key, *val;
+			char *key;
+			const char *val;
 
 			if (!hint || !(report || new_report)) {
 				if (!once++)
-- 
2.53.0.1136.gd760fbd4a0

[PATCH 10/12] range-diff: drop const to fix strstr() warnings

[Jeff King]

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(-)

diff --git a/range-diff.c b/range-diff.c
index 2712a9a107..8e2dd2eb19 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -88,7 +88,7 @@ static int read_patches(const char *range, struct string_list *list,
 	line = contents.buf;
 	size = contents.len;
 	for (; size > 0; size -= len, line += len) {
-		const char *p;
+		char *p;
 		char *eol;
 
 		eol = memchr(line, '\n', size);
-- 
2.53.0.1136.gd760fbd4a0

[PATCH 11/12] http: drop const to fix strstr() warning

[Jeff King]

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(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index 58e494e037..f60793fc36 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -914,6 +914,9 @@ static inline bool skip_iprefix(const char *str, const char *prefix,
 	return false;
 }
 
+#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
diff --git a/http.c b/http.c
index 8ea1b9d1f6..8801bd22fe 100644
--- a/http.c
+++ b/http.c
@@ -726,7 +726,7 @@ static int has_proxy_cert_password(void)
 static int redact_sensitive_header(struct strbuf *header, size_t offset)
 {
 	int ret = 0;
-	const char *sensitive_header;
+	char *sensitive_header;
 
 	if (trace_curl_redact &&
 	    (skip_iprefix(header->buf + offset, "Authorization:", &sensitive_header) ||
@@ -743,7 +743,7 @@ static int redact_sensitive_header(struct strbuf *header, size_t offset)
 	} else if (trace_curl_redact &&
 		   skip_iprefix(header->buf + offset, "Cookie:", &sensitive_header)) {
 		struct strbuf redacted_header = STRBUF_INIT;
-		const char *cookie;
+		char *cookie;
 
 		while (isspace(*sensitive_header))
 			sensitive_header++;
-- 
2.53.0.1136.gd760fbd4a0

[PATCH 12/12] refs/files-backend: drop const to fix strchr() warning

[Jeff King]

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(-)

diff --git a/hex.c b/hex.c
index 865a232167..bc756722ca 100644
--- a/hex.c
+++ b/hex.c
@@ -54,9 +54,9 @@ int get_oid_hex(const char *hex, struct object_id *oid)
 	return get_oid_hex_algop(hex, oid, the_hash_algo);
 }
 
-int parse_oid_hex_algop(const char *hex, struct object_id *oid,
-			const char **end,
-			const struct git_hash_algo *algop)
+int parse_oid_hex_algop_impl(const char *hex, struct object_id *oid,
+			     const char **end,
+			     const struct git_hash_algo *algop)
 {
 	int ret = get_oid_hex_algop(hex, oid, algop);
 	if (!ret)
diff --git a/hex.h b/hex.h
index e9ccb54065..db7882bd17 100644
--- a/hex.h
+++ b/hex.h
@@ -40,8 +40,10 @@ char *oid_to_hex(const struct object_id *oid);						/* same static buffer */
  * other invalid character.  end is only updated on success; otherwise, it is
  * unmodified.
  */
-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))
 
 /*
  * These functions work like get_oid_hex and parse_oid_hex, but they will parse
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 7ce0d57478..ad543ad751 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2190,7 +2190,7 @@ static int show_one_reflog_ent(struct files_ref_store *refs,
 	char *email_end, *message;
 	timestamp_t timestamp;
 	int tz;
-	const char *p = sb->buf;
+	char *p = sb->buf;
 
 	/* old SP new SP name <email> SP time TAB msg LF */
 	if (!sb->len || sb->buf[sb->len - 1] != '\n' ||
-- 
2.53.0.1136.gd760fbd4a0

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

[Jeff King]

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

> 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.

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

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

[Phillip Wood]

Hi peff

On 01/04/2026 00:50, Jeff King wrote:
> 
> +/*
> + * Check that an out-parameter that is "at least as const as" a matching
> + * in-parameter. For example, skip_prefix() will return "out" that is a subset
> + * of "str". So:
> + *
> + *  const str, const out: ok
> + *  non-const str, const out: ok
> + *  non-const str, non-const out: ok
> + *  const str, non-const out: compile error
> + *
> + *  See the skip_prefix macro below for an example of use.
> + */
> +#define CONST_OUTPARAM(in, out) \
> +    ((const char **)(0 ? ((*(out) = (in)),(out)) : (out)))
> +#define skip_prefix(str, prefix, out) \
> +	skip_prefix((str), (prefix), CONST_OUTPARAM((str), (out)))

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

Re: [PATCH 03/12] transport-helper: drop const to fix strchr() warnings

[Patrick Steinhardt]

On Tue, Mar 31, 2026 at 07:41:48PM -0400, Jeff King wrote:
> 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

You probably mean NUL, not NULL.

Patrick

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

[Patrick Steinhardt]

On Tue, Mar 31, 2026 at 07:50:17PM -0400, Jeff King wrote:
> diff --git a/git-compat-util.h b/git-compat-util.h
> index 4bb59b3101..58e494e037 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -491,6 +491,23 @@ static inline bool skip_prefix(const char *str, const char *prefix,
>  	return false;
>  }
>  
> +/*
> + * Check that an out-parameter that is "at least as const as" a matching
> + * in-parameter. For example, skip_prefix() will return "out" that is a subset
> + * of "str". So:
> + *
> + *  const str, const out: ok
> + *  non-const str, const out: ok
> + *  non-const str, non-const out: ok
> + *  const str, non-const out: compile error
> + *
> + *  See the skip_prefix macro below for an example of use.
> + */
> +#define CONST_OUTPARAM(in, out) \
> +    ((const char **)(0 ? ((*(out) = (in)),(out)) : (out)))
> +#define skip_prefix(str, prefix, out) \
> +	skip_prefix((str), (prefix), CONST_OUTPARAM((str), (out)))

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

Re: [PATCH 12/12] refs/files-backend: drop const to fix strchr() warning

[Patrick Steinhardt]

On Tue, Mar 31, 2026 at 07:53:41PM -0400, Jeff King wrote:
> 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.

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

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

[Phillip Wood]

On 01/04/2026 14:17, Phillip Wood wrote:
> On 01/04/2026 00:50, Jeff King wrote:
>>
>> +/*
>> + * Check that an out-parameter that is "at least as const as" a matching
>> + * in-parameter. For example, skip_prefix() will return "out" that is 
>> a subset
>> + * of "str". So:
>> + *
>> + *  const str, const out: ok
>> + *  non-const str, const out: ok
>> + *  non-const str, non-const out: ok
>> + *  const str, non-const out: compile error
>> + *
>> + *  See the skip_prefix macro below for an example of use.
>> + */
>> +#define CONST_OUTPARAM(in, out) \
>> +    ((const char **)(0 ? ((*(out) = (in)),(out)) : (out)))
>> +#define skip_prefix(str, prefix, out) \
>> +    skip_prefix((str), (prefix), CONST_OUTPARAM((str), (out)))
> 
> This is clever but it changes the behavior of skip_prefix() which is 
> documented as not touching out if it returns false.

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

> 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
> 
> 

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

[Jeff King]

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

> On 01/04/2026 14:17, Phillip Wood wrote:
> > On 01/04/2026 00:50, Jeff King wrote:
> > > 
> > > +/*
> > > + * Check that an out-parameter that is "at least as const as" a matching
> > > + * in-parameter. For example, skip_prefix() will return "out" that
> > > is a subset
> > > + * of "str". So:
> > > + *
> > > + *  const str, const out: ok
> > > + *  non-const str, const out: ok
> > > + *  non-const str, non-const out: ok
> > > + *  const str, non-const out: compile error
> > > + *
> > > + *  See the skip_prefix macro below for an example of use.
> > > + */
> > > +#define CONST_OUTPARAM(in, out) \
> > > +    ((const char **)(0 ? ((*(out) = (in)),(out)) : (out)))
> > > +#define skip_prefix(str, prefix, out) \
> > > +    skip_prefix((str), (prefix), CONST_OUTPARAM((str), (out)))
> > 
> > This is clever but it changes the behavior of skip_prefix() which is
> > documented as not touching out if it returns false.
> 
> 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.

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

Re: [PATCH 04/12] pager: explicitly cast away strchr() constness

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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().

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.



>
> 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(-)
>
> diff --git a/pager.c b/pager.c
> index 5531fff50e..801ba392f2 100644
> --- a/pager.c
> +++ b/pager.c
> @@ -118,7 +118,8 @@ static void setup_pager_env(struct strvec *env)
>  			split_cmdline_strerror(n));
>  
>  	for (i = 0; i < n; i++) {
> -		char *cp = strchr(argv[i], '=');
> +		/* we know this is writable because it was split from pager_env */
> +		char *cp = strchr((char *)argv[i], '=');
>  
>  		if (!cp)
>  			die("malformed build-time PAGER_ENV");

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Mar 31, 2026 at 07:46:23PM -0400, Jeff King wrote:
>
>> 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.
>
> 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. ;)

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> > > +#define CONST_OUTPARAM(in, out) \
>> > > +    ((const char **)(0 ? ((*(out) = (in)),(out)) : (out)))
>> > > +#define skip_prefix(str, prefix, out) \
>> > > +    skip_prefix((str), (prefix), CONST_OUTPARAM((str), (out)))
>> > 
>> > This is clever but it changes the behavior of skip_prefix() which is
>> > documented as not touching out if it returns false.
>> 
>> 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.
>
> 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).

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.

Re: [PATCH 09/12] pkt-line: make packet_reader.line non-const

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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.

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)?

> 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).

OK.

Re: [PATCH 12/12] refs/files-backend: drop const to fix strchr() warning

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> On Tue, Mar 31, 2026 at 07:53:41PM -0400, Jeff King wrote:
>> 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.
>
> 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.

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.

Re: [PATCH 04/12] pager: explicitly cast away strchr() constness

[Jeff King]

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

> Jeff King <peff@peff.net> writes:
> 
> > 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().
> 
> 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.

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

Re: [PATCH 09/12] pkt-line: make packet_reader.line non-const

[Jeff King]

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

> Jeff King <peff@peff.net> writes:
> 
> > 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.
> 
> 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)?

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

Re: [PATCH 12/12] refs/files-backend: drop const to fix strchr() warning

[Jeff King]

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

> >> 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.
> >
> > 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.
> 
> 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.

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

[PATCH v2 0/12] fixing the remainder of the C23 strchr warnings

[Jeff King]

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

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

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

[PATCH v2 01/12] convert: add const to fix strchr() warnings

[Jeff King]

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(-)

diff --git a/convert.c b/convert.c
index a34ec6ecdc..eae36c8a59 100644
--- a/convert.c
+++ b/convert.c
@@ -1168,7 +1168,8 @@ static int ident_to_worktree(const char *src, size_t len,
 			     struct strbuf *buf, int ident)
 {
 	struct object_id oid;
-	char *to_free = NULL, *dollar, *spc;
+	char *to_free = NULL;
+	const char *dollar, *spc;
 	int cnt;
 
 	if (!ident)
-- 
2.53.0.1172.ge9e20b5838

[PATCH v2 02/12] http: add const to fix strchr() warnings

[Jeff King]

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(-)

diff --git a/http-push.c b/http-push.c
index 9ae6062198..96df6344ee 100644
--- a/http-push.c
+++ b/http-push.c
@@ -99,7 +99,7 @@ static struct object_list *objects;
 
 struct repo {
 	char *url;
-	char *path;
+	const char *path;
 	int path_len;
 	int has_info_refs;
 	int can_update_info_refs;
-- 
2.53.0.1172.ge9e20b5838

[PATCH v2 03/12] transport-helper: drop const to fix strchr() warnings

[Jeff King]

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(-)

diff --git a/transport-helper.c b/transport-helper.c
index 4d95d84f9e..4614036c99 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -781,7 +781,8 @@ static int push_update_ref_status(struct strbuf *buf,
 
 	if (starts_with(buf->buf, "option ")) {
 		struct object_id old_oid, new_oid;
-		const char *key, *val;
+		char *key;
+		const char *val;
 		char *p;
 
 		if (!state->hint || !(state->report || state->new_report))
-- 
2.53.0.1172.ge9e20b5838

[PATCH v2 04/12] pager: explicitly cast away strchr() constness

[Jeff King]

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(-)

diff --git a/pager.c b/pager.c
index 5531fff50e..35b210e048 100644
--- a/pager.c
+++ b/pager.c
@@ -108,10 +108,11 @@ const char *git_pager(struct repository *r, int stdout_is_tty)
 
 static void setup_pager_env(struct strvec *env)
 {
-	const char **argv;
+	char **argv;
 	int i;
 	char *pager_env = xstrdup(PAGER_ENV);
-	int n = split_cmdline(pager_env, &argv);
+	/* split_cmdline splits in place, so we know the result is writable */
+	int n = split_cmdline(pager_env, (const char ***)&argv);
 
 	if (n < 0)
 		die("malformed build-time PAGER_ENV: %s",
-- 
2.53.0.1172.ge9e20b5838

[PATCH v2 05/12] run-command: explicitly cast away constness when assigning to void

[Jeff King]

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(-)

diff --git a/run-command.c b/run-command.c
index 32c290ee6a..d6980c79b3 100644
--- a/run-command.c
+++ b/run-command.c
@@ -604,11 +604,11 @@ static void trace_add_env(struct strbuf *dst, const char *const *deltaenv)
 	/* Last one wins, see run-command.c:prep_childenv() for context */
 	for (e = deltaenv; e && *e; e++) {
 		struct strbuf key = STRBUF_INIT;
-		char *equals = strchr(*e, '=');
+		const char *equals = strchr(*e, '=');
 
 		if (equals) {
 			strbuf_add(&key, *e, equals - *e);
-			string_list_insert(&envs, key.buf)->util = equals + 1;
+			string_list_insert(&envs, key.buf)->util = (void *)(equals + 1);
 		} else {
 			string_list_insert(&envs, *e)->util = NULL;
 		}
-- 
2.53.0.1172.ge9e20b5838

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

[Jeff King]

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(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index 4b4ea2498f..4bb59b3101 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -335,11 +335,7 @@ static inline int is_path_owned_by_current_uid(const char *path,
 #endif
 
 #ifndef find_last_dir_sep
-static inline char *git_find_last_dir_sep(const char *path)
-{
-	return strrchr(path, '/');
-}
-#define find_last_dir_sep git_find_last_dir_sep
+#define find_last_dir_sep(path) strrchr((path), '/')
 #endif
 
 #ifndef has_dir_sep
-- 
2.53.0.1172.ge9e20b5838

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

[Jeff King]

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(-)

diff --git a/pseudo-merge.c b/pseudo-merge.c
index a2d5bd85f9..ff18b6c364 100644
--- a/pseudo-merge.c
+++ b/pseudo-merge.c
@@ -638,31 +638,37 @@ static int pseudo_merge_commit_cmp(const void *va, const void *vb)
 	return 0;
 }
 
-static struct pseudo_merge_commit *find_pseudo_merge(const struct pseudo_merge_map *pm,
-						     uint32_t pos)
+static int find_pseudo_merge(const struct pseudo_merge_map *pm, uint32_t pos,
+			     struct pseudo_merge_commit *out)
 {
+	const unsigned char *at;
+
 	if (!pm->commits_nr)
-		return NULL;
+		return 0;
 
-	return bsearch(&pos, pm->commits, pm->commits_nr,
-		       PSEUDO_MERGE_COMMIT_RAWSZ, pseudo_merge_commit_cmp);
+	at = bsearch(&pos, pm->commits, pm->commits_nr,
+		     PSEUDO_MERGE_COMMIT_RAWSZ, pseudo_merge_commit_cmp);
+	if (!at)
+		return 0;
+
+	read_pseudo_merge_commit_at(out, at);
+	return 1;
 }
 
 int apply_pseudo_merges_for_commit(const struct pseudo_merge_map *pm,
 				   struct bitmap *result,
 				   struct commit *commit, uint32_t commit_pos)
 {
 	struct pseudo_merge *merge;
-	struct pseudo_merge_commit *merge_commit;
+	struct pseudo_merge_commit merge_commit;
 	int ret = 0;
 
-	merge_commit = find_pseudo_merge(pm, commit_pos);
-	if (!merge_commit)
+	if (!find_pseudo_merge(pm, commit_pos, &merge_commit))
 		return 0;
 
-	if (merge_commit->pseudo_merge_ofs & ((uint64_t)1<<63)) {
+	if (merge_commit.pseudo_merge_ofs & ((uint64_t)1<<63)) {
 		struct pseudo_merge_commit_ext ext = { 0 };
-		off_t ofs = merge_commit->pseudo_merge_ofs & ~((uint64_t)1<<63);
+		off_t ofs = merge_commit.pseudo_merge_ofs & ~((uint64_t)1<<63);
 		uint32_t i;
 
 		if (pseudo_merge_ext_at(pm, &ext, ofs) < -1) {
@@ -673,11 +679,11 @@ int apply_pseudo_merges_for_commit(const struct pseudo_merge_map *pm,
 		}
 
 		for (i = 0; i < ext.nr; i++) {
-			if (nth_pseudo_merge_ext(pm, &ext, merge_commit, i) < 0)
+			if (nth_pseudo_merge_ext(pm, &ext, &merge_commit, i) < 0)
 				return ret;
 
 			merge = pseudo_merge_at(pm, &commit->object.oid,
-						merge_commit->pseudo_merge_ofs);
+						merge_commit.pseudo_merge_ofs);
 
 			if (!merge)
 				return ret;
@@ -687,7 +693,7 @@ int apply_pseudo_merges_for_commit(const struct pseudo_merge_map *pm,
 		}
 	} else {
 		merge = pseudo_merge_at(pm, &commit->object.oid,
-					merge_commit->pseudo_merge_ofs);
+					merge_commit.pseudo_merge_ofs);
 
 		if (!merge)
 			return ret;
-- 
2.53.0.1172.ge9e20b5838

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

[Jeff King]

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(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index 4bb59b3101..e9629b2a9d 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -463,6 +463,21 @@ void set_warn_routine(report_fn routine);
 report_fn get_warn_routine(void);
 void set_die_is_recursing_routine(int (*routine)(void));
 
+/*
+ * Check that an out-parameter that is "at least as const as" a matching
+ * in-parameter. For example, skip_prefix() will return "out" that is a subset
+ * of "str". So:
+ *
+ *  const str, const out: ok
+ *  non-const str, const out: ok
+ *  non-const str, non-const out: ok
+ *  const str, non-const out: compile error
+ *
+ *  See the skip_prefix macro below for an example of use.
+ */
+#define CONST_OUTPARAM(in, out) \
+    ((const char **)(0 ? ((*(out) = (in)),(out)) : (out)))
+
 /*
  * If the string "str" begins with the string found in "prefix", return true.
  * The "out" parameter is set to "str + strlen(prefix)" (i.e., to the point in
@@ -479,8 +494,10 @@ void set_die_is_recursing_routine(int (*routine)(void));
  *   [skip prefix if present, otherwise use whole string]
  *   skip_prefix(name, "refs/heads/", &name);
  */
-static inline bool skip_prefix(const char *str, const char *prefix,
-			       const char **out)
+#define skip_prefix(str, prefix, out) \
+	skip_prefix_impl((str), (prefix), CONST_OUTPARAM((str), (out)))
+static inline bool skip_prefix_impl(const char *str, const char *prefix,
+				    const char **out)
 {
 	do {
 		if (!*prefix) {
-- 
2.53.0.1172.ge9e20b5838

[PATCH v2 09/12] pkt-line: make packet_reader.line non-const

[Jeff King]

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(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index e34edff406..a6af16c4e7 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1025,8 +1025,8 @@ static int read_proc_receive_report(struct packet_reader *reader,
 
 	for (;;) {
 		struct object_id old_oid, new_oid;
-		const char *head;
-		const char *refname;
+		char *head;
+		char *refname;
 		char *p;
 		enum packet_read_status status;
 
@@ -1050,7 +1050,8 @@ static int read_proc_receive_report(struct packet_reader *reader,
 		}
 		*p++ = '\0';
 		if (!strcmp(head, "option")) {
-			const char *key, *val;
+			char *key;
+			const char *val;
 
 			if (!hint || !(report || new_report)) {
 				if (!once++)
diff --git a/pkt-line.h b/pkt-line.h
index 3b33cc64f3..e6cf85e34e 100644
--- a/pkt-line.h
+++ b/pkt-line.h
@@ -184,7 +184,7 @@ struct packet_reader {
 	int pktlen;
 
 	/* the last line read */
-	const char *line;
+	char *line;
 
 	/* indicates if a line has been peeked */
 	int line_peeked;
diff --git a/send-pack.c b/send-pack.c
index 07ecfae4de..b4361d5610 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -175,8 +175,8 @@ static int receive_status(struct repository *r,
 	ret = receive_unpack_status(reader);
 	while (1) {
 		struct object_id old_oid, new_oid;
-		const char *head;
-		const char *refname;
+		char *head;
+		char *refname;
 		char *p;
 		if (packet_reader_read(reader) != PACKET_READ_NORMAL)
 			break;
@@ -190,7 +190,8 @@ static int receive_status(struct repository *r,
 		*p++ = '\0';
 
 		if (!strcmp(head, "option")) {
-			const char *key, *val;
+			char *key;
+			const char *val;
 
 			if (!hint || !(report || new_report)) {
 				if (!once++)
-- 
2.53.0.1172.ge9e20b5838

[PATCH v2 10/12] range-diff: drop const to fix strstr() warnings

[Jeff King]

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(-)

diff --git a/range-diff.c b/range-diff.c
index 2712a9a107..8e2dd2eb19 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -88,7 +88,7 @@ static int read_patches(const char *range, struct string_list *list,
 	line = contents.buf;
 	size = contents.len;
 	for (; size > 0; size -= len, line += len) {
-		const char *p;
+		char *p;
 		char *eol;
 
 		eol = memchr(line, '\n', size);
-- 
2.53.0.1172.ge9e20b5838

[PATCH v2 11/12] http: drop const to fix strstr() warning

[Jeff King]

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(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index e9629b2a9d..4ddac61992 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -902,8 +902,10 @@ 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_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) {
diff --git a/http.c b/http.c
index d8d016891b..67c9c6fc60 100644
--- a/http.c
+++ b/http.c
@@ -748,7 +748,7 @@ static int has_proxy_cert_password(void)
 static int redact_sensitive_header(struct strbuf *header, size_t offset)
 {
 	int ret = 0;
-	const char *sensitive_header;
+	char *sensitive_header;
 
 	if (trace_curl_redact &&
 	    (skip_iprefix(header->buf + offset, "Authorization:", &sensitive_header) ||
@@ -765,7 +765,7 @@ static int redact_sensitive_header(struct strbuf *header, size_t offset)
 	} else if (trace_curl_redact &&
 		   skip_iprefix(header->buf + offset, "Cookie:", &sensitive_header)) {
 		struct strbuf redacted_header = STRBUF_INIT;
-		const char *cookie;
+		char *cookie;
 
 		while (isspace(*sensitive_header))
 			sensitive_header++;
-- 
2.53.0.1172.ge9e20b5838

[PATCH v2 12/12] refs/files-backend: drop const to fix strchr() warning

[Jeff King]

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(-)

diff --git a/hex.c b/hex.c
index 865a232167..bc756722ca 100644
--- a/hex.c
+++ b/hex.c
@@ -54,9 +54,9 @@ int get_oid_hex(const char *hex, struct object_id *oid)
 	return get_oid_hex_algop(hex, oid, the_hash_algo);
 }
 
-int parse_oid_hex_algop(const char *hex, struct object_id *oid,
-			const char **end,
-			const struct git_hash_algo *algop)
+int parse_oid_hex_algop_impl(const char *hex, struct object_id *oid,
+			     const char **end,
+			     const struct git_hash_algo *algop)
 {
 	int ret = get_oid_hex_algop(hex, oid, algop);
 	if (!ret)
diff --git a/hex.h b/hex.h
index e9ccb54065..1e9a65d83a 100644
--- a/hex.h
+++ b/hex.h
@@ -40,8 +40,10 @@ char *oid_to_hex(const struct object_id *oid);						/* same static buffer */
  * other invalid character.  end is only updated on success; otherwise, it is
  * unmodified.
  */
-int parse_oid_hex_algop(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
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 0537a72b2a..b3b0c25f84 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2190,7 +2190,7 @@ static int show_one_reflog_ent(struct files_ref_store *refs,
 	char *email_end, *message;
 	timestamp_t timestamp;
 	int tz;
-	const char *p = sb->buf;
+	char *p = sb->buf;
 
 	/* old SP new SP name <email> SP time TAB msg LF */
 	if (!sb->len || sb->buf[sb->len - 1] != '\n' ||
-- 
2.53.0.1172.ge9e20b5838

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> +/*
> + * Check that an out-parameter that is "at least as const as" a matching
> + * in-parameter. For example, skip_prefix() will return "out" that is a subset
> + * of "str". So:

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".

> + *
> + *  const str, const out: ok
> + *  non-const str, const out: ok
> + *  non-const str, non-const out: ok
> + *  const str, non-const out: compile error
> + *
> + *  See the skip_prefix macro below for an example of use.
> + */
> +#define CONST_OUTPARAM(in, out) \
> +    ((const char **)(0 ? ((*(out) = (in)),(out)) : (out)))
> +
>  /*
>   * If the string "str" begins with the string found in "prefix", return true.
>   * The "out" parameter is set to "str + strlen(prefix)" (i.e., to the point in
> @@ -479,8 +494,10 @@ void set_die_is_recursing_routine(int (*routine)(void));
>   *   [skip prefix if present, otherwise use whole string]
>   *   skip_prefix(name, "refs/heads/", &name);
>   */
> -static inline bool skip_prefix(const char *str, const char *prefix,
> -			       const char **out)
> +#define skip_prefix(str, prefix, out) \
> +	skip_prefix_impl((str), (prefix), CONST_OUTPARAM((str), (out)))
> +static inline bool skip_prefix_impl(const char *str, const char *prefix,
> +				    const char **out)
>  {
>  	do {
>  		if (!*prefix) {

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

[Jeff King]

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

> Jeff King <peff@peff.net> writes:
> 
> > +/*
> > + * Check that an out-parameter that is "at least as const as" a matching
> > + * in-parameter. For example, skip_prefix() will return "out" that is a subset
> > + * of "str". So:
> 
> 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".

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

-Peff

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

[Phillip Wood]

On 01/04/2026 20:24, Jeff King wrote:
> On Wed, Apr 01, 2026 at 03:04:10PM +0100, Phillip Wood wrote:
> 
>> On 01/04/2026 14:17, Phillip Wood wrote:
>>> On 01/04/2026 00:50, Jeff King wrote:
>>>>
>>>> +/*
>>>> + * Check that an out-parameter that is "at least as const as" a matching
>>>> + * in-parameter. For example, skip_prefix() will return "out" that
>>>> is a subset
>>>> + * of "str". So:
>>>> + *
>>>> + *  const str, const out: ok
>>>> + *  non-const str, const out: ok
>>>> + *  non-const str, non-const out: ok
>>>> + *  const str, non-const out: compile error
>>>> + *
>>>> + *  See the skip_prefix macro below for an example of use.
>>>> + */
>>>> +#define CONST_OUTPARAM(in, out) \
>>>> +    ((const char **)(0 ? ((*(out) = (in)),(out)) : (out)))
>>>> +#define skip_prefix(str, prefix, out) \
>>>> +    skip_prefix((str), (prefix), CONST_OUTPARAM((str), (out)))
>>>
>>> This is clever but it changes the behavior of skip_prefix() which is
>>> documented as not touching out if it returns false.
>>
>> 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.
> 
> 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).

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

> 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

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

[Junio C Hamano]

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

> Jeff King <peff@peff.net> writes:
>
>> +/*
>> + * Check that an out-parameter that is "at least as const as" a matching
>> + * in-parameter. For example, skip_prefix() will return "out" that is a subset
>> + * of "str". So:
>
> 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".

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

Thanks.

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Apr 01, 2026 at 10:11:45PM -0700, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> > +/*
>> > + * Check that an out-parameter that is "at least as const as" a matching
>> > + * in-parameter. For example, skip_prefix() will return "out" that is a subset
>> > + * of "str". So:
>> 
>> 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".
>
> Yes, it's just a typo. What you wrote is what I meant. Can you fix it up
> while applying?

Will do.  Thanks.

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

[Taylor Blau]

On Tue, Mar 31, 2026 at 07:56:37PM -0400, Jeff King wrote:
> On Tue, Mar 31, 2026 at 07:46:23PM -0400, Jeff King wrote:
>
> > 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.
>
> 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. ;)

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

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

[Toon Claes]

Jeff King <peff@peff.net> writes:

> 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", 

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

> +/*
> + * Check that an out-parameter that is "at least as const as" a matching
> + * in-parameter. For example, skip_prefix() will return "out" that is a subset
> + * of "str". So:
> + *
> + *  const str, const out: ok
> + *  non-const str, const out: ok
> + *  non-const str, non-const out: ok
> + *  const str, non-const out: compile error
> + *
> + *  See the skip_prefix macro below for an example of use.
> + */
> +#define CONST_OUTPARAM(in, out) \
> +    ((const char **)(0 ? ((*(out) = (in)),(out)) : (out)))

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

-- 
Cheers,
Toon

Re: [PATCH v2 0/12] fixing the remainder of the C23 strchr warnings

[Toon Claes]

Jeff King <peff@peff.net> writes:

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

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.

-- 
Cheers,
Toon

[PATCH v2 13/12] git-compat-util: fix CONST_OUTPARAM typo and indentation

[Jeff King]

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

> > +/*
> > + * Check that an out-parameter that is "at least as const as" a matching
> > + * in-parameter. For example, skip_prefix() will return "out" that is a subset
> > + * of "str". So:
> > + *
> > + *  const str, const out: ok
> > + *  non-const str, const out: ok
> > + *  non-const str, non-const out: ok
> > + *  const str, non-const out: compile error
> > + *
> > + *  See the skip_prefix macro below for an example of use.
> > + */
> > +#define CONST_OUTPARAM(in, out) \
> > +    ((const char **)(0 ? ((*(out) = (in)),(out)) : (out)))
> 
> I'm not sure it matters, but this is indented with spaces

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(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index 4ddac61992..ae1bdc90a4 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -464,7 +464,7 @@ report_fn get_warn_routine(void);
 void set_die_is_recursing_routine(int (*routine)(void));
 
 /*
- * Check that an out-parameter that is "at least as const as" a matching
+ * Check that an out-parameter is "at least as const as" a matching
  * in-parameter. For example, skip_prefix() will return "out" that is a subset
  * of "str". So:
  *
@@ -476,7 +476,7 @@ void set_die_is_recursing_routine(int (*routine)(void));
  *  See the skip_prefix macro below for an example of use.
  */
 #define CONST_OUTPARAM(in, out) \
-    ((const char **)(0 ? ((*(out) = (in)),(out)) : (out)))
+	((const char **)(0 ? ((*(out) = (in)),(out)) : (out)))
 
 /*
  * If the string "str" begins with the string found in "prefix", return true.
-- 
2.54.0.rc0.409.g4c76eb20e4