Re: [PATCH] git-compat-util: make git_find_last_dir_sep return a const pointer
From: Collin Funk <hidden>
Date: 2026-02-04 03:15:12
Jeff King [off-list ref] writes:
On Mon, Feb 02, 2026 at 09:19:01PM -0800, Collin Funk wrote:quoted
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.quoted
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.
Yep, it is quite noisy. And that plan makes sense to me. I'll create a seperate patch handling the obvious 's/char/const char/' conversions that make sense regardless of this glibc change.
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.
Yes, I see. I think the "arg" there is from the command line or from a
buffer read using fgets() in get_object_list from
builtin/pack-objects.c, so it is safe to write to there.
It's also called like this though:
handle_revision_arg("HEAD", &revs, 0, 0);
We can't write to the string "HEAD", but it doesn't have a "dotdot" so
we don't. It could probably be cleaned up a bit.
FYI, that code would also be made much clearer if not all of the
declarations were at the top of the function. I guess it just hasn't
been touched in a long while.
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().Makes sense, I'll try to handle the non-obviously ones separately.
quoted
#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...
I figured it was okay since only one place casted the qualifier away. But I agree it is probably worth cleaning that up later.
quoted
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.
That macro depends on Generic selections from C11 [1]. I wasn't sure if Git would like that, given it is conservative with other C features.
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.
Thanks to C23. :) Collin [1] https://en.cppreference.com/w/c/language/generic.html