Re: [PATCH v4 3/3] diff --no-index: support limiting by pathspec
From: Johannes Schindelin <hidden>
Date: 2025-09-23 14:57:28
Hi Jacob, I know this is about a patch that you contributed four months ago, and the usual feedback required sweeping changes, including this one that was introduced in v4: On Wed, 21 May 2025, Jacob Keller wrote:
quoted hunk ↗ jump to hunk
diff --git a/diff-no-index.c b/diff-no-index.c index 9739b2b268b9..4aeeb98cfa8f 100644 --- a/diff-no-index.c +++ b/diff-no-index.c@@ -15,20 +15,45 @@ #include "gettext.h" #include "revision.h" #include "parse-options.h" +#include "pathspec.h" #include "string-list.h" #include "dir.h" -static int read_directory_contents(const char *path, struct string_list *list) +static int read_directory_contents(const char *path, struct string_list *list, + const struct pathspec *pathspec, + int skip) { + struct strbuf match = STRBUF_INIT; + int len; DIR *dir; struct dirent *e; if (!(dir = opendir(path))) return error("Could not open directory %s", path); - while ((e = readdir_skip_dot_and_dotdot(dir))) - string_list_insert(list, e->d_name); + if (pathspec) { + strbuf_addstr(&match, path); + strbuf_complete(&match, '/'); + strbuf_remove(&match, 0, skip);
Okay, so here the `read_directory_contents()` function learns to optionally skip `skip` bytes from the `path` variable, after potentially appending a `/`.
quoted hunk ↗ jump to hunk
[...]@@ -337,7 +369,23 @@ int diff_no_index(struct rev_info *revs, const struct git_hash_algo *algop, paths[i] = p; } - fixup_paths(paths, &replacement); + if (fixup_paths(paths, &replacement)) { + parse_pathspec(&pathspec, PATHSPEC_FROMTOP | PATHSPEC_ATTR, + PATHSPEC_PREFER_FULL | PATHSPEC_NO_REPOSITORY, + NULL, &argv[2]); + if (pathspec.nr) + ps = &pathspec; + + skip1 = strlen(paths[0]); + skip1 += paths[0][skip1] == '/' ? 0 : 1;
Since `skip1` is defined as the length of `path[0]`, I would expect `paths[0][skip1]` to always evaluate to NUL, and therefore the `== '/'` condition to always evaluate to `false`. Did I miss anything?
+ skip2 = strlen(paths[1]); + skip2 += paths[1][skip2] == '/' ? 0 : 1;
Same here, `paths[1][skip2]` should always return `NUL`. This has ramifications where `skip1` and `skip2` are each one larger than the length of `paths[0]` and `paths[1]`, respectively, and hence the code in `read_directory_contents()` will now try to remove one more than the length of the path, after potentially appending a slash. But what if there is already a slash? The answer is: $ git diff --no-index -- /tmp/ /tmp/ ':!' fatal: `pos + len' is too far after the end of the buffer This has been reported (with Windows paths, don't let that distract) in https://github.com/git-for-windows/git/issues/5836. I _think_ that what the patch should have done instead was: if (skip1 > 0 && paths[0][skip1 - 1] == '/') skip1--; and likewise if (skip2 > 0 && paths[1][skip2 - 1] == '/') skip2--; Focusing on the lines' correctness (which I don't think was the primary concern in the review of your patch), that would be what I would suggest. However, this makes me wonder whether the logic itself is sound? It is not immediately obvious to me why the `paths[0]` and `paths[1]` values aren't matched against the pathspec yet their entirety is seemingly skipped in `read_directory_contents()`? Ciao, Johannes