On Sat, Aug 02, 2025 at 12:13:25PM -0400, D. Ben Knoble wrote:
> > Also, I guess this function ought to be respecting the literal_pathspecs
> > global? The actual pathspec parser does.
> >
> > If we can, we probably ought to be feeding the paths to a function like
> > pathspec.c:parse_element_magic() and then checking the resulting flags
> > (and skipping past the prefix as it indicates).
>
> Thanks for pointing me at this; maybe I'll find some time for patches
> unless someone beats me to it.
So here's a simple-ish way to use the full pathspec parsing code:
diff --git a/setup.c b/setup.c
index 6f52dab64c..ad27a65d6b 100644
--- a/setup.c
+++ b/setup.c
@@ -176,28 +176,27 @@ int path_inside_repo(const char *prefix, const char *path)
int check_filename(const char *prefix, const char *arg)
{
- char *to_free = NULL;
+ const char *args[] = { arg, NULL };
+ struct pathspec ps;
struct stat st;
- if (skip_prefix(arg, ":/", &arg)) {
- if (!*arg) /* ":/" is root dir, always exists */
- return 1;
- prefix = NULL;
- } else if (skip_prefix(arg, ":!", &arg) ||
- skip_prefix(arg, ":^", &arg)) {
- if (!*arg) /* excluding everything is silly, but allowed */
- return 1;
- }
+ parse_pathspec(&ps, 0, 0, prefix, args);
+ if (ps.nr < 1)
+ BUG("pathspec ended up with no items?");
+ arg = ps.items[0].match;
- if (prefix)
- arg = to_free = prefix_filename(prefix, arg);
+ /* empty paths (after parsing magic) are OK */
+ if (!*arg) {
+ clear_pathspec(&ps);
+ return 1;
+ }
if (!lstat(arg, &st)) {
- free(to_free);
+ clear_pathspec(&ps);
return 1; /* file exists */
}
if (is_missing_file_error(errno)) {
- free(to_free);
+ clear_pathspec(&ps);
return 0; /* file does not exist */
}
die_errno(_("failed to stat '%s'"), arg);
It does make :^:Documentation work as you'd expect. Curiously, testing
:/Documentation is hard because that is _also_ a valid rev (the ":/"
searches for a commit with that string in its message). So it will
almost always fail anyway as "hey, this is both a rev and a file". But
you can do ":^/Documentation" to see how it behaves. ;)
But here's the interesting part: it breaks a bunch of tests. They all
seem to be doing things like ":file.txt". In check_filename() right now
we treat that literally. But as a pathspec, it is technically "colon
followed by zero or more magic signature letters", and it is eaten.
So I wonder if we have painted ourselves into a compatibility corner a
bit, if we have two conflicting expectations. We might be better off
just teaching check_filename() to parse multiple of [^/!] and the
trailing colon. It's horrible and not great for maintainability, but
this syntax is not something that changes often.
-Peff
