{"thread":{"id":"63466","subject":"[PATCH RFC v2] diff --no-index: support limiting by pathspec","startedAt":"2025-05-15T20:22:56Z","lastAt":"2025-05-15T20:22:56Z","messageCount":1,"participants":["Jacob Keller"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"518179","messageId":"20250515202250.3293814-1-jacob.e.keller@intel.com","threadId":"63466","inReplyTo":null,"subject":"[PATCH RFC v2] diff --no-index: support limiting by pathspec","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2025-05-15T20:22:50Z","receivedAt":"2025-05-15T20:22:56Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller <jacob.keller@gmail.com>\n\nThe --no-index option of git-diff enables using the diff machinery from\ngit while operating outside of a repository. This mode of git diff is\nable to compare directories and produce a diff of their contents.\n\nWhen operating git diff in a repository, git has the notion of\n\"pathspecs\" which can specify which files to compare. In particular,\nwhen using git to diff two trees, you might invoke:\n\n  $ git diff-tree -r <treeish1> <treeish2>.\n\nwhere the treeish could point to a subdirectory of the repository.\n\nWhen invoked this way, users can limit the selected paths of the tree by\nusing a pathspec. Either by providing some list of paths to accept, or\nby removing paths via a negative refspec.\n\nThe git diff --no-index mode does not support pathspecs, and cannot\nlimit the diff output in this way. Other diff programs such as GNU\ndifftools have options for excluding paths based on a pattern match.\nHowever, using git diff as a diff replacement has several advantages\nover many popular diff tools, including coloring moved lines, rename\ndetections, and similar.\n\nTeach git diff --no-index how to handle pathspecs to limit the\ncomparisons. This will only be supported if both provided paths are\ndirectories.\n\nThis is because the --no-index mode already employs some DWIM shortcuts\nwhen dealing with comparing a directory and a file.\n\nExecuting git diff --no-index D F is interpreted as if:\n\n  $ git diff --no-index D/$(basename F) F\n\nSimilarly, if you do git diff --no-index F D.\n\nModify the fixup_paths function to return 1 if both paths are\ndirectories. If this is the case, interpret any extra arguments to git\ndiff as pathspecs via parse_pathspec. Add a new PATHSPEC_NO_REPOSITORY\nflag to indicate to the parser that we do not have repository. Use this\nto aid in error message reporting in the event that the user happens to\ninvoke git diff --no-index from a repository.\n\nExtend the prefix_path_gently function to correctly handle a prefix\nwhich does not end in '/', by inserting one as appropriate.\n\nUse parse_pathspec to load the remaining arguments (if any) to git diff\n--no-index as pathspec items. Disable PATHSPEC_ATTR support since we do\nnot have a repository to do attribute lookup. Disable PATHSPEC_FROMTOP\nsince we do not have a repository root. All pathspecs are treated as\nrooted at the provided comparison paths.\n\nLoad the pathspecs twice, once prefixed with paths[0] and once prefixed\nwith paths[1]. I considered trying to avoid this, but don't yet have a\nworkable solution that reuses the same pathspec objects. We need the\ndirectory paths as prefixes otherwise the match_pathspec won't compare\nproperly.\n\nPass the pathspec object for both paths into queue_diff, who in-turn\npasses these along to read_directory_contents.\n\nModify read_directory_contents to check against the pathspecs when\nscanning the directory. In order to properly recurse, we must handle\nleading directory checks. Thus, a new \"match_leading_pathspecs\" variant\nis created, which sets the DO_MATCH_LEADING_PATHSPEC flag. This is\nrequired to correctly handle nested directories. Consider this:\n\n  $ git diff --no-index a b c/d\n\nThis should include all paths in a and b which match the c/d pathspec.\nIn particular, if there was 'a/c/d' we need to match it. But to check\n'a/c/d', we need to first get to that part, which requires comparing\n'a/c' first. Without DO_MATCH_LEADING_PATHSPEC, 'a/c' won't match the\n'a/c/d' pathspec string.\n\nThis implementation appears to work ok, but I still need to create some\nunit tests. I also think this might be a little hacky, and I am not sure\nhow folks feel about parsing separate pathspecs for both entries.\n\nSome other gotchas and open questions:\n\n 1) pathspecs must all come after the first two path arguments, you\n    can't re-arrange them to come first. I'm treating them sort of like\n    the treeish arguments to git diff-tree.\n\n 2) The pathspecs are interpreted relative to the provided paths, and\n    thus will always need to be specified as relative paths, and will be\n    interpreted as relative to the root of the search for each path\n    separately.\n\n 3) negative pathspecs have to be fully qualified from the root, i.e.\n    ':(exclude)file' will only exclude 'a/file' and not 'a/b/file'\n    unless you also use '(glob)' or similar. I think this matches the\n    other pathspec support, but I an not 100% sure.\n\nI feel like some of these changes are a bit hacky and could use some\nfurther refinement or suggestion, but the core idea appears to work\nreasonably well!\n\nSigned-off-by: Jacob Keller <jacob.keller@gmail.com>\n---\nThis version uses pathspecs and hopefully works in a reasonable enough way\nto not be too confusing. Still needs tests, likely needs to be broken up\ninto smaller pieces, or some of the hacks need to be smoothed over. I\nhave executed this, but didn't run the full test suite yet.\n\n pathspec.h      | 10 ++++++\n diff-no-index.c | 85 ++++++++++++++++++++++++++++++++++++++++---------\n dir.c           | 24 ++++++++++++--\n pathspec.c      |  7 +++-\n setup.c         |  4 ++-\n 5 files changed, 110 insertions(+), 20 deletions(-)\n\ndiff --git a/pathspec.h b/pathspec.h\nindex de537cff3cb6..363fb6309663 100644\n--- a/pathspec.h\n+++ b/pathspec.h\n@@ -76,6 +76,11 @@ struct pathspec {\n  * allowed, then it will automatically set for every pathspec.\n  */\n #define PATHSPEC_LITERAL_PATH (1<<6)\n+/*\n+ * For git diff --no-index, indicate that we are operating without\n+ * a repository or index.\n+ */\n+#define PATHSPEC_NO_REPOSITORY (1<<7)\n \n /**\n  * Given command line arguments and a prefix, convert the input to\n@@ -184,6 +189,11 @@ int match_pathspec(struct index_state *istate,\n \t\t   const char *name, int namelen,\n \t\t   int prefix, char *seen, int is_dir);\n \n+int match_leading_pathspec(struct index_state *istate,\n+\t\t\t   const struct pathspec *ps,\n+\t\t\t   const char *name, int namelen,\n+\t\t\t   int prefix, char *seen, int is_dir);\n+\n /*\n  * Determine whether a pathspec will match only entire index entries (non-sparse\n  * files and/or entire sparse directories). If the pathspec has the potential to\ndiff --git a/diff-no-index.c b/diff-no-index.c\nindex 9739b2b268b9..4535d032c27f 100644\n--- a/diff-no-index.c\n+++ b/diff-no-index.c\n@@ -15,20 +15,43 @@\n #include \"gettext.h\"\n #include \"revision.h\"\n #include \"parse-options.h\"\n+#include \"pathspec.h\"\n #include \"string-list.h\"\n #include \"dir.h\"\n \n-static int read_directory_contents(const char *path, struct string_list *list)\n+static int read_directory_contents(const char *path, struct string_list *list,\n+\t\t\t\t   const struct pathspec *pathspec)\n {\n+\tstruct strbuf match = STRBUF_INIT;\n+\tint len;\n \tDIR *dir;\n \tstruct dirent *e;\n \n \tif (!(dir = opendir(path)))\n \t\treturn error(\"Could not open directory %s\", path);\n \n-\twhile ((e = readdir_skip_dot_and_dotdot(dir)))\n-\t\tstring_list_insert(list, e->d_name);\n+\tif (pathspec) {\n+\t\tstrbuf_addstr(&match, path);\n+\t\tstrbuf_complete(&match, '/');\n+\t\tlen = match.len;\n+\t}\n \n+\twhile ((e = readdir_skip_dot_and_dotdot(dir))) {\n+\t\tif (pathspec) {\n+\t\t\tstrbuf_setlen(&match, len);\n+\t\t\tstrbuf_addstr(&match, e->d_name);\n+\n+\t\t\tif (!match_leading_pathspec(NULL, pathspec,\n+\t\t\t\t\t\t    match.buf, match.len,\n+\t\t\t\t\t\t    0, NULL,\n+\t\t\t\t\t\t    e->d_type == DT_DIR ? 1 : 0))\n+\t\t\t\tcontinue;\n+\t\t}\n+\n+\t\tstring_list_insert(list, e->d_name);\n+\t}\n+\n+\tstrbuf_release(&match);\n \tclosedir(dir);\n \treturn 0;\n }\n@@ -131,7 +154,8 @@ static struct diff_filespec *noindex_filespec(const struct git_hash_algo *algop,\n }\n \n static int queue_diff(struct diff_options *o, const struct git_hash_algo *algop,\n-\t\t      const char *name1, const char *name2, int recursing)\n+\t\t      const char *name1, const char *name2, int recursing,\n+\t\t      const struct pathspec *ps1, const struct pathspec *ps2)\n {\n \tint mode1 = 0, mode2 = 0;\n \tenum special special1 = SPECIAL_NONE, special2 = SPECIAL_NONE;\n@@ -171,9 +195,9 @@ static int queue_diff(struct diff_options *o, const struct git_hash_algo *algop,\n \t\tint i1, i2, ret = 0;\n \t\tsize_t len1 = 0, len2 = 0;\n \n-\t\tif (name1 && read_directory_contents(name1, &p1))\n+\t\tif (name1 && read_directory_contents(name1, &p1, ps1))\n \t\t\treturn -1;\n-\t\tif (name2 && read_directory_contents(name2, &p2)) {\n+\t\tif (name2 && read_directory_contents(name2, &p2, ps2)) {\n \t\t\tstring_list_clear(&p1, 0);\n \t\t\treturn -1;\n \t\t}\n@@ -218,7 +242,7 @@ static int queue_diff(struct diff_options *o, const struct git_hash_algo *algop,\n \t\t\t\tn2 = buffer2.buf;\n \t\t\t}\n \n-\t\t\tret = queue_diff(o, algop, n1, n2, 1);\n+\t\t\tret = queue_diff(o, algop, n1, n2, 1, ps1, ps2);\n \t\t}\n \t\tstring_list_clear(&p1, 0);\n \t\tstring_list_clear(&p2, 0);\n@@ -258,8 +282,10 @@ static void append_basename(struct strbuf *path, const char *dir, const char *fi\n  * DWIM \"diff D F\" into \"diff D/F F\" and \"diff F D\" into \"diff F D/F\"\n  * Note that we append the basename of F to D/, so \"diff a/b/file D\"\n  * becomes \"diff a/b/file D/file\", not \"diff a/b/file D/a/b/file\".\n+ *\n+ * Return 1 if both paths are directories, 0 otherwise.\n  */\n-static void fixup_paths(const char **path, struct strbuf *replacement)\n+static int fixup_paths(const char **path, struct strbuf *replacement)\n {\n \tstruct stat st;\n \tunsigned int isdir0 = 0, isdir1 = 0;\n@@ -282,25 +308,30 @@ static void fixup_paths(const char **path, struct strbuf *replacement)\n \tif ((isdir0 && ispipe1) || (ispipe0 && isdir1))\n \t\tdie(_(\"cannot compare a named pipe to a directory\"));\n \n-\tif (isdir0 == isdir1)\n-\t\treturn;\n+\t/* if both paths are directories, we will enable pathspecs */\n+\tif (isdir0 && isdir1)\n+\t\treturn 1;\n+\n \tif (isdir0) {\n \t\tappend_basename(replacement, path[0], path[1]);\n \t\tpath[0] = replacement->buf;\n-\t} else {\n+\t} else if (isdir1) {\n \t\tappend_basename(replacement, path[1], path[0]);\n \t\tpath[1] = replacement->buf;\n \t}\n+\n+\treturn 0;\n }\n \n static const char * const diff_no_index_usage[] = {\n-\tN_(\"git diff --no-index [<options>] <path> <path>\"),\n+\tN_(\"git diff --no-index [<options>] <path> <path> [<pathspec>...]\"),\n \tNULL\n };\n \n int diff_no_index(struct rev_info *revs, const struct git_hash_algo *algop,\n \t\t  int implicit_no_index, int argc, const char **argv)\n {\n+\tstruct pathspec pathspec1, pathspec2, *ps1 = NULL, *ps2 = NULL;\n \tint i, no_index;\n \tint ret = 1;\n \tconst char *paths[2];\n@@ -317,7 +348,7 @@ int diff_no_index(struct rev_info *revs, const struct git_hash_algo *algop,\n \toptions = add_diff_options(no_index_options, &revs->diffopt);\n \targc = parse_options(argc, argv, revs->prefix, options,\n \t\t\t     diff_no_index_usage, 0);\n-\tif (argc != 2) {\n+\tif (argc < 2) {\n \t\tif (implicit_no_index)\n \t\t\twarning(_(\"Not a git repository. Use --no-index to \"\n \t\t\t\t  \"compare two paths outside a working tree\"));\n@@ -337,7 +368,27 @@ int diff_no_index(struct rev_info *revs, const struct git_hash_algo *algop,\n \t\tpaths[i] = p;\n \t}\n \n-\tfixup_paths(paths, &replacement);\n+\t/* TODO: should we try to catch pathspec-like paths first and warn or\n+\t * error? We accepted those as valid 'paths' before so it seems\n+\t * unlikely we can change that behavior.\n+\t */\n+\tif (fixup_paths(paths, &replacement)) {\n+\t\tparse_pathspec(&pathspec1, PATHSPEC_FROMTOP | PATHSPEC_ATTR,\n+\t\t\t       PATHSPEC_PREFER_FULL | PATHSPEC_NO_REPOSITORY,\n+\t\t\t       paths[0], &argv[2]);\n+\t\tif (pathspec1.nr)\n+\t\t\tps1 = &pathspec1;\n+\n+\t\tparse_pathspec(&pathspec2, PATHSPEC_FROMTOP | PATHSPEC_ATTR,\n+\t\t\t       PATHSPEC_PREFER_FULL | PATHSPEC_NO_REPOSITORY,\n+\t\t\t       paths[1], &argv[2]);\n+\t\tif (pathspec2.nr)\n+\t\t\tps2 = &pathspec2;\n+\t} else if (argc > 2) {\n+\t\twarning(_(\"Limiting comparison with pathspecs is only \"\n+\t\t\t  \"supported if both paths are directories.\"));\n+\t\tusage_with_options(diff_no_index_usage, options);\n+\t}\n \n \trevs->diffopt.skip_stat_unmatch = 1;\n \tif (!revs->diffopt.output_format)\n@@ -354,7 +405,7 @@ int diff_no_index(struct rev_info *revs, const struct git_hash_algo *algop,\n \tsetup_diff_pager(&revs->diffopt);\n \trevs->diffopt.flags.exit_with_status = 1;\n \n-\tif (queue_diff(&revs->diffopt, algop, paths[0], paths[1], 0))\n+\tif (queue_diff(&revs->diffopt, algop, paths[0], paths[1], 0, ps1, ps2))\n \t\tgoto out;\n \tdiff_set_mnemonic_prefix(&revs->diffopt, \"1/\", \"2/\");\n \tdiffcore_std(&revs->diffopt);\n@@ -370,5 +421,9 @@ int diff_no_index(struct rev_info *revs, const struct git_hash_algo *algop,\n \tfor (i = 0; i < ARRAY_SIZE(to_free); i++)\n \t\tfree(to_free[i]);\n \tstrbuf_release(&replacement);\n+\tif (ps1)\n+\t\tclear_pathspec(ps1);\n+\tif (ps2)\n+\t\tclear_pathspec(ps2);\n \treturn ret;\n }\ndiff --git a/dir.c b/dir.c\nindex a374972b6243..7f6079475397 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -397,9 +397,13 @@ static int match_pathspec_item(struct index_state *istate,\n \t    strncmp(item->match, name - prefix, item->prefix))\n \t\treturn 0;\n \n-\tif (item->attr_match_nr &&\n-\t    !match_pathspec_attrs(istate, name - prefix, namelen + prefix, item))\n-\t\treturn 0;\n+\tif (item->attr_match_nr) {\n+\t\t/* TODO: is there a better way to handle this? */\n+\t\tif (!istate)\n+\t\t\tBUG(\"magic PATHSPEC_ATTR requires an index\");\n+\t\tif (!match_pathspec_attrs(istate, name - prefix, namelen + prefix, item))\n+\t\t\treturn 0;\n+\t}\n \n \t/* If the match was just the prefix, we matched */\n \tif (!*match)\n@@ -577,6 +581,20 @@ int match_pathspec(struct index_state *istate,\n \t\t\t\t\t prefix, seen, flags);\n }\n \n+int match_leading_pathspec(struct index_state *istate,\n+\t\t\t   const struct pathspec *ps,\n+\t\t\t   const char *name, int namelen,\n+\t\t\t   int prefix, char *seen, int is_dir)\n+{\n+\tunsigned flags = DO_MATCH_LEADING_PATHSPEC;\n+\n+\tif (is_dir)\n+\t\tflags |= DO_MATCH_DIRECTORY;\n+\n+\treturn match_pathspec_with_flags(istate, ps, name, namelen,\n+\t\t\t\t\t prefix, seen, flags);\n+}\n+\n /**\n  * Check if a submodule is a superset of the pathspec\n  */\ndiff --git a/pathspec.c b/pathspec.c\nindex 2b4e434bc0aa..7a46da50dc77 100644\n--- a/pathspec.c\n+++ b/pathspec.c\n@@ -492,7 +492,12 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,\n \t\tif (!match) {\n \t\t\tconst char *hint_path;\n \n-\t\t\tif (!have_git_dir())\n+\t\t\t/* TODO: should we have a different error message?\n+\t\t\t * Really, we can't have absolute paths for pathspec\n+\t\t\t * in git diff --no-index since it must be relative to\n+\t\t\t * both directories in order to work at all.\n+\t\t\t */\n+\t\t\tif ((flags & PATHSPEC_NO_REPOSITORY) || !have_git_dir())\n \t\t\t\tdie(_(\"'%s' is outside the directory tree\"),\n \t\t\t\t    copyfrom);\n \t\t\thint_path = repo_get_work_tree(the_repository);\ndiff --git a/setup.c b/setup.c\nindex f93bd6a24a5d..56ebf1218442 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -139,7 +139,9 @@ char *prefix_path_gently(const char *prefix, int len,\n \t\t\treturn NULL;\n \t\t}\n \t} else {\n-\t\tsanitized = xstrfmt(\"%.*s%s\", len, len ? prefix : \"\", path);\n+\t\tsanitized = xstrfmt(\"%.*s%s%s\", len, len ? prefix : \"\",\n+\t\t\t\t    len && prefix[len - 1] == '/' ? \"\" : \"/\",\n+\t\t\t\t    path);\n \t\tif (remaining_prefix)\n \t\t\t*remaining_prefix = len;\n \t\tif (normalize_path_copy_len(sanitized, sanitized, remaining_prefix)) {\n-- \n2.48.1.397.gec9d649cc640\n\n"}]}