Re: [PATCH] git-compat-util: make git_find_last_dir_sep return a const pointer

From: Jeff King <peff@peff.net>
Date: Feb 3, 2026, 06:25 UTC
Message-ID: <20260203062537.GA286409@coredump.intra.peff.net>
In-Reply-To: <e6f7e2eddbc9aef1c21f661420a4b8cb9cd8e2c1.1770095829.git.collin.funk1@gmail.com>

On Mon, Feb 02, 2026 at 09:19:01PM -0800, Collin Funk wrote:

> Unsure if this should be tagged [RFC], but this patch clears up lots
> of warning spam with glibc 2.43 because of a change mentioned in the
> commit message.

Thanks for the heads-up. I can reproduce here by installing glibc 2.43 via "apt install -t experimental libc6" on my debian unstable machine.

> I plan to handle the rest of them and try to organize the changes by
> subsystem, for lack of a better term. But I figured it was best to
> submit just this one for review first.

Wow, there's...a lot of spots. Looks like ~65 of them based on my hacky first-pass. Many of them are quite obvious "s/char/const char/" fixes in variable declarations, that should have been const all along. I think those can all go together in one patch, as the compiler can verify that we never try to write to the result.

And then, yeah, I'd do the tricky ones system by system. Some of the ones that do write to the resulting pointers are rather nasty, and seem to fall into one of two camps:

  1. Some function interface takes a const pointer, even though we try
     to write to it under the hood (after laundering it through strchr()
     or similar). I think it would be worth refactoring these interfaces
     when we can, though some of them are pretty questionable. For
     instance, all of the rev-parse/revision.c "dotdot" parsing works on
     a "const char *arg". Surely we feed this from command line options
     in some cases? I guess argv is guaranteed to be writable by the
     standard, though we tend to treat is as const everywhere.

  2. We know we have a non-const pointer, but it is passed through a
     const pointer that is used as an out-parameter to a function like
     skip_prefix(). For instance, in http.c's redact_sensitive_header()
     we have something like this:

        const char *sensitive_header;
	if (skip_iprefix(header->buf, "Cookie:", &sensitive_header)) {
		const char *cookie = sensitive_header;
		char *semicolon = strchr(cookie, "; ");
		*semicolon = 0;
		...

     Our header->buf here is a strbuf, so we know we are working with a
     non-const buffer. We launder away constness with the strchr()
     assignment to "semicolon", which glibc now complains about. We
     should make "cookie" non-const, which is easy. But now we'll get a
     complaint about assigning the const "sensitive_header" to it. And
     that one should _also_ be non-const, because it comes from
     header->buf. But switching it will cause the compiler to complain
     about passing it to skip_iprefix().

     So we have the problem in reverse (instead of laundering a const
     string to a non-const, we've accidentally added constness where it
     is not needed). If we drop the const from skip_iprefix(), then that
     has fallout in all the other spots that do pass in a const haystack
     parameter.

     I don't know what the right solution is here. I guess the best we
     can do is probably adding casts with comments like "this is OK
     because it comes from...". But I'm not sure if we are better to
     cast away the constness in one spot, or to make all of the
     variables non-const and cast the out-parameter to skip_iprefix().

Show 6 quoted lines

>  #ifndef find_last_dir_sep
> -static inline char *git_find_last_dir_sep(const char *path)
> +static inline const char *git_find_last_dir_sep(const char *path)
>  {
>  	return strrchr(path, '/');
>  }

This kind of recreates that reverse problem again, though: any caller who really does have a non-const "path" will get "const" added back into it. And that leads to casts like...

Show 7 quoted lines

>  static int chop_last_dir(char **remoteurl, int is_relative)
>  {
> -	char *rfind = find_last_dir_sep(*remoteurl);
> +	char *rfind = (char *) find_last_dir_sep(*remoteurl);
>  	if (rfind) {
>  		*rfind = '\0';
>  		return 0;

...this one. Can we implement it as a macro? That lets the compiler do the right thing, because we do not declare any type then. It used to be a macro, but switched in bf7283465b (turn path macros into inline function, 2014-08-16). There's also a level of macro indirection; on Windows this expands to win32_find_last_dir_sep(), which of course casts away the constness manually. ;)

I also wonder if we could do some gcc/glibc-specific magic to get the best of both worlds. That is, could we get the same "the return value is const if the input parameter was" type-checking that is happening with strchr()?

Looking at strchr()'s declaration in string.h, which is defined like:

  #  define strchr(S, C)                                          \
    __glibc_const_generic (S, const char *, strchr (S, C))

I think the answer is probably "yes". But it also doesn't quite solve our problem. That would give us type-checking of callers of our function, but we still have to convince the compiler not to complain about its implementation. For that we'd need to either cast away const manually, I guess.

Yuck. What a mess. I do think that fixing these warnings will improve most of the call-sites I looked at, but some of them get a bit hairy.

-Peff