{"thread":{"id":"58721","subject":"[PATCH] [RFC] diff: introduce scope option","startedAt":"2022-10-31T04:12:01Z","lastAt":"2022-11-29T12:01:36Z","messageCount":11,"participants":["ZheNing Hu via GitGitGadget","Taylor Blau","ZheNing Hu","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"466077","messageId":"pull.1398.git.1667189512579.gitgitgadget@gmail.com","threadId":"58721","inReplyTo":null,"subject":"[PATCH] [RFC] diff: introduce scope option","fromName":"ZheNing Hu via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-10-31T04:11:52Z","receivedAt":"2022-10-31T04:12:01Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"From: ZheNing Hu \n\nWhen we use sparse-checkout, we often want the set of files\nthat some commands operate on to be restricted to the\nsparse-checkout specification.\n\nSo introduce the `--scope` option to git diff, which have two\nvalue: \"sparse\" and \"all\". \"sparse\" mean that diff is performed\nrestrict to paths which matching sparse-checkout specification,\n\"all\" mean that diff is performed regardless of whether the path\nmeets the sparse-checkout specification. `--no-scope` is the default\noption for now.\n\nAdd `diff.scope={sparse, all}` config, which can also have the same\ncapabilities as `--scope`, and it will be covered by `--scope` option.\n\nSigned-off-by: ZheNing Hu \n---\n [RFC] diff: introduce scope option\n \n In [1], we discovered that users working on different sparse-checkout\n specification may download unnecessary blobs from each other's\n specification in collaboration. In [2] Junio suggested that maybe we can\n restrict some git command's filespec in sparse-checkout specification to\n elegantly solve this problem above. In [3]: Newren and Derrick Stolee\n prefer to name the option --scope={sparse, all}.\n \n So this patch is attempt to do this thing on git diff:\n \n v1:\n \n 1. add --restrict option to git diff, which restrict diff filespec in\n sparse-checkout specification. [4] v2.\n 2. rename --restrict to --scope={sparse, all}, support --no-scope.\n 3. add config: diff.scope={sparse,all}.\n \n Unresolved work:\n \n 1. how to properly pass this --scope={sparse, all} to other commands\n like git log, git format-patch, etc.\n 2. how to set the default value of scope for different diff commands.\n \n [1]:\n https://lore.kernel.org/git/CAOLTT8SHo66kGbvWr=+LQ9UVd1NHgqGGEYK2qq6==QgRCgLZqQ@mail.gmail.com/\n [2]: https://lore.kernel.org/git/xmqqzgeqw0sy.fsf@gitster.g/ [3]:\n https://lore.kernel.org/git/07a25d48-e364-0d9b-6ffa-41a5984eb5db@github.com/\n [4]:\n https://lore.kernel.org/git/pull.1368.git.1664036052741.gitgitgadget@gmail.com/\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1398%2Fadlternative%2Fzh%2Fdiff-scope-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1398/adlternative/zh/diff-scope-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1398\n\n Documentation/config/diff.txt | 12 +\n Documentation/diff-options.txt | 18 +\n builtin/diff.c | 4 +\n diff-lib.c | 36 +-\n diff-no-index.c | 4 +\n diff.c | 39 +++\n diff.h | 11 +\n t/t4070-diff-sparse-checkout-scope.sh | 469 ++++++++++++++++++++++++++\n tree-diff.c | 5 +\n 9 files changed, 597 insertions(+), 1 deletion(-)\n create mode 100644 t/t4070-diff-sparse-checkout-scope.sh\n\ndiff --git a/Documentation/config/diff.txt b/Documentation/config/diff.txt\nindex 35a7bf86d77..52707e1b2d6 100644\n--- a/Documentation/config/diff.txt\n+++ b/Documentation/config/diff.txt\n@@ -201,6 +201,18 @@ diff.algorithm::\n --\n +\n \n+diff.scope::\n+\tChoose diff scope. The variants are as follows:\n++\n+--\n+`sparse`;;\n+\tRestrict diff paths to those matching sparse-checkout specification.\n+`all`;;\n+\tWithout restriction, diff is performed regardless of whether the path\n+\tmeets the sparse-checkout specification.\n+--\n++\n+\n diff.wsErrorHighlight::\n \tHighlight whitespace errors in the `context`, `old` or `new`\n \tlines of the diff. Multiple values are separated by comma,\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex 3674ac48e92..04bf83e8be1 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -195,6 +195,24 @@ For instance, if you configured the `diff.algorithm` variable to a\n non-default value and want to use the default one, then you\n have to use `--diff-algorithm=default` option.\n \n+ifndef::git-format-patch[]\n+ifndef::git-log[]\n+\n+--scope={sparse|all}::\n+\tChoose diff scope. The variants are as follows:\n++\n+--\n+`--sparse`;;\n+\tRestrict diff paths to those matching sparse-checkout specification.\n+`--all`;;\n+\tWithout restriction, diff is performed regardless of whether the path\n+\tmeets the sparse-checkout specification.\n+--\n++\n+\n+endif::git-log[]\n+endif::git-format-patch[]\n+\n --stat[=[,[,]]]::\n \tGenerate a diffstat. By default, as much space as necessary\n \twill be used for the filename part, and the rest for the graph\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex 854d2c5a5c4..6b450f7184c 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -54,6 +54,10 @@ static void stuff_change(struct diff_options *opt,\n \t oideq(old_oid, new_oid) && (old_mode == new_mode))\n \t\treturn;\n \n+\tif (opt->scope == DIFF_SCOPE_SPARSE &&\n+\t !diff_paths_in_sparse_checkout(old_path, new_path))\n+\t\treturn;\n+\n \tif (opt->flags.reverse_diff) {\n \t\tSWAP(old_mode, new_mode);\n \t\tSWAP(old_oid, new_oid);\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 2edea41a234..a3381f2e0ff 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -88,6 +88,22 @@ static int match_stat_with_submodule(struct diff_options *diffopt,\n \treturn changed;\n }\n \n+int diff_path_in_sparse_checkout(const char *path) {\n+\tif (core_sparse_checkout_cone)\n+\t\treturn path_in_cone_mode_sparse_checkout(path, the_repository->index);\n+\telse\n+\t\treturn path_in_sparse_checkout(path, the_repository->index);\n+}\n+\n+int diff_paths_in_sparse_checkout(const char *one, const char*two) {\n+\tif (one == two || !strcmp(one, two))\n+\t\treturn diff_path_in_sparse_checkout(one);\n+\telse\n+\t\treturn diff_path_in_sparse_checkout(one) &&\n+\t\t diff_path_in_sparse_checkout(two);\n+}\n+\n+\n int run_diff_files(struct rev_info *revs, unsigned int option)\n {\n \tint entries, i;\n@@ -113,6 +129,9 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \n \t\tif (diff_can_quit_early(&revs->diffopt))\n \t\t\tbreak;\n+\t\tif (revs->diffopt.scope == DIFF_SCOPE_SPARSE &&\n+\t\t !diff_path_in_sparse_checkout(ce->name))\n+\t\t\tcontinue;\n \n \t\tif (!ce_path_match(istate, ce, &revs->prune_data, NULL))\n \t\t\tcontinue;\n@@ -202,7 +221,8 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\t\t\tcontinue;\n \t\t}\n \n-\t\tif (ce_uptodate(ce) || ce_skip_worktree(ce))\n+\t\tif (ce_uptodate(ce) ||\n+\t\t (revs->diffopt.scope != DIFF_SCOPE_ALL && ce_skip_worktree(ce)))\n \t\t\tcontinue;\n \n \t\t/*\n@@ -439,6 +459,20 @@ static void do_oneway_diff(struct unpack_trees_options *o,\n \t\t\treturn;\t/* nothing to diff.. */\n \t}\n \n+\tif (revs->diffopt.scope == DIFF_SCOPE_SPARSE) {\n+\t\tif (idx && tree) {\n+\t\t\tif (!diff_paths_in_sparse_checkout(idx->name, tree->name))\n+\t\t\t\treturn;\n+\t\t} else if (idx) {\n+\t\t\tif (!diff_path_in_sparse_checkout(idx->name))\n+\t\t\t\treturn;\n+\t\t} else if (tree) {\n+\t\t\tif (!diff_path_in_sparse_checkout(tree->name))\n+\t\t\t\treturn;\n+\t\t} else\n+\t\t\treturn;\n+\t}\n+\n \t/* if the entry is not checked out, don't examine work tree */\n \tcached = o->index_only ||\n \t\t(idx && ((idx->ce_flags & CE_VALID) || ce_skip_worktree(idx)));\ndiff --git a/diff-no-index.c b/diff-no-index.c\nindex 18edbdf4b59..ea94a104ea4 100644\n--- a/diff-no-index.c\n+++ b/diff-no-index.c\n@@ -281,6 +281,10 @@ int diff_no_index(struct rev_info *revs,\n \n \tfixup_paths(paths, &replacement);\n \n+\tif (revs->diffopt.scope == DIFF_SCOPE_SPARSE &&\n+\t !diff_paths_in_sparse_checkout(paths[0], paths[1]))\n+\t\tgoto out;\n+\n \trevs->diffopt.skip_stat_unmatch = 1;\n \tif (!revs->diffopt.output_format)\n \t\trevs->diffopt.output_format = DIFF_FORMAT_PATCH;\ndiff --git a/diff.c b/diff.c\nindex 285d6e2d575..9de4044ae05 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -48,6 +48,7 @@ static int diff_interhunk_context_default;\n static const char *diff_word_regex_cfg;\n static const char *external_diff_cmd_cfg;\n static const char *diff_order_file_cfg;\n+static const char *external_diff_scope_cfg;\n int diff_auto_refresh_index = 1;\n static int diff_mnemonic_prefix;\n static int diff_no_prefix;\n@@ -57,6 +58,7 @@ static int diff_dirstat_permille_default = 30;\n static struct diff_options default_diff_options;\n static long diff_algorithm;\n static unsigned ws_error_highlight_default = WSEH_NEW;\n+static enum diff_scope external_diff_scope;\n \n static char diff_colors[][COLOR_MAXLEN] = {\n \tGIT_COLOR_RESET,\n@@ -423,6 +425,16 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \n+\tif (!strcmp(var, \"diff.scope\")) {\n+\t\tgit_config_string(&external_diff_scope_cfg, var, value);\n+\t\tif (!strcmp(value, \"all\"))\n+\t\t\texternal_diff_scope = DIFF_SCOPE_ALL;\n+\t\telse if (!strcmp(value, \"sparse\"))\n+\t\t\texternal_diff_scope = DIFF_SCOPE_SPARSE;\n+\t\telse\n+\t\t\treturn -1;\n+\t}\n+\n \tif (git_color_config(var, value, cb) < 0)\n \t\treturn -1;\n \n@@ -4663,6 +4675,7 @@ void repo_diff_setup(struct repository *r, struct diff_options *options)\n \n \toptions->color_moved = diff_color_moved_default;\n \toptions->color_moved_ws_handling = diff_color_moved_ws_default;\n+\toptions->scope = external_diff_scope;\n \n \tprep_parse_options(options);\n }\n@@ -4914,6 +4927,29 @@ static int parse_dirstat_opt(struct diff_options *options, const char *params)\n \treturn 1;\n }\n \n+static int diff_opt_diff_scope(const struct option *option,\n+\t\t\t\tconst char *optarg, int unset)\n+{\n+\tstruct diff_options *opt = option->value;\n+\n+\tif (unset) {\n+\t\topt->scope = DIFF_SCOPE_NONE;\n+\t} else if (optarg) {\n+\t\tif (!strcmp(optarg, \"all\")) {\n+\t\t\tif (core_apply_sparse_checkout) {\n+\t\t\t\topt->scope = DIFF_SCOPE_ALL;\n+\t\t\t}\n+\t\t} else if (!strcmp(optarg, \"sparse\")) {\n+\t\t\tif (core_apply_sparse_checkout) {\n+\t\t\t\topt->scope = DIFF_SCOPE_SPARSE;\n+\t\t\t}\n+\t\t} else\n+\t\t\treturn error(_(\"invalid --scope value: %s\"), optarg);\n+\t}\n+\n+\treturn 0;\n+}\n+\n static int diff_opt_diff_filter(const struct option *option,\n \t\t\t\tconst char *optarg, int unset)\n {\n@@ -5683,6 +5719,9 @@ static void prep_parse_options(struct diff_options *options)\n \t\tOPT_CALLBACK_F(0, \"diff-filter\", options, N_(\"[(A|C|D|M|R|T|U|X|B)...[*]]\"),\n \t\t\t N_(\"select files by diff type\"),\n \t\t\t PARSE_OPT_NONEG, diff_opt_diff_filter),\n+\t\tOPT_CALLBACK_F(0, \"scope\", options, N_(\"[sparse|all]\"),\n+\t\t\t N_(\"choose diff scope\"),\n+\t\t\t PARSE_OPT_OPTARG, diff_opt_diff_scope),\n \t\t{ OPTION_CALLBACK, 0, \"output\", options, N_(\"\"),\n \t\t N_(\"output to a specific file\"),\n \t\t PARSE_OPT_NONEG, NULL, 0, diff_opt_output },\ndiff --git a/diff.h b/diff.h\nindex 8ae18e5ab1e..90f7512034c 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -230,6 +230,12 @@ enum diff_submodule_format {\n \tDIFF_SUBMODULE_INLINE_DIFF\n };\n \n+enum diff_scope {\n+\tDIFF_SCOPE_NONE = 0,\n+\tDIFF_SCOPE_ALL,\n+\tDIFF_SCOPE_SPARSE,\n+};\n+\n /**\n * the set of options the calling program wants to affect the operation of\n * diffcore library with.\n@@ -285,6 +291,9 @@ struct diff_options {\n \t/* diff-filter bits */\n \tunsigned int filter, filter_not;\n \n+\t/* diff sparse-checkout scope */\n+\tenum diff_scope scope;\n+\n \tint use_color;\n \n \t/* Number of context lines to generate in patch output. */\n@@ -696,4 +705,6 @@ void print_stat_summary(FILE *fp, int files,\n \t\t\tint insertions, int deletions);\n void setup_diff_pager(struct diff_options *);\n \n+int diff_path_in_sparse_checkout(const char *path);\n+int diff_paths_in_sparse_checkout(const char *one, const char *two);\n #endif /* DIFF_H */\ndiff --git a/t/t4070-diff-sparse-checkout-scope.sh b/t/t4070-diff-sparse-checkout-scope.sh\nnew file mode 100644\nindex 00000000000..dca75a3308b\n--- /dev/null\n+++ b/t/t4070-diff-sparse-checkout-scope.sh\n@@ -0,0 +1,469 @@\n+#!/bin/sh\n+\n+test_description='diff sparse-checkout scope'\n+\n+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n+\n+. ./test-lib.sh\n+\n+\n+test_expect_success 'setup' '\n+\tgit init temp &&\n+\t(\n+\t\tcd temp &&\n+\t\tmkdir sub1 &&\n+\t\tmkdir sub2 &&\n+\t\techo sub1/file1 >sub1/file1 &&\n+\t\techo sub2/file2 >sub2/file2 &&\n+\t\techo file1 >file1 &&\n+\t\techo file2 >file2 &&\n+\t\tgit add --all &&\n+\t\tgit commit -m init &&\n+\t\techo sub1/file1 >>sub1/file1 &&\n+\t\techo sub1/file2 >>sub1/file2 &&\n+\t\techo sub2/file1 >>sub2/file1 &&\n+\t\techo sub2/file2 >>sub2/file2 &&\n+\t\techo file1 >>file1 &&\n+\t\techo file2 >>file2 &&\n+\t\tgit add --all &&\n+\t\tgit commit -m change1 &&\n+\t\techo sub1/file1 >>sub1/file1 &&\n+\t\techo sub1/file2 >>sub1/file2 &&\n+\t\techo sub2/file1 >>sub2/file1 &&\n+\t\techo sub2/file2 >>sub2/file2 &&\n+\t\techo file1 >>file1 &&\n+\t\techo file2 >>file2 &&\n+\t\tgit add --all &&\n+\t\tgit commit -m change2\n+\t)\n+'\n+\n+reset_repo () {\n+\trm -rf repo &&\n+\tgit clone --no-checkout temp repo\n+}\n+\n+reset_with_sparse_checkout() {\n+\treset_repo &&\n+\tgit -C repo sparse-checkout set $1 sub1 &&\n+\tgit -C repo checkout\n+}\n+\n+change_worktree_and_index() {\n+\t(\n+\t\tcd repo &&\n+\t\tmkdir sub2 sub3 &&\n+\t\techo sub1/file3 >sub1/file3 &&\n+\t\techo sub2/file3 >sub2/file3 &&\n+\t\techo sub3/file3 >sub3/file3 &&\n+\t\techo file3 >file3 &&\n+\t\tgit add --all --sparse &&\n+\t\techo sub1/file3 >>sub1/file3 &&\n+\t\techo sub2/file3 >>sub2/file3 &&\n+\t\techo sub3/file3 >>sub3/file3 &&\n+\t\techo file3 >>file3\n+\t)\n+}\n+\n+diff_scope() {\n+\ttitle=$1\n+\tneed_change_worktree_and_index=$2\n+\tsparse_checkout_option=$3\n+\tscope_option=$4\n+\texpect=$5\n+\tshift 5\n+\targs=(\"$@\")\n+\n+\ttest_expect_success \"$title $sparse_checkout_option $scope_option\" \"\n+\t\treset_with_sparse_checkout $sparse_checkout_option &&\n+\t\tif test \\\"$need_change_worktree_and_index\\\" = \\\"true\\\" ; then\n+\t\t\tchange_worktree_and_index\n+\t\tfi &&\n+\t\tgit -C repo diff $scope_option ${args[*]} >actual &&\n+\t\tif test -z \\\"$expect\\\" ; then\n+\t\t\t>expect\n+\t\telse\n+\t\t\tcat > expect <<-EOF\n+$expect\n+\t\t\tEOF\n+\t\tfi &&\n+\t\ttest_cmp expect actual\n+\t\"\n+}\n+\n+args=(\"--name-only\" \"HEAD\" \"HEAD~\")\n+diff_scope builtin_diff_tree false \"--no-cone\" \"--scope=sparse\" \\\n+\"sub1/file1\n+sub1/file2\" \"${args[@]}\"\n+\n+diff_scope builtin_diff_tree false \"--no-cone\" \"--scope=all\" \\\n+\"file1\n+file2\n+sub1/file1\n+sub1/file2\n+sub2/file1\n+sub2/file2\" \"${args[@]}\"\n+\n+diff_scope builtin_diff_tree false \"--no-cone\" \"--no-scope\" \\\n+\"file1\n+file2\n+sub1/file1\n+sub1/file2\n+sub2/file1\n+sub2/file2\" \"${args[@]}\"\n+\n+diff_scope builtin_diff_tree false \"--cone\" \"--scope=sparse\" \\\n+\"file1\n+file2\n+sub1/file1\n+sub1/file2\" \"${args[@]}\"\n+\n+diff_scope builtin_diff_tree false \"--cone\" \"--scope=all\" \\\n+\"file1\n+file2\n+sub1/file1\n+sub1/file2\n+sub2/file1\n+sub2/file2\" \"${args[@]}\"\n+\n+diff_scope builtin_diff_tree false \"--cone\" \"--no-scope\" \\\n+\"file1\n+file2\n+sub1/file1\n+sub1/file2\n+sub2/file1\n+sub2/file2\" \"${args[@]}\"\n+\n+args=(\"--name-only\" \"HEAD~\")\n+diff_scope builtin_diff_index true \"--no-cone\" \"--scope=sparse\" \\\n+\"sub1/file1\n+sub1/file2\n+sub1/file3\" \"${args[@]}\"\n+\n+diff_scope builtin_diff_index true \"--no-cone\" \"--scope=all\" \\\n+\"file1\n+file2\n+file3\n+sub1/file1\n+sub1/file2\n+sub1/file3\n+sub2/file1\n+sub2/file2\n+sub2/file3\n+sub3/file3\" \"${args[@]}\"\n+\n+diff_scope builtin_diff_index true \"--no-cone\" \"--no-scope\" \\\n+\"file1\n+file2\n+file3\n+sub1/file1\n+sub1/file2\n+sub1/file3\n+sub2/file1\n+sub2/file2\n+sub2/file3\n+sub3/file3\" \"${args[@]}\"\n+\n+diff_scope builtin_diff_index true \"--cone\" \"--scope=sparse\" \\\n+\"file1\n+file2\n+file3\n+sub1/file1\n+sub1/file2\n+sub1/file3\" \"${args[@]}\"\n+\n+diff_scope builtin_diff_index true \"--cone\" \"--scope=all\" \\\n+\"file1\n+file2\n+file3\n+sub1/file1\n+sub1/file2\n+sub1/file3\n+sub2/file1\n+sub2/file2\n+sub2/file3\n+sub3/file3\" \"${args[@]}\"\n+\n+diff_scope builtin_diff_index true \"--cone\" \"--no-scope\" \\\n+\"file1\n+file2\n+file3\n+sub1/file1\n+sub1/file2\n+sub1/file3\n+sub2/file1\n+sub2/file2\n+sub2/file3\n+sub3/file3\" \"${args[@]}\"\n+\n+args=(\"--name-only\" \"file3\" \"sub1/\" \"sub2/\")\n+\n+diff_scope builtin_diff_files true \"--no-cone\" \"--scope=sparse\" \\\n+\"sub1/file3\" \"${args[@]}\"\n+\n+diff_scope builtin_diff_files true \"--no-cone\" \"--scope=all\" \\\n+\"file3\n+sub1/file3\n+sub2/file1\n+sub2/file2\n+sub2/file3\" \"${args[@]}\"\n+\n+diff_scope builtin_diff_files true \"--no-cone\" \"--no-scope\" \\\n+\"file3\n+sub1/file3\n+sub2/file3\" \"${args[@]}\"\n+\n+diff_scope builtin_diff_files true \"--cone\" \"--scope=sparse\" \\\n+\"file3\n+sub1/file3\" \"${args[@]}\"\n+\n+diff_scope builtin_diff_files true \"--cone\" \"--scope=all\" \\\n+\"file3\n+sub1/file3\n+sub2/file1\n+sub2/file2\n+sub2/file3\" \"${args[@]}\"\n+\n+diff_scope builtin_diff_files true \"--cone\" \"--no-scope\" \\\n+\"file3\n+sub1/file3\n+sub2/file3\" \"${args[@]}\"\n+\n+\n+args=(\"--name-only\" \"HEAD~:sub2/file2\" \"sub1/file2\")\n+\n+diff_scope builtin_diff_b_f true \"--no-cone\" \"--scope=sparse\" \\\n+\"\" \"${args[@]}\"\n+\n+diff_scope builtin_diff_b_f true \"--no-cone\" \"--scope=all\" \\\n+\"sub1/file2\" \"${args[@]}\"\n+\n+diff_scope builtin_diff_b_f true \"--no-cone\" \"--no-scope\" \\\n+\"sub1/file2\" \"${args[@]}\"\n+\n+args=(\"--name-only\" \"HEAD~:sub1/file1\" \"file3\")\n+\n+diff_scope builtin_diff_b_f true \"--cone\" \"--scope=sparse\" \\\n+\"file3\" \"${args[@]}\"\n+\n+diff_scope builtin_diff_b_f true \"--cone\" \"--scope=all\" \\\n+\"file3\" \"${args[@]}\"\n+\n+diff_scope builtin_diff_b_f true \"--cone\" \"--no-scope\" \\\n+\"file3\" \"${args[@]}\"\n+\n+args=(\"--name-only\" HEAD~:sub2/file2 HEAD:sub1/file2)\n+\n+diff_scope builtin_diff_blobs true \"--no-cone\" \"--scope=sparse\" \\\n+\"\" \"${args[@]}\"\n+\n+diff_scope builtin_diff_blobs true \"--no-cone\" \"--scope=all\" \\\n+\"sub1/file2\" \"${args[@]}\"\n+\n+diff_scope builtin_diff_blobs true \"--no-cone\" \"--no-scope\" \\\n+\"sub1/file2\" \"${args[@]}\"\n+\n+args=(\"--name-only\" HEAD~:sub1/file1 HEAD:file2)\n+\n+diff_scope builtin_diff_blobs false \"--cone\" \"--scope=sparse\" \\\n+\"file2\" \"${args[@]}\"\n+\n+diff_scope builtin_diff_blobs false \"--cone\" \"--scope=all\" \\\n+\"file2\" \"${args[@]}\"\n+\n+diff_scope builtin_diff_blobs false \"--cone\" \"--no-scope\" \\\n+\"file2\" \"${args[@]}\"\n+\n+args=(\"--name-only\" HEAD~2 HEAD~ HEAD)\n+\n+diff_scope builtin_diff_combined false \"--no-cone\" \"--scope=sparse\" \\\n+\"sub1/file1\n+sub1/file2\" \"${args[@]}\"\n+\n+diff_scope builtin_diff_combined false \"--no-cone\" \"--scope=all\" \\\n+\"file1\n+file2\n+sub1/file1\n+sub1/file2\n+sub2/file1\n+sub2/file2\" \"${args[@]}\"\n+\n+diff_scope builtin_diff_combined false \"--no-cone\" \"--no-scope\" \\\n+\"file1\n+file2\n+sub1/file1\n+sub1/file2\n+sub2/file1\n+sub2/file2\" \"${args[@]}\"\n+\n+diff_scope builtin_diff_combined false \"--cone\" \"--scope=sparse\" \\\n+\"file1\n+file2\n+sub1/file1\n+sub1/file2\" \"${args[@]}\"\n+\n+diff_scope builtin_diff_combined false \"--cone\" \"--scope=all\" \\\n+\"file1\n+file2\n+sub1/file1\n+sub1/file2\n+sub2/file1\n+sub2/file2\" \"${args[@]}\"\n+\n+diff_scope builtin_diff_combined false \"--cone\" \"--no-scope\" \\\n+\"file1\n+file2\n+sub1/file1\n+sub1/file2\n+sub2/file1\n+sub2/file2\" \"${args[@]}\"\n+\n+test_expect_success 'diff_no_index --no-cone, --scope=sparse' '\n+\treset_with_sparse_checkout --no-cone &&\n+\t(\n+\t\tcd repo &&\n+\t\tmkdir sub3 &&\n+\t\techo sub3/file3 >sub3/file3\n+\t) &&\n+\ttest_expect_code 1 git -C repo diff --no-index --name-only --scope=sparse sub1/file1 sub1/file2 >actual &&\n+\tcat > expect <<-EOF &&\n+sub1/file2\n+\tEOF\n+\ttest_expect_code 1 git -C repo diff --no-index --name-only --scope=sparse sub1/file1 sub3/file3 >actual &&\n+\t>expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'diff_no_index --no-cone, --scope=all' '\n+\treset_with_sparse_checkout --no-cone &&\n+\t(\n+\t\tcd repo &&\n+\t\tmkdir sub3 &&\n+\t\techo sub3/file3 >sub3/file3\n+\t) &&\n+\ttest_expect_code 1 git -C repo diff --no-index --name-only --scope=all sub1/file1 sub1/file2 >actual &&\n+\tcat > expect <<-EOF &&\n+sub1/file2\n+\tEOF\n+\ttest_expect_code 1 git -C repo diff --no-index --name-only --scope=all sub1/file1 sub3/file3 >actual &&\n+\tcat > expect <<-EOF &&\n+sub3/file3\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'diff_no_index --no-cone, --no-scope' '\n+\treset_with_sparse_checkout --no-cone &&\n+\t(\n+\t\tcd repo &&\n+\t\tmkdir sub3 &&\n+\t\techo sub3/file3 >sub3/file3\n+\t) &&\n+\ttest_expect_code 1 git -C repo diff --no-index --name-only --no-scope sub1/file1 sub1/file2 >actual &&\n+\tcat > expect <<-EOF &&\n+sub1/file2\n+\tEOF\n+\ttest_expect_code 1 git -C repo diff --no-index --name-only --no-scope sub1/file1 sub3/file3 >actual &&\n+\tcat > expect <<-EOF &&\n+sub3/file3\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'diff_no_index --cone, --scope=sparse' '\n+\treset_with_sparse_checkout --cone &&\n+\t(\n+\t\tcd repo &&\n+\t\techo file3 >file3 &&\n+\t\tmkdir sub3 &&\n+\t\techo sub3/file3 >sub3/file3\n+\t) &&\n+\ttest_expect_code 1 git -C repo diff --no-index --name-only --scope=sparse sub1/file1 file3 >actual &&\n+\tcat > expect <<-EOF &&\n+file3\n+\tEOF\n+\ttest_expect_code 1 git -C repo diff --no-index --name-only --scope=sparse sub1/file1 sub3/file3 >actual &&\n+\t>expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'diff_no_index --cone, --scope=all' '\n+\treset_with_sparse_checkout --cone &&\n+\t(\n+\t\tcd repo &&\n+\t\techo file3 >file3 &&\n+\t\tmkdir sub3 &&\n+\t\techo sub3/file3 >sub3/file3\n+\t) &&\n+\ttest_expect_code 1 git -C repo diff --no-index --name-only --scope=all sub1/file1 file3 >actual &&\n+\tcat > expect <<-EOF &&\n+file3\n+\tEOF\n+\ttest_expect_code 1 git -C repo diff --no-index --name-only --scope=all sub1/file1 sub3/file3 >actual &&\n+\tcat > expect <<-EOF &&\n+sub3/file3\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'diff_no_index --cone, --no-scope' '\n+\treset_with_sparse_checkout --cone &&\n+\t(\n+\t\tcd repo &&\n+\t\techo file3 >file3 &&\n+\t\tmkdir sub3 &&\n+\t\techo sub3/file3 >sub3/file3\n+\t) &&\n+\ttest_expect_code 1 git -C repo diff --no-index --name-only --no-scope sub1/file1 file3 >actual &&\n+\tcat > expect <<-EOF &&\n+file3\n+\tEOF\n+\ttest_expect_code 1 git -C repo diff --no-index --name-only --no-scope sub1/file1 sub3/file3 >actual &&\n+\tcat > expect <<-EOF &&\n+sub3/file3\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'diff scope config sparse' '\n+\treset_with_sparse_checkout --cone &&\n+\tgit -C repo -c diff.scope=sparse diff --name-only HEAD~ >actual &&\n+\tcat > expect <<-EOF &&\n+file1\n+file2\n+sub1/file1\n+sub1/file2\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'diff scope config all' '\n+\treset_with_sparse_checkout --cone &&\n+\tgit -C repo -c diff.scope=all diff --name-only HEAD~ >actual &&\n+\tcat > expect <<-EOF &&\n+file1\n+file2\n+sub1/file1\n+sub1/file2\n+sub2/file1\n+sub2/file2\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'diff scope config override by option' '\n+\treset_with_sparse_checkout --cone &&\n+\tgit -C repo -c diff.scope=sparse diff --name-only --scope=all HEAD~ >actual &&\n+\tcat > expect <<-EOF &&\n+file1\n+file2\n+sub1/file1\n+sub1/file2\n+sub2/file1\n+sub2/file2\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_done\ndiff --git a/tree-diff.c b/tree-diff.c\nindex 69031d7cbae..67f99c8e4df 100644\n--- a/tree-diff.c\n+++ b/tree-diff.c\n@@ -76,6 +76,11 @@ static int tree_entry_pathcmp(struct tree_desc *t1, struct tree_desc *t2)\n static int emit_diff_first_parent_only(struct diff_options *opt, struct combine_diff_path *p)\n {\n \tstruct combine_diff_parent *p0 = &p->parent[0];\n+\n+\tif (opt->scope == DIFF_SCOPE_SPARSE &&\n+\t !diff_path_in_sparse_checkout(p->path))\n+\t\treturn 0;\n+\n \tif (p->mode && p0->mode) {\n \t\topt->change(opt, p0->mode, p->mode, &p0->oid, &p->oid,\n \t\t\t1, 1, p->path, 0, 0);\n\nbase-commit: 63bba4fdd86d80ef061c449daa97a981a9be0792\n-- \ngitgitgadget\n"},{"id":"466163","messageId":"Y2B3j3sFcd/mJoHx@nand.local","threadId":"58721","inReplyTo":"pull.1398.git.1667189512579.gitgitgadget@gmail.com","subject":"Re: [PATCH] [RFC] diff: introduce scope option","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-01T01:34:07Z","receivedAt":"2022-11-01T01:34:13Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 31, 2022 at 04:11:52AM +0000, ZheNing Hu via GitGitGadget wrote:\n> t/t4070-diff-sparse-checkout-scope.sh | 469 ++++++++++++++++++++++++++\n\nIt looks like this test is non-executable, leading to the following\nerror:\n\n ~/s/git [nand] (zh/diff--scope) $ make test\n SUBDIR templates\n make -C t/ all\n make[1]: Entering directory '/home/ttaylorr/src/git/t'\n rm -f -r 'test-results'\n non-executable tests: t4070-diff-sparse-checkout-scope.sh\n make[1]: *** [Makefile:120: test-lint-executable] Error 1\n make[1]: *** Waiting for unfinished jobs....\n make[1]: Leaving directory '/home/ttaylorr/src/git/t'\n make: *** [Makefile:3083: test] Error 2\n\nThanks,\nTaylor\n"},{"id":"466166","messageId":"CAOLTT8RZEsJcNquSU0FQMZvhA++eZAkzMq9um9YkkruCJc3aiA@mail.gmail.com","threadId":"58721","inReplyTo":"Y2B3j3sFcd/mJoHx@nand.local","subject":"Re: [PATCH] [RFC] diff: introduce scope option","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2022-11-01T02:13:50Z","receivedAt":"2022-11-01T02:14:04Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Taylor Blau 于2022年11月1日周二 09:34写道：\n>\n> On Mon, Oct 31, 2022 at 04:11:52AM +0000, ZheNing Hu via GitGitGadget wrote:\n> > t/t4070-diff-sparse-checkout-scope.sh | 469 ++++++++++++++++++++++++++\n>\n> It looks like this test is non-executable, leading to the following\n> error:\n>\n> ~/s/git [nand] (zh/diff--scope) $ make test\n> SUBDIR templates\n> make -C t/ all\n> make[1]: Entering directory '/home/ttaylorr/src/git/t'\n> rm -f -r 'test-results'\n> non-executable tests: t4070-diff-sparse-checkout-scope.sh\n> make[1]: *** [Makefile:120: test-lint-executable] Error 1\n> make[1]: *** Waiting for unfinished jobs....\n> make[1]: Leaving directory '/home/ttaylorr/src/git/t'\n> make: *** [Makefile:3083: test] Error 2\n>\n\nThanks for pointing out that I had overlooked this problem.\n\n> Thanks,\n> Taylor\n\nZheNing Hu\n"},{"id":"466178","messageId":"CABPp-BGUXKk-LSJtHP2jSDSVYNpQgzOeferx6xJ36ntDgrBNCw@mail.gmail.com","threadId":"58721","inReplyTo":"pull.1398.git.1667189512579.gitgitgadget@gmail.com","subject":"Re: [PATCH] [RFC] diff: introduce scope option","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-11-01T05:18:00Z","receivedAt":"2022-11-01T05:18:58Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi,\n\nOn Sun, Oct 30, 2022 at 9:11 PM ZheNing Hu via GitGitGadget\n wrote:\n>\n> From: ZheNing Hu \n>\n> When we use sparse-checkout, we often want the set of files\n> that some commands operate on to be restricted to the\n> sparse-checkout specification.\n\nIt seems a bit premature to send this, when the guideline document[*]\ndetailing how these options should work is still in the \"Needs Review\"\nstate. I know, it's been waiting for a while, but it's a _long_\ndocument.\n\n[*] https://lore.kernel.org/git/pull.1367.v3.git.1665269538608.gitgitgadget@gmail.com/\n\n> So introduce the `--scope` option to git diff, which have two\n> value: \"sparse\" and \"all\". \"sparse\" mean that diff is performed\n> restrict to paths which matching sparse-checkout specification,\n> \"all\" mean that diff is performed regardless of whether the path\n> meets the sparse-checkout specification.\n\nThe wording probably needs some care to reflect the fact that it only\naffects cases where either --cached or revisions are passed. In\nparticular, your wording for \"all\" suggests behavior very different\nfrom today, whereas \"all\" is probably best thought of as today's\ncurrent behavior. For example, a plain `git diff` without --cached or\nrevisions, should be unaffected by either of these flags.\n\n> `--no-scope` is the default\n> option for now.\n\nWhat does --no-scope even mean? You didn't explain it, and I don't\nsee how it could make sense. We explicitly avoided a `--no-` prefix\nby allowing the --scope option to take a value. I don't think there\nshould be a --no-scope option.\n\n> Add `diff.scope={sparse, all}` config, which can also have the same\n> capabilities as `--scope`, and it will be covered by `--scope` option.\n\nThis is not what we want. The high level usecases should not need to\nbe configured per-command. There should be a config option which\nreflects the high level use cases (e.g. sparse.scope) and then all\nrelevant commands (diff, log, grep, etc.) can key off it.\n\n> Signed-off-by: ZheNing Hu \n> ---\n> [RFC] diff: introduce scope option\n>\n> In [1], we discovered that users working on different sparse-checkout\n> specification may download unnecessary blobs from each other's\n> specification in collaboration. In [2] Junio suggested that maybe we can\n> restrict some git command's filespec in sparse-checkout specification to\n> elegantly solve this problem above. In [3]: Newren and Derrick Stolee\n> prefer to name the option --scope={sparse, all}.\n>\n> So this patch is attempt to do this thing on git diff:\n>\n> v1:\n>\n> 1. add --restrict option to git diff, which restrict diff filespec in\n> sparse-checkout specification. [4] v2.\n> 2. rename --restrict to --scope={sparse, all}, support --no-scope.\n> 3. add config: diff.scope={sparse,all}.\n>\n> Unresolved work:\n>\n> 1. how to properly pass this --scope={sparse, all} to other commands\n> like git log, git format-patch, etc.\n\nlog & grep should accept a similar flag. format-patch should not, and\nshould ignore any config in this area.\n\n> 2. how to set the default value of scope for different diff commands.\n\nI don't understand this.\n\n> [1]:\n> https://lore.kernel.org/git/CAOLTT8SHo66kGbvWr=+LQ9UVd1NHgqGGEYK2qq6==QgRCgLZqQ@mail.gmail.com/\n> [2]: https://lore.kernel.org/git/xmqqzgeqw0sy.fsf@gitster.g/ [3]:\n> https://lore.kernel.org/git/07a25d48-e364-0d9b-6ffa-41a5984eb5db@github.com/\n> [4]:\n> https://lore.kernel.org/git/pull.1368.git.1664036052741.gitgitgadget@gmail.com/\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1398%2Fadlternative%2Fzh%2Fdiff-scope-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1398/adlternative/zh/diff-scope-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1398\n>\n> Documentation/config/diff.txt | 12 +\n> Documentation/diff-options.txt | 18 +\n> builtin/diff.c | 4 +\n> diff-lib.c | 36 +-\n> diff-no-index.c | 4 +\n> diff.c | 39 +++\n> diff.h | 11 +\n> t/t4070-diff-sparse-checkout-scope.sh | 469 ++++++++++++++++++++++++++\n> tree-diff.c | 5 +\n> 9 files changed, 597 insertions(+), 1 deletion(-)\n> create mode 100644 t/t4070-diff-sparse-checkout-scope.sh\n>\n> diff --git a/Documentation/config/diff.txt b/Documentation/config/diff.txt\n> index 35a7bf86d77..52707e1b2d6 100644\n> --- a/Documentation/config/diff.txt\n> +++ b/Documentation/config/diff.txt\n> @@ -201,6 +201,18 @@ diff.algorithm::\n> --\n> +\n>\n> +diff.scope::\n> + Choose diff scope. The variants are as follows:\n> ++\n> +--\n> +`sparse`;;\n> + Restrict diff paths to those matching sparse-checkout specification.\n> +`all`;;\n> + Without restriction, diff is performed regardless of whether the path\n> + meets the sparse-checkout specification.\n\nAs noted above, this is the wrong level to specify things. The\ndescription for \"all\" is misleading as well and suggests something\nother than \"behavior B\" from the direction document.\n\n> diff.wsErrorHighlight::\n> Highlight whitespace errors in the `context`, `old` or `new`\n> lines of the diff. Multiple values are separated by comma,\n> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\n> index 3674ac48e92..04bf83e8be1 100644\n> --- a/Documentation/diff-options.txt\n> +++ b/Documentation/diff-options.txt\n> @@ -195,6 +195,24 @@ For instance, if you configured the `diff.algorithm` variable to a\n> non-default value and want to use the default one, then you\n> have to use `--diff-algorithm=default` option.\n>\n> +ifndef::git-format-patch[]\n> +ifndef::git-log[]\n> +\n> +--scope={sparse|all}::\n> + Choose diff scope. The variants are as follows:\n> ++\n> +--\n> +`--sparse`;;\n> + Restrict diff paths to those matching sparse-checkout specification.\n> +`--all`;;\n> + Without restriction, diff is performed regardless of whether the path\n> + meets the sparse-checkout specification.\n> +--\n> ++\n> +\n> +endif::git-log[]\n> +endif::git-format-patch[]\n\nWhat about diff-files, diff-index, diff-tree, and show?\n\n> +\n> --stat[=[,[,]]]::\n> Generate a diffstat. By default, as much space as necessary\n> will be used for the filename part, and the rest for the graph\n> diff --git a/builtin/diff.c b/builtin/diff.c\n> index 854d2c5a5c4..6b450f7184c 100644\n> --- a/builtin/diff.c\n> +++ b/builtin/diff.c\n> @@ -54,6 +54,10 @@ static void stuff_change(struct diff_options *opt,\n> oideq(old_oid, new_oid) && (old_mode == new_mode))\n> return;\n>\n> + if (opt->scope == DIFF_SCOPE_SPARSE &&\n> + !diff_paths_in_sparse_checkout(old_path, new_path))\n> + return;\n\nThis can't be right.\n git diff c231e0f26fe9b2ea9ec46aa68ff95ba984ce592e\n72d42bd856228c15f702fa3c353432f4f1defe03\n(to directly diff two known blobs) will go through this function, with\nold_path == c231e0f26fe9b2ea9ec46aa68ff95ba984ce592e and new_path ==\n72d42bd856228c15f702fa3c353432f4f1defe03. But those aren't real\npaths, and sparse-checkout should not restrict what is shown in those\ncases.\n\n> +\n> if (opt->flags.reverse_diff) {\n> SWAP(old_mode, new_mode);\n> SWAP(old_oid, new_oid);\n> diff --git a/diff-lib.c b/diff-lib.c\n> index 2edea41a234..a3381f2e0ff 100644\n> --- a/diff-lib.c\n> +++ b/diff-lib.c\n> @@ -88,6 +88,22 @@ static int match_stat_with_submodule(struct diff_options *diffopt,\n> return changed;\n> }\n>\n> +int diff_path_in_sparse_checkout(const char *path) {\n> + if (core_sparse_checkout_cone)\n> + return path_in_cone_mode_sparse_checkout(path, the_repository->index);\n> + else\n> + return path_in_sparse_checkout(path, the_repository->index);\n> +}\n\nThis says we are including the path if it matches the sparsity\npatterns. Thus, we have to be careful when we use this function,\nbecause the relevant paths are ones that match the sparsity\nspecification. The sparsity specification will always match the\nsparsity patterns when diffing two commits, but when either the index\nor the working tree is part of the diff, the sparsity specification\n*might* be temporarily expanded.\n\n> +int diff_paths_in_sparse_checkout(const char *one, const char*two) {\n> + if (one == two || !strcmp(one, two))\n> + return diff_path_in_sparse_checkout(one);\n> + else\n> + return diff_path_in_sparse_checkout(one) &&\n> + diff_path_in_sparse_checkout(two);\n\nWhy && rather than || ?\n\n> +}\n> +\n> +\n> int run_diff_files(struct rev_info *revs, unsigned int option)\n> {\n> int entries, i;\n> @@ -113,6 +129,9 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n>\n> if (diff_can_quit_early(&revs->diffopt))\n> break;\n> + if (revs->diffopt.scope == DIFF_SCOPE_SPARSE &&\n> + !diff_path_in_sparse_checkout(ce->name))\n> + continue;\n\nHere you've cut off the possibility of showing diffs for anything\noutside the sparsity patterns, which is a mistake. We need to handle\na temporarily expanded sparse specification too.\n\n> if (!ce_path_match(istate, ce, &revs->prune_data, NULL))\n> continue;\n> @@ -202,7 +221,8 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n> continue;\n> }\n>\n> - if (ce_uptodate(ce) || ce_skip_worktree(ce))\n> + if (ce_uptodate(ce) ||\n> + (revs->diffopt.scope != DIFF_SCOPE_ALL && ce_skip_worktree(ce)))\n> continue;\n\nHere you make --scope=all show files even if they are skip-worktree,\nmaking them appear to have been deleted. I called out your\ndescription earlier as potentially misleading, because it could imply\nthis behavior. It looks like you were consistent with the description\nand implementation, it just doesn't match what we want.\n\n> /*\n> @@ -439,6 +459,20 @@ static void do_oneway_diff(struct unpack_trees_options *o,\n\ndo_oneway_diff is for cases where we are diffing against the index...\n\n> return; /* nothing to diff.. */\n> }\n>\n> + if (revs->diffopt.scope == DIFF_SCOPE_SPARSE) {\n> + if (idx && tree) {\n> + if (!diff_paths_in_sparse_checkout(idx->name, tree->name))\n> + return;\n> + } else if (idx) {\n> + if (!diff_path_in_sparse_checkout(idx->name))\n> + return;\n> + } else if (tree) {\n> + if (!diff_path_in_sparse_checkout(tree->name))\n> + return;\n> + } else\n> + return;\n> + }\n\n...and you again mistakenly only compare to the sparsity patterns\ninstead of the sparse specification.\n\n> +\n> /* if the entry is not checked out, don't examine work tree */\n> cached = o->index_only ||\n> (idx && ((idx->ce_flags & CE_VALID) || ce_skip_worktree(idx)));\n\n\n\n> diff --git a/diff-no-index.c b/diff-no-index.c\n> index 18edbdf4b59..ea94a104ea4 100644\n> --- a/diff-no-index.c\n> +++ b/diff-no-index.c\n> @@ -281,6 +281,10 @@ int diff_no_index(struct rev_info *revs,\n>\n> fixup_paths(paths, &replacement);\n>\n> + if (revs->diffopt.scope == DIFF_SCOPE_SPARSE &&\n> + !diff_paths_in_sparse_checkout(paths[0], paths[1]))\n> + goto out;\n\n--no-index means we're diffing two files that are not tracked, or at\nleast treating them as not tracked. sparse-checkout should not affect\nsuch files.\n\n> +\n> revs->diffopt.skip_stat_unmatch = 1;\n> if (!revs->diffopt.output_format)\n> revs->diffopt.output_format = DIFF_FORMAT_PATCH;\n> diff --git a/diff.c b/diff.c\n> index 285d6e2d575..9de4044ae05 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -48,6 +48,7 @@ static int diff_interhunk_context_default;\n> static const char *diff_word_regex_cfg;\n> static const char *external_diff_cmd_cfg;\n> static const char *diff_order_file_cfg;\n> +static const char *external_diff_scope_cfg;\n> int diff_auto_refresh_index = 1;\n> static int diff_mnemonic_prefix;\n> static int diff_no_prefix;\n> @@ -57,6 +58,7 @@ static int diff_dirstat_permille_default = 30;\n> static struct diff_options default_diff_options;\n> static long diff_algorithm;\n> static unsigned ws_error_highlight_default = WSEH_NEW;\n> +static enum diff_scope external_diff_scope;\n\nWhy is this called \"external\"?\n\n> static char diff_colors[][COLOR_MAXLEN] = {\n> GIT_COLOR_RESET,\n> @@ -423,6 +425,16 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)\n> return 0;\n> }\n>\n> + if (!strcmp(var, \"diff.scope\")) {\n> + git_config_string(&external_diff_scope_cfg, var, value);\n> + if (!strcmp(value, \"all\"))\n> + external_diff_scope = DIFF_SCOPE_ALL;\n> + else if (!strcmp(value, \"sparse\"))\n> + external_diff_scope = DIFF_SCOPE_SPARSE;\n> + else\n> + return -1;\n> + }\n> +\n> if (git_color_config(var, value, cb) < 0)\n> return -1;\n>\n> @@ -4663,6 +4675,7 @@ void repo_diff_setup(struct repository *r, struct diff_options *options)\n>\n> options->color_moved = diff_color_moved_default;\n> options->color_moved_ws_handling = diff_color_moved_ws_default;\n> + options->scope = external_diff_scope;\n>\n> prep_parse_options(options);\n> }\n> @@ -4914,6 +4927,29 @@ static int parse_dirstat_opt(struct diff_options *options, const char *params)\n> return 1;\n> }\n>\n> +static int diff_opt_diff_scope(const struct option *option,\n> + const char *optarg, int unset)\n> +{\n> + struct diff_options *opt = option->value;\n> +\n> + if (unset) {\n> + opt->scope = DIFF_SCOPE_NONE;\n\nI think we should instead have a\n BUG_ON_OPT_NEG(unset)\nor, even better, a\n BUG_ON_OPT_NEG_NOARG(unset, optarg)\nat the beginning of this function...\n\n> + } else if (optarg) {\n\n...which would also allow you to drop this if and dedent the rest of\nthe function.\n\n> + if (!strcmp(optarg, \"all\")) {\n> + if (core_apply_sparse_checkout) {\n> + opt->scope = DIFF_SCOPE_ALL;\n> + }\n> + } else if (!strcmp(optarg, \"sparse\")) {\n> + if (core_apply_sparse_checkout) {\n> + opt->scope = DIFF_SCOPE_SPARSE;\n> + }\n\nIf core_apply_sparse_checkout is false, should we perhaps throw an\nerror instead of just silently ignoring the option the user passed?\n\n> + } else\n> + return error(_(\"invalid --scope value: %s\"), optarg);\n> + }\n\nAs written with no follow-on else clause here, wouldn't this silently\naccept \"--scope\" without an \"=\" argument without an error\nand without properly initializing opt->scope?\n\n> +\n> + return 0;\n> +}\n> +\n> static int diff_opt_diff_filter(const struct option *option,\n> const char *optarg, int unset)\n> {\n> @@ -5683,6 +5719,9 @@ static void prep_parse_options(struct diff_options *options)\n> OPT_CALLBACK_F(0, \"diff-filter\", options, N_(\"[(A|C|D|M|R|T|U|X|B)...[*]]\"),\n> N_(\"select files by diff type\"),\n> PARSE_OPT_NONEG, diff_opt_diff_filter),\n> + OPT_CALLBACK_F(0, \"scope\", options, N_(\"[sparse|all]\"),\n> + N_(\"choose diff scope\"),\n\nmaybe \"choose diff scope in sparse checkouts\"?\n\n> + PARSE_OPT_OPTARG, diff_opt_diff_scope),\n> { OPTION_CALLBACK, 0, \"output\", options, N_(\"\"),\n> N_(\"output to a specific file\"),\n> PARSE_OPT_NONEG, NULL, 0, diff_opt_output },\n> diff --git a/diff.h b/diff.h\n> index 8ae18e5ab1e..90f7512034c 100644\n> --- a/diff.h\n> +++ b/diff.h\n> @@ -230,6 +230,12 @@ enum diff_submodule_format {\n> DIFF_SUBMODULE_INLINE_DIFF\n> };\n>\n> +enum diff_scope {\n> + DIFF_SCOPE_NONE = 0,\n> + DIFF_SCOPE_ALL,\n> + DIFF_SCOPE_SPARSE,\n> +};\n> +\n> /**\n> * the set of options the calling program wants to affect the operation of\n> * diffcore library with.\n> @@ -285,6 +291,9 @@ struct diff_options {\n> /* diff-filter bits */\n> unsigned int filter, filter_not;\n>\n> + /* diff sparse-checkout scope */\n> + enum diff_scope scope;\n> +\n> int use_color;\n>\n> /* Number of context lines to generate in patch output. */\n> @@ -696,4 +705,6 @@ void print_stat_summary(FILE *fp, int files,\n> int insertions, int deletions);\n> void setup_diff_pager(struct diff_options *);\n>\n> +int diff_path_in_sparse_checkout(const char *path);\n> +int diff_paths_in_sparse_checkout(const char *one, const char *two);\n> #endif /* DIFF_H */\n> diff --git a/t/t4070-diff-sparse-checkout-scope.sh b/t/t4070-diff-sparse-checkout-scope.sh\n> new file mode 100644\n\nThis needs to be fixed.\n\n> index 00000000000..dca75a3308b\n> --- /dev/null\n> +++ b/t/t4070-diff-sparse-checkout-scope.sh\n> @@ -0,0 +1,469 @@\n> +#!/bin/sh\n> +\n> +test_description='diff sparse-checkout scope'\n> +\n> +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n> +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n> +\n> +. ./test-lib.sh\n> +\n> +\n> +test_expect_success 'setup' '\n> + git init temp &&\n> + (\n> + cd temp &&\n> + mkdir sub1 &&\n> + mkdir sub2 &&\n> + echo sub1/file1 >sub1/file1 &&\n> + echo sub2/file2 >sub2/file2 &&\n> + echo file1 >file1 &&\n> + echo file2 >file2 &&\n> + git add --all &&\n> + git commit -m init &&\n> + echo sub1/file1 >>sub1/file1 &&\n> + echo sub1/file2 >>sub1/file2 &&\n> + echo sub2/file1 >>sub2/file1 &&\n> + echo sub2/file2 >>sub2/file2 &&\n> + echo file1 >>file1 &&\n> + echo file2 >>file2 &&\n> + git add --all &&\n> + git commit -m change1 &&\n> + echo sub1/file1 >>sub1/file1 &&\n> + echo sub1/file2 >>sub1/file2 &&\n> + echo sub2/file1 >>sub2/file1 &&\n> + echo sub2/file2 >>sub2/file2 &&\n> + echo file1 >>file1 &&\n> + echo file2 >>file2 &&\n> + git add --all &&\n> + git commit -m change2\n> + )\n> +'\n> +\n> +reset_repo () {\n> + rm -rf repo &&\n> + git clone --no-checkout temp repo\n\nWhy --no-checkout rather than say --sparse?\n\n> +}\n> +\n> +reset_with_sparse_checkout() {\n> + reset_repo &&\n> + git -C repo sparse-checkout set $1 sub1 &&\n> + git -C repo checkout\n\nFixing the above would let us get rid of this really weird extra\ncheckout command too.\n\n> +}\n> +\n> +change_worktree_and_index() {\n> + (\n> + cd repo &&\n> + mkdir sub2 sub3 &&\n> + echo sub1/file3 >sub1/file3 &&\n> + echo sub2/file3 >sub2/file3 &&\n> + echo sub3/file3 >sub3/file3 &&\n> + echo file3 >file3 &&\n> + git add --all --sparse &&\n> + echo sub1/file3 >>sub1/file3 &&\n> + echo sub2/file3 >>sub2/file3 &&\n> + echo sub3/file3 >>sub3/file3 &&\n> + echo file3 >>file3\n> + )\n> +}\n\nIt would be nice to modify different paths in the working tree and\nindex, to see if we can handle cases where the sparse specification\ndoes not match the sparsity patterns better. (So, modify files inside\nand outside the sparsity patterns, stage those changes, and then do a\n`git sparse-checkout reapply` to make the files outside the sparsity\npatterns disappear from the working tree...then modify different files\nin the working tree both inside and outside the sparsity patterns.\nAnd also remove some file that matches the sparsity patterns and\nmanually mark it as SKIP_WORKTREE.) That'd give us much better\ncoverage.\n\n> +\n> +diff_scope() {\n> + title=$1\n> + need_change_worktree_and_index=$2\n> + sparse_checkout_option=$3\n> + scope_option=$4\n> + expect=$5\n> + shift 5\n> + args=(\"$@\")\n> +\n> + test_expect_success \"$title $sparse_checkout_option $scope_option\" \"\n> + reset_with_sparse_checkout $sparse_checkout_option &&\n> + if test \\\"$need_change_worktree_and_index\\\" = \\\"true\\\" ; then\n> + change_worktree_and_index\n> + fi &&\n> + git -C repo diff $scope_option ${args[*]} >actual &&\n> + if test -z \\\"$expect\\\" ; then\n> + >expect\n> + else\n> + cat > expect <<-EOF\n> +$expect\n> + EOF\n> + fi &&\n> + test_cmp expect actual\n> + \"\n> +}\n> +\n> +args=(\"--name-only\" \"HEAD\" \"HEAD~\")\n> +diff_scope builtin_diff_tree false \"--no-cone\" \"--scope=sparse\" \\\n> +\"sub1/file1\n> +sub1/file2\" \"${args[@]}\"\n> +\n> +diff_scope builtin_diff_tree false \"--no-cone\" \"--scope=all\" \\\n> +\"file1\n> +file2\n> +sub1/file1\n> +sub1/file2\n> +sub2/file1\n> +sub2/file2\" \"${args[@]}\"\n> +\n> +diff_scope builtin_diff_tree false \"--no-cone\" \"--no-scope\" \\\n> +\"file1\n> +file2\n> +sub1/file1\n> +sub1/file2\n> +sub2/file1\n> +sub2/file2\" \"${args[@]}\"\n> +\n> +diff_scope builtin_diff_tree false \"--cone\" \"--scope=sparse\" \\\n> +\"file1\n> +file2\n> +sub1/file1\n> +sub1/file2\" \"${args[@]}\"\n> +\n> +diff_scope builtin_diff_tree false \"--cone\" \"--scope=all\" \\\n> +\"file1\n> +file2\n> +sub1/file1\n> +sub1/file2\n> +sub2/file1\n> +sub2/file2\" \"${args[@]}\"\n> +\n> +diff_scope builtin_diff_tree false \"--cone\" \"--no-scope\" \\\n> +\"file1\n> +file2\n> +sub1/file1\n> +sub1/file2\n> +sub2/file1\n> +sub2/file2\" \"${args[@]}\"\n> +\n> +args=(\"--name-only\" \"HEAD~\")\n> +diff_scope builtin_diff_index true \"--no-cone\" \"--scope=sparse\" \\\n> +\"sub1/file1\n> +sub1/file2\n> +sub1/file3\" \"${args[@]}\"\n\nHere's a good example where the testcase is doing the wrong thing.\nThe expected answer here would also include file3, sub2/file3, and\nsub3/file3.\n\n> +\n> +diff_scope builtin_diff_index true \"--no-cone\" \"--scope=all\" \\\n> +\"file1\n> +file2\n> +file3\n> +sub1/file1\n> +sub1/file2\n> +sub1/file3\n> +sub2/file1\n> +sub2/file2\n> +sub2/file3\n> +sub3/file3\" \"${args[@]}\"\n> +\n> +diff_scope builtin_diff_index true \"--no-cone\" \"--no-scope\" \\\n> +\"file1\n> +file2\n> +file3\n> +sub1/file1\n> +sub1/file2\n> +sub1/file3\n> +sub2/file1\n> +sub2/file2\n> +sub2/file3\n> +sub3/file3\" \"${args[@]}\"\n> +\n> +diff_scope builtin_diff_index true \"--cone\" \"--scope=sparse\" \\\n> +\"file1\n> +file2\n> +file3\n> +sub1/file1\n> +sub1/file2\n> +sub1/file3\" \"${args[@]}\"\n\nThis is also wrong; it's missing sub2/file3 and sub3/file3.\n\n> +\n> +diff_scope builtin_diff_index true \"--cone\" \"--scope=all\" \\\n> +\"file1\n> +file2\n> +file3\n> +sub1/file1\n> +sub1/file2\n> +sub1/file3\n> +sub2/file1\n> +sub2/file2\n> +sub2/file3\n> +sub3/file3\" \"${args[@]}\"\n> +\n> +diff_scope builtin_diff_index true \"--cone\" \"--no-scope\" \\\n> +\"file1\n> +file2\n> +file3\n> +sub1/file1\n> +sub1/file2\n> +sub1/file3\n> +sub2/file1\n> +sub2/file2\n> +sub2/file3\n> +sub3/file3\" \"${args[@]}\"\n> +\n> +args=(\"--name-only\" \"file3\" \"sub1/\" \"sub2/\")\n> +\n> +diff_scope builtin_diff_files true \"--no-cone\" \"--scope=sparse\" \\\n> +\"sub1/file3\" \"${args[@]}\"\n\nThis should also include file3, sub2/file3, and sub3/file3.\n`--scope=` should not affect diff output at all if neither --cached\nnor revision arguments are supplied.\n\n> +\n> +diff_scope builtin_diff_files true \"--no-cone\" \"--scope=all\" \\\n> +\"file3\n> +sub1/file3\n> +sub2/file1\n> +sub2/file2\n> +sub2/file3\" \"${args[@]}\"\n\nThis is wrong due to including too much; it should not include\nsub2/file1 or sub2/file2 (it is only including those because it is\nshowing them as deleted, when they are not deleted but are\nSKIP_WORKTREE).\n\nI think I'm going to stop reviewing here. I'm probably just going to\nkeep repeating the same issues I identified earlier if I continue.\n"},{"id":"466622","messageId":"CAOLTT8TceM-NpV2_hUCZj2Dx=W30f_9SHW8CcRH-pw32BRd-oA@mail.gmail.com","threadId":"58721","inReplyTo":"CABPp-BGUXKk-LSJtHP2jSDSVYNpQgzOeferx6xJ36ntDgrBNCw@mail.gmail.com","subject":"Re: [PATCH] [RFC] diff: introduce scope option","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2022-11-06T02:11:02Z","receivedAt":"2022-11-06T02:17:19Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":" inHi,\n\nElijah Newren 于2022年11月1日周二 13:18写道：\n>\n> Hi,\n>\n> On Sun, Oct 30, 2022 at 9:11 PM ZheNing Hu via GitGitGadget\n> wrote:\n> >\n> > From: ZheNing Hu \n> >\n> > When we use sparse-checkout, we often want the set of files\n> > that some commands operate on to be restricted to the\n> > sparse-checkout specification.\n>\n> It seems a bit premature to send this, when the guideline document[*]\n> detailing how these options should work is still in the \"Needs Review\"\n> state. I know, it's been waiting for a while, but it's a _long_\n> document.\n>\n> [*] https://lore.kernel.org/git/pull.1367.v3.git.1665269538608.gitgitgadget@gmail.com/\n>\n\nFine, I just want to start trying to experiment with this feature in\ngit-diff earlier,\nand I can wait for the sparse-checkout.txt documentation to finish\nfirst if needed :)\n\n> > So introduce the `--scope` option to git diff, which have two\n> > value: \"sparse\" and \"all\". \"sparse\" mean that diff is performed\n> > restrict to paths which matching sparse-checkout specification,\n> > \"all\" mean that diff is performed regardless of whether the path\n> > meets the sparse-checkout specification.\n>\n> The wording probably needs some care to reflect the fact that it only\n> affects cases where either --cached or revisions are passed. In\n> particular, your wording for \"all\" suggests behavior very different\n> from today, whereas \"all\" is probably best thought of as today's\n> current behavior. For example, a plain `git diff` without --cached or\n> revisions, should be unaffected by either of these flags.\n>\n\nYes, after re-reading your sparse-checkout.txt patch, I realized that I\nmisinterpreted \"--scope=sparse\" as sparse patterns instead of sparse\nspecification, and misinterpreted \"-scope=all\" as diff on all files.\n\n> > `--no-scope` is the default\n> > option for now.\n>\n> What does --no-scope even mean? You didn't explain it, and I don't\n> see how it could make sense. We explicitly avoided a `--no-` prefix\n> by allowing the --scope option to take a value. I don't think there\n> should be a --no-scope option.\n>\n\nI think the “--no-scope” here does nothing, as if it were unaffected by scope\n(just like correctly \"--scope=all\", right?)\n\n> > Add `diff.scope={sparse, all}` config, which can also have the same\n> > capabilities as `--scope`, and it will be covered by `--scope` option.\n>\n> This is not what we want. The high level usecases should not need to\n> be configured per-command. There should be a config option which\n> reflects the high level use cases (e.g. sparse.scope) and then all\n> relevant commands (diff, log, grep, etc.) can key off it.\n>\n\nOk, using a global config should indeed be more useful.\n\n> > Signed-off-by: ZheNing Hu \n> > ---\n> > [RFC] diff: introduce scope option\n> >\n> > In [1], we discovered that users working on different sparse-checkout\n> > specification may download unnecessary blobs from each other's\n> > specification in collaboration. In [2] Junio suggested that maybe we can\n> > restrict some git command's filespec in sparse-checkout specification to\n> > elegantly solve this problem above. In [3]: Newren and Derrick Stolee\n> > prefer to name the option --scope={sparse, all}.\n> >\n> > So this patch is attempt to do this thing on git diff:\n> >\n> > v1:\n> >\n> > 1. add --restrict option to git diff, which restrict diff filespec in\n> > sparse-checkout specification. [4] v2.\n> > 2. rename --restrict to --scope={sparse, all}, support --no-scope.\n> > 3. add config: diff.scope={sparse,all}.\n> >\n> > Unresolved work:\n> >\n> > 1. how to properly pass this --scope={sparse, all} to other commands\n> > like git log, git format-patch, etc.\n>\n> log & grep should accept a similar flag. format-patch should not, and\n> should ignore any config in this area.\n>\n> > 2. how to set the default value of scope for different diff commands.\n>\n> I don't understand this.\n>\n\nOh, I was just curious if the config defaults for scope needed to be configured\nseparately for the different diff commands git diff-files, git diff-index,\ngit diff-no-index, git diff-tree, since sparse-checkout.txt mentions\nthe different\nbehavior of scope for them. Now I think this just needs to be handled in command\ncode logic and no additional command level configuration is needed.\n\n> > [1]:\n> > https://lore.kernel.org/git/CAOLTT8SHo66kGbvWr=+LQ9UVd1NHgqGGEYK2qq6==QgRCgLZqQ@mail.gmail.com/\n> > [2]: https://lore.kernel.org/git/xmqqzgeqw0sy.fsf@gitster.g/ [3]:\n> > https://lore.kernel.org/git/07a25d48-e364-0d9b-6ffa-41a5984eb5db@github.com/\n> > [4]:\n> > https://lore.kernel.org/git/pull.1368.git.1664036052741.gitgitgadget@gmail.com/\n> >\n> > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1398%2Fadlternative%2Fzh%2Fdiff-scope-v1\n> > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1398/adlternative/zh/diff-scope-v1\n> > Pull-Request: https://github.com/gitgitgadget/git/pull/1398\n> >\n> > Documentation/config/diff.txt | 12 +\n> > Documentation/diff-options.txt | 18 +\n> > builtin/diff.c | 4 +\n> > diff-lib.c | 36 +-\n> > diff-no-index.c | 4 +\n> > diff.c | 39 +++\n> > diff.h | 11 +\n> > t/t4070-diff-sparse-checkout-scope.sh | 469 ++++++++++++++++++++++++++\n> > tree-diff.c | 5 +\n> > 9 files changed, 597 insertions(+), 1 deletion(-)\n> > create mode 100644 t/t4070-diff-sparse-checkout-scope.sh\n> >\n> > diff --git a/Documentation/config/diff.txt b/Documentation/config/diff.txt\n> > index 35a7bf86d77..52707e1b2d6 100644\n> > --- a/Documentation/config/diff.txt\n> > +++ b/Documentation/config/diff.txt\n> > @@ -201,6 +201,18 @@ diff.algorithm::\n> > --\n> > +\n> >\n> > +diff.scope::\n> > + Choose diff scope. The variants are as follows:\n> > ++\n> > +--\n> > +`sparse`;;\n> > + Restrict diff paths to those matching sparse-checkout specification.\n> > +`all`;;\n> > + Without restriction, diff is performed regardless of whether the path\n> > + meets the sparse-checkout specification.\n>\n> As noted above, this is the wrong level to specify things. The\n> description for \"all\" is misleading as well and suggests something\n> other than \"behavior B\" from the direction document.\n>\n\nSo do we need to make \"--scope=all\" correspond to the \"behavior B\",\nThe correct description of it should be: \"worktree-sparse-history-dense\"...\n\n> > diff.wsErrorHighlight::\n> > Highlight whitespace errors in the `context`, `old` or `new`\n> > lines of the diff. Multiple values are separated by comma,\n> > diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\n> > index 3674ac48e92..04bf83e8be1 100644\n> > --- a/Documentation/diff-options.txt\n> > +++ b/Documentation/diff-options.txt\n> > @@ -195,6 +195,24 @@ For instance, if you configured the `diff.algorithm` variable to a\n> > non-default value and want to use the default one, then you\n> > have to use `--diff-algorithm=default` option.\n> >\n> > +ifndef::git-format-patch[]\n> > +ifndef::git-log[]\n> > +\n> > +--scope={sparse|all}::\n> > + Choose diff scope. The variants are as follows:\n> > ++\n> > +--\n> > +`--sparse`;;\n> > + Restrict diff paths to those matching sparse-checkout specification.\n> > +`--all`;;\n> > + Without restriction, diff is performed regardless of whether the path\n> > + meets the sparse-checkout specification.\n> > +--\n> > ++\n> > +\n> > +endif::git-log[]\n> > +endif::git-format-patch[]\n>\n> What about diff-files, diff-index, diff-tree, and show?\n>\n\ndiff-options.txt included in their documents, and git-format-patch.txt,\ngit-log.txt, but should not in git-format-patch.txt. I don't know if it\nshould be included in git-diff-files.txt, because git diff-files compare\nthe files in the working tree and the index (so it's the same as\n\"simple\" git-diff, which should not be affected by scope?)\n\n> > +\n> > --stat[=[,[,]]]::\n> > Generate a diffstat. By default, as much space as necessary\n> > will be used for the filename part, and the rest for the graph\n> > diff --git a/builtin/diff.c b/builtin/diff.c\n> > index 854d2c5a5c4..6b450f7184c 100644\n> > --- a/builtin/diff.c\n> > +++ b/builtin/diff.c\n> > @@ -54,6 +54,10 @@ static void stuff_change(struct diff_options *opt,\n> > oideq(old_oid, new_oid) && (old_mode == new_mode))\n> > return;\n> >\n> > + if (opt->scope == DIFF_SCOPE_SPARSE &&\n> > + !diff_paths_in_sparse_checkout(old_path, new_path))\n> > + return;\n>\n> This can't be right.\n> git diff c231e0f26fe9b2ea9ec46aa68ff95ba984ce592e\n> 72d42bd856228c15f702fa3c353432f4f1defe03\n> (to directly diff two known blobs) will go through this function, with\n> old_path == c231e0f26fe9b2ea9ec46aa68ff95ba984ce592e and new_path ==\n> 72d42bd856228c15f702fa3c353432f4f1defe03. But those aren't real\n> paths, and sparse-checkout should not restrict what is shown in those\n> cases.\n>\n\nYeah, it's buggy that I forget to check two blobs without paths.\n\n> > +\n> > if (opt->flags.reverse_diff) {\n> > SWAP(old_mode, new_mode);\n> > SWAP(old_oid, new_oid);\n> > diff --git a/diff-lib.c b/diff-lib.c\n> > index 2edea41a234..a3381f2e0ff 100644\n> > --- a/diff-lib.c\n> > +++ b/diff-lib.c\n> > @@ -88,6 +88,22 @@ static int match_stat_with_submodule(struct diff_options *diffopt,\n> > return changed;\n> > }\n> >\n> > +int diff_path_in_sparse_checkout(const char *path) {\n> > + if (core_sparse_checkout_cone)\n> > + return path_in_cone_mode_sparse_checkout(path, the_repository->index);\n> > + else\n> > + return path_in_sparse_checkout(path, the_repository->index);\n> > +}\n>\n> This says we are including the path if it matches the sparsity\n> patterns. Thus, we have to be careful when we use this function,\n> because the relevant paths are ones that match the sparsity\n> specification. The sparsity specification will always match the\n> sparsity patterns when diffing two commits, but when either the index\n> or the working tree is part of the diff, the sparsity specification\n> *might* be temporarily expanded.\n>\n\nYes, I may have to look at more code to better understand how and when the\n\"sparsity specification\" is extended. Any recommendations for places to read?\n\n> > +int diff_paths_in_sparse_checkout(const char *one, const char*two) {\n> > + if (one == two || !strcmp(one, two))\n> > + return diff_path_in_sparse_checkout(one);\n> > + else\n> > + return diff_path_in_sparse_checkout(one) &&\n> > + diff_path_in_sparse_checkout(two);\n>\n> Why && rather than || ?\n>\n\nAgree, we do need to use || here, because the one diff side in the sparse\nspecification, we should diff the two files.\n\n> > +}\n> > +\n> > +\n> > int run_diff_files(struct rev_info *revs, unsigned int option)\n> > {\n> > int entries, i;\n> > @@ -113,6 +129,9 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n> >\n> > if (diff_can_quit_early(&revs->diffopt))\n> > break;\n> > + if (revs->diffopt.scope == DIFF_SCOPE_SPARSE &&\n> > + !diff_path_in_sparse_checkout(ce->name))\n> > + continue;\n>\n> Here you've cut off the possibility of showing diffs for anything\n> outside the sparsity patterns, which is a mistake. We need to handle\n> a temporarily expanded sparse specification too.\n>\n\nAgree.\n\n> > if (!ce_path_match(istate, ce, &revs->prune_data, NULL))\n> > continue;\n> > @@ -202,7 +221,8 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n> > continue;\n> > }\n> >\n> > - if (ce_uptodate(ce) || ce_skip_worktree(ce))\n> > + if (ce_uptodate(ce) ||\n> > + (revs->diffopt.scope != DIFF_SCOPE_ALL && ce_skip_worktree(ce)))\n> > continue;\n>\n> Here you make --scope=all show files even if they are skip-worktree,\n> making them appear to have been deleted. I called out your\n> description earlier as potentially misleading, because it could imply\n> this behavior. It looks like you were consistent with the description\n> and implementation, it just doesn't match what we want.\n>\n\nAgree too. So we should not do anything in run_diff_files.\n\n> > /*\n> > @@ -439,6 +459,20 @@ static void do_oneway_diff(struct unpack_trees_options *o,\n>\n> do_oneway_diff is for cases where we are diffing against the index...\n>\n> > return; /* nothing to diff.. */\n> > }\n> >\n> > + if (revs->diffopt.scope == DIFF_SCOPE_SPARSE) {\n> > + if (idx && tree) {\n> > + if (!diff_paths_in_sparse_checkout(idx->name, tree->name))\n> > + return;\n> > + } else if (idx) {\n> > + if (!diff_path_in_sparse_checkout(idx->name))\n> > + return;\n> > + } else if (tree) {\n> > + if (!diff_path_in_sparse_checkout(tree->name))\n> > + return;\n> > + } else\n> > + return;\n> > + }\n>\n> ...and you again mistakenly only compare to the sparsity patterns\n> instead of the sparse specification.\n>\n\nSo here we should use ce_skip_worktree(idx) to check if this index entry matches\nsparse specification.\n\n> > +\n> > /* if the entry is not checked out, don't examine work tree */\n> > cached = o->index_only ||\n> > (idx && ((idx->ce_flags & CE_VALID) || ce_skip_worktree(idx)));\n>\n>\n>\n> > diff --git a/diff-no-index.c b/diff-no-index.c\n> > index 18edbdf4b59..ea94a104ea4 100644\n> > --- a/diff-no-index.c\n> > +++ b/diff-no-index.c\n> > @@ -281,6 +281,10 @@ int diff_no_index(struct rev_info *revs,\n> >\n> > fixup_paths(paths, &replacement);\n> >\n> > + if (revs->diffopt.scope == DIFF_SCOPE_SPARSE &&\n> > + !diff_paths_in_sparse_checkout(paths[0], paths[1]))\n> > + goto out;\n>\n> --no-index means we're diffing two files that are not tracked, or at\n> least treating them as not tracked. sparse-checkout should not affect\n> such files.\n>\n\nYeah, we should care about untracked files sparse-checkout only when\nwe are using git add/rm/update-index...\n\n> > +\n> > revs->diffopt.skip_stat_unmatch = 1;\n> > if (!revs->diffopt.output_format)\n> > revs->diffopt.output_format = DIFF_FORMAT_PATCH;\n> > diff --git a/diff.c b/diff.c\n> > index 285d6e2d575..9de4044ae05 100644\n> > --- a/diff.c\n> > +++ b/diff.c\n> > @@ -48,6 +48,7 @@ static int diff_interhunk_context_default;\n> > static const char *diff_word_regex_cfg;\n> > static const char *external_diff_cmd_cfg;\n> > static const char *diff_order_file_cfg;\n> > +static const char *external_diff_scope_cfg;\n> > int diff_auto_refresh_index = 1;\n> > static int diff_mnemonic_prefix;\n> > static int diff_no_prefix;\n> > @@ -57,6 +58,7 @@ static int diff_dirstat_permille_default = 30;\n> > static struct diff_options default_diff_options;\n> > static long diff_algorithm;\n> > static unsigned ws_error_highlight_default = WSEH_NEW;\n> > +static enum diff_scope external_diff_scope;\n>\n> Why is this called \"external\"?\n>\n\nLearn from external_diff_cmd_cfg, I should remove it.\n\n> > static char diff_colors[][COLOR_MAXLEN] = {\n> > GIT_COLOR_RESET,\n> > @@ -423,6 +425,16 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)\n> > return 0;\n> > }\n> >\n> > + if (!strcmp(var, \"diff.scope\")) {\n> > + git_config_string(&external_diff_scope_cfg, var, value);\n> > + if (!strcmp(value, \"all\"))\n> > + external_diff_scope = DIFF_SCOPE_ALL;\n> > + else if (!strcmp(value, \"sparse\"))\n> > + external_diff_scope = DIFF_SCOPE_SPARSE;\n> > + else\n> > + return -1;\n> > + }\n> > +\n> > if (git_color_config(var, value, cb) < 0)\n> > return -1;\n> >\n> > @@ -4663,6 +4675,7 @@ void repo_diff_setup(struct repository *r, struct diff_options *options)\n> >\n> > options->color_moved = diff_color_moved_default;\n> > options->color_moved_ws_handling = diff_color_moved_ws_default;\n> > + options->scope = external_diff_scope;\n> >\n> > prep_parse_options(options);\n> > }\n> > @@ -4914,6 +4927,29 @@ static int parse_dirstat_opt(struct diff_options *options, const char *params)\n> > return 1;\n> > }\n> >\n> > +static int diff_opt_diff_scope(const struct option *option,\n> > + const char *optarg, int unset)\n> > +{\n> > + struct diff_options *opt = option->value;\n> > +\n> > + if (unset) {\n> > + opt->scope = DIFF_SCOPE_NONE;\n>\n> I think we should instead have a\n> BUG_ON_OPT_NEG(unset)\n> or, even better, a\n> BUG_ON_OPT_NEG_NOARG(unset, optarg)\n> at the beginning of this function...\n>\n> > + } else if (optarg) {\n>\n> ...which would also allow you to drop this if and dedent the rest of\n> the function.\n>\n\nAgree.\n\n> > + if (!strcmp(optarg, \"all\")) {\n> > + if (core_apply_sparse_checkout) {\n> > + opt->scope = DIFF_SCOPE_ALL;\n> > + }\n> > + } else if (!strcmp(optarg, \"sparse\")) {\n> > + if (core_apply_sparse_checkout) {\n> > + opt->scope = DIFF_SCOPE_SPARSE;\n> > + }\n>\n> If core_apply_sparse_checkout is false, should we perhaps throw an\n> error instead of just silently ignoring the option the user passed?\n>\n\nAgree.\n\n> > + } else\n> > + return error(_(\"invalid --scope value: %s\"), optarg);\n> > + }\n>\n> As written with no follow-on else clause here, wouldn't this silently\n> accept \"--scope\" without an \"=\" argument without an error\n> and without properly initializing opt->scope?\n>\n\nBecause opt will be initializing with default_diff_options in repo_diff_setup(),\nand opt->scope should respect config value first. So I don't think there's a\nmistake here.\n\n> > +\n> > + return 0;\n> > +}\n> > +\n> > static int diff_opt_diff_filter(const struct option *option,\n> > const char *optarg, int unset)\n> > {\n> > @@ -5683,6 +5719,9 @@ static void prep_parse_options(struct diff_options *options)\n> > OPT_CALLBACK_F(0, \"diff-filter\", options, N_(\"[(A|C|D|M|R|T|U|X|B)...[*]]\"),\n> > N_(\"select files by diff type\"),\n> > PARSE_OPT_NONEG, diff_opt_diff_filter),\n> > + OPT_CALLBACK_F(0, \"scope\", options, N_(\"[sparse|all]\"),\n> > + N_(\"choose diff scope\"),\n>\n> maybe \"choose diff scope in sparse checkouts\"?\n>\n\nOK.\n\n> > + PARSE_OPT_OPTARG, diff_opt_diff_scope),\n> > { OPTION_CALLBACK, 0, \"output\", options, N_(\"\"),\n> > N_(\"output to a specific file\"),\n> > PARSE_OPT_NONEG, NULL, 0, diff_opt_output },\n> > diff --git a/diff.h b/diff.h\n> > index 8ae18e5ab1e..90f7512034c 100644\n> > --- a/diff.h\n> > +++ b/diff.h\n> > @@ -230,6 +230,12 @@ enum diff_submodule_format {\n> > DIFF_SUBMODULE_INLINE_DIFF\n> > };\n> >\n> > +enum diff_scope {\n> > + DIFF_SCOPE_NONE = 0,\n> > + DIFF_SCOPE_ALL,\n> > + DIFF_SCOPE_SPARSE,\n> > +};\n> > +\n> > /**\n> > * the set of options the calling program wants to affect the operation of\n> > * diffcore library with.\n> > @@ -285,6 +291,9 @@ struct diff_options {\n> > /* diff-filter bits */\n> > unsigned int filter, filter_not;\n> >\n> > + /* diff sparse-checkout scope */\n> > + enum diff_scope scope;\n> > +\n> > int use_color;\n> >\n> > /* Number of context lines to generate in patch output. */\n> > @@ -696,4 +705,6 @@ void print_stat_summary(FILE *fp, int files,\n> > int insertions, int deletions);\n> > void setup_diff_pager(struct diff_options *);\n> >\n> > +int diff_path_in_sparse_checkout(const char *path);\n> > +int diff_paths_in_sparse_checkout(const char *one, const char *two);\n> > #endif /* DIFF_H */\n> > diff --git a/t/t4070-diff-sparse-checkout-scope.sh b/t/t4070-diff-sparse-checkout-scope.sh\n> > new file mode 100644\n>\n> This needs to be fixed.\n>\n> > index 00000000000..dca75a3308b\n> > --- /dev/null\n> > +++ b/t/t4070-diff-sparse-checkout-scope.sh\n> > @@ -0,0 +1,469 @@\n> > +#!/bin/sh\n> > +\n> > +test_description='diff sparse-checkout scope'\n> > +\n> > +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n> > +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n> > +\n> > +. ./test-lib.sh\n> > +\n> > +\n> > +test_expect_success 'setup' '\n> > + git init temp &&\n> > + (\n> > + cd temp &&\n> > + mkdir sub1 &&\n> > + mkdir sub2 &&\n> > + echo sub1/file1 >sub1/file1 &&\n> > + echo sub2/file2 >sub2/file2 &&\n> > + echo file1 >file1 &&\n> > + echo file2 >file2 &&\n> > + git add --all &&\n> > + git commit -m init &&\n> > + echo sub1/file1 >>sub1/file1 &&\n> > + echo sub1/file2 >>sub1/file2 &&\n> > + echo sub2/file1 >>sub2/file1 &&\n> > + echo sub2/file2 >>sub2/file2 &&\n> > + echo file1 >>file1 &&\n> > + echo file2 >>file2 &&\n> > + git add --all &&\n> > + git commit -m change1 &&\n> > + echo sub1/file1 >>sub1/file1 &&\n> > + echo sub1/file2 >>sub1/file2 &&\n> > + echo sub2/file1 >>sub2/file1 &&\n> > + echo sub2/file2 >>sub2/file2 &&\n> > + echo file1 >>file1 &&\n> > + echo file2 >>file2 &&\n> > + git add --all &&\n> > + git commit -m change2\n> > + )\n> > +'\n> > +\n> > +reset_repo () {\n> > + rm -rf repo &&\n> > + git clone --no-checkout temp repo\n>\n> Why --no-checkout rather than say --sparse?\n>\n\nThis is because I accidentally associated it with a\npartial clone. I often use \"git clone -filter=blob:none -no-checkout\"\nto do a partial clone, then \"git sparse- checkout set \"\nto set sparse-checkout patterns, and \"git checkout\" to download\nthe missing blobs and checkout to a branch. But in this\ntest file, we only need sparse-checkout, so it's true that I should\nnot do such strange no-checkout thing.\n\n> > +}\n> > +\n> > +reset_with_sparse_checkout() {\n> > + reset_repo &&\n> > + git -C repo sparse-checkout set $1 sub1 &&\n> > + git -C repo checkout\n>\n> Fixing the above would let us get rid of this really weird extra\n> checkout command too.\n>\n> > +}\n> > +\n> > +change_worktree_and_index() {\n> > + (\n> > + cd repo &&\n> > + mkdir sub2 sub3 &&\n> > + echo sub1/file3 >sub1/file3 &&\n> > + echo sub2/file3 >sub2/file3 &&\n> > + echo sub3/file3 >sub3/file3 &&\n> > + echo file3 >file3 &&\n> > + git add --all --sparse &&\n> > + echo sub1/file3 >>sub1/file3 &&\n> > + echo sub2/file3 >>sub2/file3 &&\n> > + echo sub3/file3 >>sub3/file3 &&\n> > + echo file3 >>file3\n> > + )\n> > +}\n>\n> It would be nice to modify different paths in the working tree and\n> index, to see if we can handle cases where the sparse specification\n> does not match the sparsity patterns better. (So, modify files inside\n> and outside the sparsity patterns, stage those changes, and then do a\n> `git sparse-checkout reapply` to make the files outside the sparsity\n> patterns disappear from the working tree...then modify different files\n> in the working tree both inside and outside the sparsity patterns.\n> And also remove some file that matches the sparsity patterns and\n> manually mark it as SKIP_WORKTREE.) That'd give us much better\n> coverage.\n>\n\nNice addition. So I should use git update-index --skip-worktree to set\nskip_worktree bit for index entries.\n\n> > +\n> > +diff_scope() {\n> > + title=$1\n> > + need_change_worktree_and_index=$2\n> > + sparse_checkout_option=$3\n> > + scope_option=$4\n> > + expect=$5\n> > + shift 5\n> > + args=(\"$@\")\n> > +\n> > + test_expect_success \"$title $sparse_checkout_option $scope_option\" \"\n> > + reset_with_sparse_checkout $sparse_checkout_option &&\n> > + if test \\\"$need_change_worktree_and_index\\\" = \\\"true\\\" ; then\n> > + change_worktree_and_index\n> > + fi &&\n> > + git -C repo diff $scope_option ${args[*]} >actual &&\n> > + if test -z \\\"$expect\\\" ; then\n> > + >expect\n> > + else\n> > + cat > expect <<-EOF\n> > +$expect\n> > + EOF\n> > + fi &&\n> > + test_cmp expect actual\n> > + \"\n> > +}\n> > +\n> > +args=(\"--name-only\" \"HEAD\" \"HEAD~\")\n> > +diff_scope builtin_diff_tree false \"--no-cone\" \"--scope=sparse\" \\\n> > +\"sub1/file1\n> > +sub1/file2\" \"${args[@]}\"\n> > +\n> > +diff_scope builtin_diff_tree false \"--no-cone\" \"--scope=all\" \\\n> > +\"file1\n> > +file2\n> > +sub1/file1\n> > +sub1/file2\n> > +sub2/file1\n> > +sub2/file2\" \"${args[@]}\"\n> > +\n> > +diff_scope builtin_diff_tree false \"--no-cone\" \"--no-scope\" \\\n> > +\"file1\n> > +file2\n> > +sub1/file1\n> > +sub1/file2\n> > +sub2/file1\n> > +sub2/file2\" \"${args[@]}\"\n> > +\n> > +diff_scope builtin_diff_tree false \"--cone\" \"--scope=sparse\" \\\n> > +\"file1\n> > +file2\n> > +sub1/file1\n> > +sub1/file2\" \"${args[@]}\"\n> > +\n> > +diff_scope builtin_diff_tree false \"--cone\" \"--scope=all\" \\\n> > +\"file1\n> > +file2\n> > +sub1/file1\n> > +sub1/file2\n> > +sub2/file1\n> > +sub2/file2\" \"${args[@]}\"\n> > +\n> > +diff_scope builtin_diff_tree false \"--cone\" \"--no-scope\" \\\n> > +\"file1\n> > +file2\n> > +sub1/file1\n> > +sub1/file2\n> > +sub2/file1\n> > +sub2/file2\" \"${args[@]}\"\n> > +\n> > +args=(\"--name-only\" \"HEAD~\")\n> > +diff_scope builtin_diff_index true \"--no-cone\" \"--scope=sparse\" \\\n> > +\"sub1/file1\n> > +sub1/file2\n> > +sub1/file3\" \"${args[@]}\"\n>\n> Here's a good example where the testcase is doing the wrong thing.\n> The expected answer here would also include file3, sub2/file3, and\n> sub3/file3.\n>\n\nYeah. Files that are not part of the sparse-checkout patterns are temporarily\nextended into the sparse specification.\n\n> > +\n> > +diff_scope builtin_diff_index true \"--no-cone\" \"--scope=all\" \\\n> > +\"file1\n> > +file2\n> > +file3\n> > +sub1/file1\n> > +sub1/file2\n> > +sub1/file3\n> > +sub2/file1\n> > +sub2/file2\n> > +sub2/file3\n> > +sub3/file3\" \"${args[@]}\"\n> > +\n> > +diff_scope builtin_diff_index true \"--no-cone\" \"--no-scope\" \\\n> > +\"file1\n> > +file2\n> > +file3\n> > +sub1/file1\n> > +sub1/file2\n> > +sub1/file3\n> > +sub2/file1\n> > +sub2/file2\n> > +sub2/file3\n> > +sub3/file3\" \"${args[@]}\"\n> > +\n> > +diff_scope builtin_diff_index true \"--cone\" \"--scope=sparse\" \\\n> > +\"file1\n> > +file2\n> > +file3\n> > +sub1/file1\n> > +sub1/file2\n> > +sub1/file3\" \"${args[@]}\"\n>\n> This is also wrong; it's missing sub2/file3 and sub3/file3.\n>\n> > +\n> > +diff_scope builtin_diff_index true \"--cone\" \"--scope=all\" \\\n> > +\"file1\n> > +file2\n> > +file3\n> > +sub1/file1\n> > +sub1/file2\n> > +sub1/file3\n> > +sub2/file1\n> > +sub2/file2\n> > +sub2/file3\n> > +sub3/file3\" \"${args[@]}\"\n> > +\n> > +diff_scope builtin_diff_index true \"--cone\" \"--no-scope\" \\\n> > +\"file1\n> > +file2\n> > +file3\n> > +sub1/file1\n> > +sub1/file2\n> > +sub1/file3\n> > +sub2/file1\n> > +sub2/file2\n> > +sub2/file3\n> > +sub3/file3\" \"${args[@]}\"\n> > +\n> > +args=(\"--name-only\" \"file3\" \"sub1/\" \"sub2/\")\n> > +\n> > +diff_scope builtin_diff_files true \"--no-cone\" \"--scope=sparse\" \\\n> > +\"sub1/file3\" \"${args[@]}\"\n>\n> This should also include file3, sub2/file3, and sub3/file3.\n> `--scope=` should not affect diff output at all if neither --cached\n> nor revision arguments are supplied.\n>\n\nAgree.\n\n> > +\n> > +diff_scope builtin_diff_files true \"--no-cone\" \"--scope=all\" \\\n> > +\"file3\n> > +sub1/file3\n> > +sub2/file1\n> > +sub2/file2\n> > +sub2/file3\" \"${args[@]}\"\n>\n> This is wrong due to including too much; it should not include\n> sub2/file1 or sub2/file2 (it is only including those because it is\n> showing them as deleted, when they are not deleted but are\n> SKIP_WORKTREE).\n>\n\nAgree.\n\n> I think I'm going to stop reviewing here. I'm probably just going to\n> keep repeating the same issues I identified earlier if I continue.\n\nThank you very much for your review, you have pointed out very many\nproblems with this patch :)\n\n--\nZheNing Hu\n"},{"id":"466624","messageId":"CABPp-BErUhzm5=c6Tn4nPFKx-Kx0tYwf-J=G+nOFJ2bu-r+Dhg@mail.gmail.com","threadId":"58721","inReplyTo":"CAOLTT8TceM-NpV2_hUCZj2Dx=W30f_9SHW8CcRH-pw32BRd-oA@mail.gmail.com","subject":"Re: [PATCH] [RFC] diff: introduce scope option","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-11-06T06:58:26Z","receivedAt":"2022-11-06T06:58:45Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi,\n\nOn Sat, Nov 5, 2022 at 7:11 PM ZheNing Hu wrote:\n> Elijah Newren 于2022年11月1日周二 13:18写道：\n> > On Sun, Oct 30, 2022 at 9:11 PM ZheNing Hu via GitGitGadget\n> > wrote:\n> > >\n> > > From: ZheNing Hu \n> > >\n> > > When we use sparse-checkout, we often want the set of files\n> > > that some commands operate on to be restricted to the\n> > > sparse-checkout specification.\n> >\n> > It seems a bit premature to send this, when the guideline document[*]\n> > detailing how these options should work is still in the \"Needs Review\"\n> > state. I know, it's been waiting for a while, but it's a _long_\n> > document.\n> >\n> > [*] https://lore.kernel.org/git/pull.1367.v3.git.1665269538608.gitgitgadget@gmail.com/\n> >\n>\n> Fine, I just want to start trying to experiment with this feature in\n> git-diff earlier,\n> and I can wait for the sparse-checkout.txt documentation to finish\n> first if needed :)\n\nNote that you may be able to help reduce the wait by reviewing the\ndocument. (You commented on v1 -- thanks! -- but no one has commented\non any of the newer versions, despite being out for over a month and\nstill being marked as \"Needs Review\" in the \"What's cooking\" reports.)\n\n> > > `--no-scope` is the default\n> > > option for now.\n> >\n> > What does --no-scope even mean? You didn't explain it, and I don't\n> > see how it could make sense. We explicitly avoided a `--no-` prefix\n> > by allowing the --scope option to take a value. I don't think there\n> > should be a --no-scope option.\n>\n> I think the “--no-scope” here does nothing, as if it were unaffected by scope\n> (just like correctly \"--scope=all\", right?)\n\nReading your patch, I was unable to determine where you made\n--no-scope behave differently than how you implemented --scope=all.\nMaybe there was a difference and I missed it.\n\nAnyway, I don't think there should be a --no-scope option.\n\n> > > diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\n> > > index 3674ac48e92..04bf83e8be1 100644\n> > > --- a/Documentation/diff-options.txt\n> > > +++ b/Documentation/diff-options.txt\n> > > @@ -195,6 +195,24 @@ For instance, if you configured the `diff.algorithm` variable to a\n> > > non-default value and want to use the default one, then you\n> > > have to use `--diff-algorithm=default` option.\n> > >\n> > > +ifndef::git-format-patch[]\n> > > +ifndef::git-log[]\n> > > +\n> > > +--scope={sparse|all}::\n> > > + Choose diff scope. The variants are as follows:\n> > > ++\n> > > +--\n> > > +`--sparse`;;\n> > > + Restrict diff paths to those matching sparse-checkout specification.\n> > > +`--all`;;\n> > > + Without restriction, diff is performed regardless of whether the path\n> > > + meets the sparse-checkout specification.\n> > > +--\n> > > ++\n> > > +\n> > > +endif::git-log[]\n> > > +endif::git-format-patch[]\n> >\n> > What about diff-files, diff-index, diff-tree, and show?\n> >\n>\n> diff-options.txt included in their documents, and git-format-patch.txt,\n> git-log.txt, but should not in git-format-patch.txt. I don't know if it\n> should be included in git-diff-files.txt, because git diff-files compare\n> the files in the working tree and the index (so it's the same as\n> \"simple\" git-diff, which should not be affected by scope?)\n\nOh, good point. Yeah, git-diff-files.txt should not get this option\nas it won't have any affect. git-diff-index and git-diff-tree and\ngit-show and git-log should get it, though.\n\n> > > +int diff_path_in_sparse_checkout(const char *path) {\n> > > + if (core_sparse_checkout_cone)\n> > > + return path_in_cone_mode_sparse_checkout(path, the_repository->index);\n> > > + else\n> > > + return path_in_sparse_checkout(path, the_repository->index);\n> > > +}\n> >\n> > This says we are including the path if it matches the sparsity\n> > patterns. Thus, we have to be careful when we use this function,\n> > because the relevant paths are ones that match the sparsity\n> > specification. The sparsity specification will always match the\n> > sparsity patterns when diffing two commits, but when either the index\n> > or the working tree is part of the diff, the sparsity specification\n> > *might* be temporarily expanded.\n>\n> Yes, I may have to look at more code to better understand how and when the\n> \"sparsity specification\" is extended. Any recommendations for places to read?\n\nYes, please review the newer version of my sparse-checkout.txt\ndirections file; it covers this in more detail.\n\n> > > @@ -439,6 +459,20 @@ static void do_oneway_diff(struct unpack_trees_options *o,\n> >\n> > do_oneway_diff is for cases where we are diffing against the index...\n> >\n> > > return; /* nothing to diff.. */\n> > > }\n> > >\n> > > + if (revs->diffopt.scope == DIFF_SCOPE_SPARSE) {\n> > > + if (idx && tree) {\n> > > + if (!diff_paths_in_sparse_checkout(idx->name, tree->name))\n> > > + return;\n> > > + } else if (idx) {\n> > > + if (!diff_path_in_sparse_checkout(idx->name))\n> > > + return;\n> > > + } else if (tree) {\n> > > + if (!diff_path_in_sparse_checkout(tree->name))\n> > > + return;\n> > > + } else\n> > > + return;\n> > > + }\n> >\n> > ...and you again mistakenly only compare to the sparsity patterns\n> > instead of the sparse specification.\n> >\n>\n> So here we should use ce_skip_worktree(idx) to check if this index entry matches\n> sparse specification.\n\nce_skip_worktree(idx) only checks for expansions to the sparse\nspecification in the working tree. If dealing with index files, the\nsparse specification is also expanded to handle all files in the index\nthat differ from HEAD.\n\n> > > + } else\n> > > + return error(_(\"invalid --scope value: %s\"), optarg);\n> > > + }\n> >\n> > As written with no follow-on else clause here, wouldn't this silently\n> > accept \"--scope\" without an \"=\" argument without an error\n> > and without properly initializing opt->scope?\n> >\n>\n> Because opt will be initializing with default_diff_options in repo_diff_setup(),\n> and opt->scope should respect config value first. So I don't think there's a\n> mistake here.\n\nOkay, it's good that you've got the variables initialized somehow, but\nthat's only half the point here. The main point is that the user can\nspecify something that makes no sense and if they do, we should throw\nan error informing them of their mistake. --scope should not be\npassed without an argument (either \"all\" or \"sparse\", currently), but\nthis code allowed it.\n\n> > > diff --git a/t/t4070-diff-sparse-checkout-scope.sh b/t/t4070-diff-sparse-checkout-scope.sh\n> > > new file mode 100644\n> >\n> > This needs to be fixed.\n\nSince you didn't comment on this but usually do comment on thing, I'll\njust re-ping it to make sure you don't miss the comment about the\nincorrect file mode here.\n\n> > > +reset_repo () {\n> > > + rm -rf repo &&\n> > > + git clone --no-checkout temp repo\n> >\n> > Why --no-checkout rather than say --sparse?\n> >\n>\n> This is because I accidentally associated it with a\n> partial clone. I often use \"git clone -filter=blob:none -no-checkout\"\n> to do a partial clone, then \"git sparse- checkout set \"\n> to set sparse-checkout patterns, and \"git checkout\" to download\n> the missing blobs and checkout to a branch. But in this\n> test file, we only need sparse-checkout, so it's true that I should\n> not do such strange no-checkout thing.\n\nYeah, no need to involve partial clones here. (As an aside, though, I\nthink even with a partial clone that --sparse makes more sense than\n--no-checkout.)\n\n> > > +change_worktree_and_index() {\n> > > + (\n> > > + cd repo &&\n> > > + mkdir sub2 sub3 &&\n> > > + echo sub1/file3 >sub1/file3 &&\n> > > + echo sub2/file3 >sub2/file3 &&\n> > > + echo sub3/file3 >sub3/file3 &&\n> > > + echo file3 >file3 &&\n> > > + git add --all --sparse &&\n> > > + echo sub1/file3 >>sub1/file3 &&\n> > > + echo sub2/file3 >>sub2/file3 &&\n> > > + echo sub3/file3 >>sub3/file3 &&\n> > > + echo file3 >>file3\n> > > + )\n> > > +}\n> >\n> > It would be nice to modify different paths in the working tree and\n> > index, to see if we can handle cases where the sparse specification\n> > does not match the sparsity patterns better. (So, modify files inside\n> > and outside the sparsity patterns, stage those changes, and then do a\n> > `git sparse-checkout reapply` to make the files outside the sparsity\n> > patterns disappear from the working tree...then modify different files\n> > in the working tree both inside and outside the sparsity patterns.\n> > And also remove some file that matches the sparsity patterns and\n> > manually mark it as SKIP_WORKTREE.) That'd give us much better\n> > coverage.\n> >\n>\n> Nice addition. So I should use git update-index --skip-worktree to set\n> skip_worktree bit for index entries.\n\nWell, I'd say use normal \"sparse-checkout\" commands to set most of\nthem. However, adding one or two extra SKIP_WORKTREE paths via\nrunning `git update-index --skip-worktree $PATH` (and removing the\ncorresponding local file) would make the testcases more interesting.\n\n> > I think I'm going to stop reviewing here. I'm probably just going to\n> > keep repeating the same issues I identified earlier if I continue.\n>\n> Thank you very much for your review, you have pointed out very many\n> problems with this patch :)\n\nI'm glad it was helpful. :)\n"},{"id":"467231","messageId":"CAOLTT8R1K7L5p3+FkUAKMOA5+KcQU3YM8Pq2Oe4u24zi+t9Nrg@mail.gmail.com","threadId":"58721","inReplyTo":"CABPp-BErUhzm5=c6Tn4nPFKx-Kx0tYwf-J=G+nOFJ2bu-r+Dhg@mail.gmail.com","subject":"Re: [PATCH] [RFC] diff: introduce scope option","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2022-11-14T09:08:21Z","receivedAt":"2022-11-14T09:09:19Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Elijah Newren 于2022年11月6日周日 14:58写道：\n>\n> Hi,\n>\n> On Sat, Nov 5, 2022 at 7:11 PM ZheNing Hu wrote:\n> > Elijah Newren 于2022年11月1日周二 13:18写道：\n> > > On Sun, Oct 30, 2022 at 9:11 PM ZheNing Hu via GitGitGadget\n> > > wrote:\n> > > >\n> > > > From: ZheNing Hu \n> > > >\n> > > > When we use sparse-checkout, we often want the set of files\n> > > > that some commands operate on to be restricted to the\n> > > > sparse-checkout specification.\n> > >\n> > > It seems a bit premature to send this, when the guideline document[*]\n> > > detailing how these options should work is still in the \"Needs Review\"\n> > > state. I know, it's been waiting for a while, but it's a _long_\n> > > document.\n> > >\n> > > [*] https://lore.kernel.org/git/pull.1367.v3.git.1665269538608.gitgitgadget@gmail.com/\n> > >\n> >\n> > Fine, I just want to start trying to experiment with this feature in\n> > git-diff earlier,\n> > and I can wait for the sparse-checkout.txt documentation to finish\n> > first if needed :)\n>\n> Note that you may be able to help reduce the wait by reviewing the\n> document. (You commented on v1 -- thanks! -- but no one has commented\n> on any of the newer versions, despite being out for over a month and\n> still being marked as \"Needs Review\" in the \"What's cooking\" reports.)\n>\n\nI've skimmed v3 in general, but haven't looked at v4 yet, I'll take a closer\nlook at it.\n\n> > > > `--no-scope` is the default\n> > > > option for now.\n> > >\n> > > What does --no-scope even mean? You didn't explain it, and I don't\n> > > see how it could make sense. We explicitly avoided a `--no-` prefix\n> > > by allowing the --scope option to take a value. I don't think there\n> > > should be a --no-scope option.\n> >\n> > I think the “--no-scope” here does nothing, as if it were unaffected by scope\n> > (just like correctly \"--scope=all\", right?)\n>\n> Reading your patch, I was unable to determine where you made\n> --no-scope behave differently than how you implemented --scope=all.\n> Maybe there was a difference and I missed it.\n>\n> Anyway, I don't think there should be a --no-scope option.\n>\n\nFine, I'll get rid of it.\n\n> > > > diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\n> > > > index 3674ac48e92..04bf83e8be1 100644\n> > > > --- a/Documentation/diff-options.txt\n> > > > +++ b/Documentation/diff-options.txt\n> > > > @@ -195,6 +195,24 @@ For instance, if you configured the `diff.algorithm` variable to a\n> > > > non-default value and want to use the default one, then you\n> > > > have to use `--diff-algorithm=default` option.\n> > > >\n> > > > +ifndef::git-format-patch[]\n> > > > +ifndef::git-log[]\n> > > > +\n> > > > +--scope={sparse|all}::\n> > > > + Choose diff scope. The variants are as follows:\n> > > > ++\n> > > > +--\n> > > > +`--sparse`;;\n> > > > + Restrict diff paths to those matching sparse-checkout specification.\n> > > > +`--all`;;\n> > > > + Without restriction, diff is performed regardless of whether the path\n> > > > + meets the sparse-checkout specification.\n> > > > +--\n> > > > ++\n> > > > +\n> > > > +endif::git-log[]\n> > > > +endif::git-format-patch[]\n> > >\n> > > What about diff-files, diff-index, diff-tree, and show?\n> > >\n> >\n> > diff-options.txt included in their documents, and git-format-patch.txt,\n> > git-log.txt, but should not in git-format-patch.txt. I don't know if it\n> > should be included in git-diff-files.txt, because git diff-files compare\n> > the files in the working tree and the index (so it's the same as\n> > \"simple\" git-diff, which should not be affected by scope?)\n>\n> Oh, good point. Yeah, git-diff-files.txt should not get this option\n> as it won't have any affect. git-diff-index and git-diff-tree and\n> git-show and git-log should get it, though.\n>\n> > > > +int diff_path_in_sparse_checkout(const char *path) {\n> > > > + if (core_sparse_checkout_cone)\n> > > > + return path_in_cone_mode_sparse_checkout(path, the_repository->index);\n> > > > + else\n> > > > + return path_in_sparse_checkout(path, the_repository->index);\n> > > > +}\n> > >\n> > > This says we are including the path if it matches the sparsity\n> > > patterns. Thus, we have to be careful when we use this function,\n> > > because the relevant paths are ones that match the sparsity\n> > > specification. The sparsity specification will always match the\n> > > sparsity patterns when diffing two commits, but when either the index\n> > > or the working tree is part of the diff, the sparsity specification\n> > > *might* be temporarily expanded.\n> >\n> > Yes, I may have to look at more code to better understand how and when the\n> > \"sparsity specification\" is extended. Any recommendations for places to read?\n>\n> Yes, please review the newer version of my sparse-checkout.txt\n> directions file; it covers this in more detail.\n>\n\nOk, thanks.\n\n> > > > @@ -439,6 +459,20 @@ static void do_oneway_diff(struct unpack_trees_options *o,\n> > >\n> > > do_oneway_diff is for cases where we are diffing against the index...\n> > >\n> > > > return; /* nothing to diff.. */\n> > > > }\n> > > >\n> > > > + if (revs->diffopt.scope == DIFF_SCOPE_SPARSE) {\n> > > > + if (idx && tree) {\n> > > > + if (!diff_paths_in_sparse_checkout(idx->name, tree->name))\n> > > > + return;\n> > > > + } else if (idx) {\n> > > > + if (!diff_path_in_sparse_checkout(idx->name))\n> > > > + return;\n> > > > + } else if (tree) {\n> > > > + if (!diff_path_in_sparse_checkout(tree->name))\n> > > > + return;\n> > > > + } else\n> > > > + return;\n> > > > + }\n> > >\n> > > ...and you again mistakenly only compare to the sparsity patterns\n> > > instead of the sparse specification.\n> > >\n> >\n> > So here we should use ce_skip_worktree(idx) to check if this index entry matches\n> > sparse specification.\n>\n> ce_skip_worktree(idx) only checks for expansions to the sparse\n> specification in the working tree. If dealing with index files, the\n> sparse specification is also expanded to handle all files in the index\n> that differ from HEAD.\n>\n\nI will read more code to understand this detail.\n\n> > > > + } else\n> > > > + return error(_(\"invalid --scope value: %s\"), optarg);\n> > > > + }\n> > >\n> > > As written with no follow-on else clause here, wouldn't this silently\n> > > accept \"--scope\" without an \"=\" argument without an error\n> > > and without properly initializing opt->scope?\n> > >\n> >\n> > Because opt will be initializing with default_diff_options in repo_diff_setup(),\n> > and opt->scope should respect config value first. So I don't think there's a\n> > mistake here.\n>\n> Okay, it's good that you've got the variables initialized somehow, but\n> that's only half the point here. The main point is that the user can\n> specify something that makes no sense and if they do, we should throw\n> an error informing them of their mistake. --scope should not be\n> passed without an argument (either \"all\" or \"sparse\", currently), but\n> this code allowed it.\n>\n\nAh, it makes sense, I will fix it.\n\n> > > > diff --git a/t/t4070-diff-sparse-checkout-scope.sh b/t/t4070-diff-sparse-checkout-scope.sh\n> > > > new file mode 100644\n> > >\n> > > This needs to be fixed.\n>\n> Since you didn't comment on this but usually do comment on thing, I'll\n> just re-ping it to make sure you don't miss the comment about the\n> incorrect file mode here.\n>\n\nYes, I've noticed the file permissions issue here, but there may be some other\nissues with this test file: it fails when running CI tests, presumably\nbecause of\nsome shell syntax incompatibility.\n\n> > > > +reset_repo () {\n> > > > + rm -rf repo &&\n> > > > + git clone --no-checkout temp repo\n> > >\n> > > Why --no-checkout rather than say --sparse?\n> > >\n> >\n> > This is because I accidentally associated it with a\n> > partial clone. I often use \"git clone -filter=blob:none -no-checkout\"\n> > to do a partial clone, then \"git sparse- checkout set \"\n> > to set sparse-checkout patterns, and \"git checkout\" to download\n> > the missing blobs and checkout to a branch. But in this\n> > test file, we only need sparse-checkout, so it's true that I should\n> > not do such strange no-checkout thing.\n>\n> Yeah, no need to involve partial clones here. (As an aside, though, I\n> think even with a partial clone that --sparse makes more sense than\n> --no-checkout.)\n>\n\nUsing --sparse may indeed be better than -no-checkout, as there is no\nneed to perform an additional checkout after using clone --no-checkout\nand sparse-checkout. But it uses sparse-checkout cone mode by default.\n\nA little curious, why can't we specify --no-cone mode when doing git clone\nand why can't we specify sparse-checkout patterns here? If such a feature\nis available, git clone and git sparse-checkout will be combined in one step.\n\n> > > > +change_worktree_and_index() {\n> > > > + (\n> > > > + cd repo &&\n> > > > + mkdir sub2 sub3 &&\n> > > > + echo sub1/file3 >sub1/file3 &&\n> > > > + echo sub2/file3 >sub2/file3 &&\n> > > > + echo sub3/file3 >sub3/file3 &&\n> > > > + echo file3 >file3 &&\n> > > > + git add --all --sparse &&\n> > > > + echo sub1/file3 >>sub1/file3 &&\n> > > > + echo sub2/file3 >>sub2/file3 &&\n> > > > + echo sub3/file3 >>sub3/file3 &&\n> > > > + echo file3 >>file3\n> > > > + )\n> > > > +}\n> > >\n> > > It would be nice to modify different paths in the working tree and\n> > > index, to see if we can handle cases where the sparse specification\n> > > does not match the sparsity patterns better. (So, modify files inside\n> > > and outside the sparsity patterns, stage those changes, and then do a\n> > > `git sparse-checkout reapply` to make the files outside the sparsity\n> > > patterns disappear from the working tree...then modify different files\n> > > in the working tree both inside and outside the sparsity patterns.\n> > > And also remove some file that matches the sparsity patterns and\n> > > manually mark it as SKIP_WORKTREE.) That'd give us much better\n> > > coverage.\n> > >\n> >\n> > Nice addition. So I should use git update-index --skip-worktree to set\n> > skip_worktree bit for index entries.\n>\n> Well, I'd say use normal \"sparse-checkout\" commands to set most of\n> them. However, adding one or two extra SKIP_WORKTREE paths via\n> running `git update-index --skip-worktree $PATH` (and removing the\n> corresponding local file) would make the testcases more interesting.\n>\n\nGet it.\n\n> > > I think I'm going to stop reviewing here. I'm probably just going to\n> > > keep repeating the same issues I identified earlier if I continue.\n> >\n> > Thank you very much for your review, you have pointed out very many\n> > problems with this patch :)\n>\n> I'm glad it was helpful. :)\n\nThanks.\n\nZheNing Hu\n"},{"id":"467960","messageId":"pull.1398.v2.git.1669344333627.gitgitgadget@gmail.com","threadId":"58721","inReplyTo":"pull.1398.git.1667189512579.gitgitgadget@gmail.com","subject":"[PATCH v2] [RFC] diff: introduce --scope option","fromName":"ZheNing Hu via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-25T02:45:33Z","receivedAt":"2022-11-25T02:45:41Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"From: ZheNing Hu \n\nMany of git commands, such as \"git grep\", \"git diff\", they\nwill search the \"full-tree\" scope of the entire git repository,\nwhich is reasonable under normal circumstances, but if the user\nuses sparse checkout in a git monorepo, it's very possible that\nhe just wants to use files within the sparse specification,\nperhaps because:\n\n* He wants to be able to focus on his subprojects, the output\nof other subprojects will only interfere with him.\n\n* He's using partial cloning at the same time, and he doesn't\nwant to be able to execute the above git commands download a\nlarge number of blobs which out of sparse specification, which\nis a waste of time and may cause the size of the git repository\nto gradually expand.\n\nSo we need a way to restrict git commands to the sparse\nspecification. Implementing \"diff --scope\" is the first step\nin this plan. We are looking for a suitable option to choose:\nrestrict the path scope of diff to the sparse specification\nor keep the full tree scope (default action now). \"--scope=sparse\",\n\"--scope=all\" are the parameters corresponding to these two\ncases.\n\nIt is worth noting that \"--scope\" option only works on diff\ncommands specify \"--cached\" or \"REVISION\", because normal\n\"git diff\" has retrict the scope of diff files to the sparse\nspecificaiton by default, while \"git diff --cached\" or\n\"git diff REVSION\" will compare to the commit history, and\n\"--scope\" options can works here to restrict or not.\n\nAdd \"--scope\" option to git \"diff-index\" and \"git diff-tree\"\ntoo, because they also meet the above: specify \"--cached\",\nor \"REVISION\". Meanwhile, \"git diff-no-index\", \"git diff-files\"\ndon't have this option.\n\nSigned-off-by: ZheNing Hu \n---\n [RFC] diff: introduce scope option\n \n In [1], we discovered that users working on different sparse-checkout\n specification may download unnecessary blobs from each other's\n specification in collaboration. In [2] Junio suggested that maybe we can\n restrict some git command's filespec in sparse-checkout specification to\n elegantly solve this problem above. In [3]: Newren and Derrick Stolee\n prefer to name the option --scope={sparse, all}.\n \n So this patch is attempt to do this thing on git diff:\n \n v1:\n \n 1. add --restrict option to git diff, which restrict diff filespec in\n sparse-checkout specification. [4]\n \n v2.\n \n 1. rename --restrict to --scope={sparse, all}, support --no-scope.\n 2. add config: diff.scope={sparse,all}.\n \n v3.\n \n 1. with the help of newren's review, fix the wrong --scope behavior,\n its previous meaning was misrepresented as sparse patterns, and now\n it is fixed to match sparse specification. [5]\n 2. remove wrong diff.scope config.\n 3. apply --scope to git diff, git diff-index, git diff-tree.\n \n Since I split --scope into a separate option, this option will not be\n directly inherited by git commands such as git log, git format-patch,\n etc. If necessary, we can add it to git log or other commands in a\n similar way later.\n \n Global scope config haven’t implement yet... Since we haven't decided on\n an appropriate name for scope config. [6]\n \n [1]:\n https://lore.kernel.org/git/CAOLTT8SHo66kGbvWr=+LQ9UVd1NHgqGGEYK2qq6==QgRCgLZqQ@mail.gmail.com/\n [2]: https://lore.kernel.org/git/xmqqzgeqw0sy.fsf@gitster.g/ [3]:\n https://lore.kernel.org/git/07a25d48-e364-0d9b-6ffa-41a5984eb5db@github.com/\n [4]:\n https://lore.kernel.org/git/pull.1368.git.1664036052741.gitgitgadget@gmail.com/\n [5]:\n https://lore.kernel.org/git/CAOLTT8TceM-NpV2_hUCZj2Dx=W30f_9SHW8CcRH-pw32BRd-oA@mail.gmail.com/\n [6]:\n https://lore.kernel.org/git/CABPp-BGHMsMxP6e7p0HAZA=ugk+GY3XW6_EaTN=HzaLQYAzQYA@mail.gmail.com/\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1398%2Fadlternative%2Fzh%2Fdiff-scope-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1398/adlternative/zh/diff-scope-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1398\n\nContributor requested no range-diff. You can review it using these commands:\n git fetch https://github.com/gitgitgadget/git 63bba4fd 471a0691\n git range-diff 63bba4fd..93ddbcbd c000d916..471a0691\n\n Documentation/diff-options.txt | 33 +++\n builtin/diff-index.c | 29 ++-\n builtin/diff-tree.c | 15 ++\n builtin/diff.c | 46 ++++-\n cache.h | 5 +\n diff-lib.c | 44 ++++\n diff.c | 2 +\n diff.h | 8 +\n dir.c | 20 ++\n dir.h | 4 +\n t/t4070-diff-sparse-checkout-scope.sh | 286 ++++++++++++++++++++++++++\n tree-diff.c | 11 +\n 12 files changed, 497 insertions(+), 6 deletions(-)\n create mode 100755 t/t4070-diff-sparse-checkout-scope.sh\n\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex 3674ac48e92..778b22ae982 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -195,6 +195,39 @@ For instance, if you configured the `diff.algorithm` variable to a\n non-default value and want to use the default one, then you\n have to use `--diff-algorithm=default` option.\n \n+ifdef::git-diff[]\n+ifdef::git-diff-index[]\n+ifdef::git-diff-tree[]\n+\n+--scope=[sparse|all]::\n+\tRestrict or not restrict diff path scope in sparse specification.\n+\tThe variants are as follows:\n+\n++\n+--\n+`sparse`;;\n+\tWhen using diff to compare commit history, restrict the\n+\tscope of file path comparisons to the sparse specification.\n+\tSee sparse specification in link:technical/sparse-checkout.html\n+\t[the sparse-checkout design document] for more information.\n+`all`;;\n+\tWhen using diff to compare commit history, the file comparison\n+\tscope is full-tree. This is consistent with the current default\n+\tbehavior.\n+--\n++\n+\n+Note that `--scope` option only take effect if diff command specify\n+`--cached` or `REVISION`.\n+\n+The behavior of this `--scope` option is experimental and may change\n+in the future. See link:technical/sparse-checkout.html [the sparse-checkout\n+design document] for more information.\n+\n+endif::git-diff-tree[]\n+endif::git-diff-index[]\n+endif::git-diff[]\n+\n --stat[=[,[,]]]::\n \tGenerate a diffstat. By default, as much space as necessary\n \twill be used for the filename part, and the rest for the graph\ndiff --git a/builtin/diff-index.c b/builtin/diff-index.c\nindex aea139b9d8f..9cf37af8990 100644\n--- a/builtin/diff-index.c\n+++ b/builtin/diff-index.c\n@@ -20,6 +20,14 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)\n \tunsigned int option = 0;\n \tint i;\n \tint result;\n+\tenum sparse_scope scope;\n+\n+\tstruct option sparse_scope_options[] = {\n+\t\tOPT_CALLBACK_F(0, \"scope\", &scope, N_(\"[sparse|all]\"),\n+\t\t\t\tN_(\"restrict path scope in sparse specification\"),\n+\t\t\t\tPARSE_OPT_NONEG, diff_opt_sparse_scope),\n+\t\tOPT_END()\n+\t};\n \n \tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n \t\tusage(diff_cache_usage);\n@@ -36,6 +44,15 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)\n \tdiff_merges_suppress_m_parsing();\n \n \targc = setup_revisions(argc, argv, &rev, NULL);\n+\n+\targc = parse_options(argc, argv, prefix, sparse_scope_options, NULL,\n+\t\t\t PARSE_OPT_KEEP_DASHDASH |\n+\t\t\t PARSE_OPT_KEEP_UNKNOWN_OPT |\n+\t\t\t PARSE_OPT_KEEP_ARGV0 |\n+\t\t\t PARSE_OPT_NO_INTERNAL_HELP);\n+\n+\trev.diffopt.scope = scope;\n+\n \tfor (i = 1; i < argc; i++) {\n \t\tconst char *arg = argv[i];\n \n@@ -66,9 +83,15 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)\n \t\t\tperror(\"read_cache_preload\");\n \t\t\treturn -1;\n \t\t}\n-\t} else if (read_cache() < 0) {\n-\t\tperror(\"read_cache\");\n-\t\treturn -1;\n+\t} else {\n+\t\tif (read_cache() < 0) {\n+\t\t\tperror(\"read_cache\");\n+\t\t\treturn -1;\n+\t\t}\n+\t\tif (rev.diffopt.scope == SPARSE_SCOPE_SPARSE &&\n+\t\t strcmp(rev.pending.objects[0].name, \"HEAD\"))\n+\t\t\tdiff_collect_changes_index(&rev.diffopt.pathspec,\n+\t\t\t\t\t\t &rev.diffopt.change_index_files);\n \t}\n \tresult = run_diff_index(&rev, option);\n \tresult = diff_result_code(&rev.diffopt, result);\ndiff --git a/builtin/diff-tree.c b/builtin/diff-tree.c\nindex 85e8c81e594..5da13bdb5ca 100644\n--- a/builtin/diff-tree.c\n+++ b/builtin/diff-tree.c\n@@ -114,6 +114,14 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)\n \tstruct userformat_want w;\n \tint read_stdin = 0;\n \tint merge_base = 0;\n+\tenum sparse_scope scope;\n+\n+\tstruct option sparse_scope_options[] = {\n+\t\tOPT_CALLBACK_F(0, \"scope\", &scope, N_(\"[sparse|all]\"),\n+\t\t\t\tN_(\"restrict path scope in sparse specification\"),\n+\t\t\t\tPARSE_OPT_NONEG, diff_opt_sparse_scope),\n+\t\tOPT_END()\n+\t};\n \n \tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n \t\tusage(diff_tree_usage);\n@@ -131,6 +139,13 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)\n \tprefix = precompose_argv_prefix(argc, argv, prefix);\n \targc = setup_revisions(argc, argv, opt, &s_r_opt);\n \n+\targc = parse_options(argc, argv, prefix, sparse_scope_options, NULL,\n+\t\t\t PARSE_OPT_KEEP_DASHDASH |\n+\t\t\t PARSE_OPT_KEEP_UNKNOWN_OPT |\n+\t\t\t PARSE_OPT_KEEP_ARGV0 |\n+\t\t\t PARSE_OPT_NO_INTERNAL_HELP);\n+\topt->diffopt.scope = scope;\n+\n \tmemset(&w, 0, sizeof(w));\n \tuserformat_find_requirements(NULL, &w);\n \ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex 854d2c5a5c4..e30b3548c78 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -161,9 +161,15 @@ static int builtin_diff_index(struct rev_info *revs,\n \t\t\tperror(\"read_cache_preload\");\n \t\t\treturn -1;\n \t\t}\n-\t} else if (read_cache() < 0) {\n-\t\tperror(\"read_cache\");\n-\t\treturn -1;\n+\t} else {\n+\t\tif (read_cache() < 0) {\n+\t\t\tperror(\"read_cache\");\n+\t\t\treturn -1;\n+\t\t}\n+\t\tif (revs->diffopt.scope == SPARSE_SCOPE_SPARSE &&\n+\t\t strcmp(revs->pending.objects[0].name, \"HEAD\"))\n+\t\t\tdiff_collect_changes_index(&revs->diffopt.pathspec,\n+\t\t\t\t\t\t &revs->diffopt.change_index_files);\n \t}\n \treturn run_diff_index(revs, option);\n }\n@@ -388,6 +394,25 @@ static void symdiff_prepare(struct rev_info *rev, struct symdiff *sym)\n \tsym->skip = map;\n }\n \n+int diff_opt_sparse_scope(const struct option *option,\n+\t\t\t\tconst char *optarg, int unset)\n+{\n+\tenum sparse_scope *scope = option->value;\n+\n+\tBUG_ON_OPT_NEG_NOARG(unset, optarg);\n+\n+\tif (!core_apply_sparse_checkout)\n+\t\treturn error(_(\"this git repository don't \"\n+\t\t\t \"use sparse-checkout, --scope option cannot be used\"));\n+\tif (!strcmp(optarg, \"all\"))\n+\t\t*scope = SPARSE_SCOPE_ALL;\n+\telse if (!strcmp(optarg, \"sparse\"))\n+\t\t*scope = SPARSE_SCOPE_SPARSE;\n+\telse\n+\t\treturn error(_(\"invalid --scope value: %s\"), optarg);\n+\treturn 0;\n+}\n+\n int cmd_diff(int argc, const char **argv, const char *prefix)\n {\n \tint i;\n@@ -399,6 +424,14 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \tint nongit = 0, no_index = 0;\n \tint result = 0;\n \tstruct symdiff sdiff;\n+\tenum sparse_scope scope;\n+\n+\tstruct option sparse_scope_options[] = {\n+\t\tOPT_CALLBACK_F(0, \"scope\", &scope, N_(\"[sparse|all]\"),\n+\t\t\t\tN_(\"restrict path scope in sparse specification\"),\n+\t\t\t\tPARSE_OPT_NONEG, diff_opt_sparse_scope),\n+\t\tOPT_END()\n+\t};\n \n \t/*\n \t * We could get N tree-ish in the rev.pending_objects list.\n@@ -504,6 +537,13 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t\tdiff_setup_done(&rev.diffopt);\n \t}\n \n+\targc = parse_options(argc, argv, prefix, sparse_scope_options, NULL,\n+\t\t\t PARSE_OPT_KEEP_DASHDASH |\n+\t\t\t PARSE_OPT_KEEP_UNKNOWN_OPT |\n+\t\t\t PARSE_OPT_KEEP_ARGV0 |\n+\t\t\t PARSE_OPT_NO_INTERNAL_HELP);\n+\n+\trev.diffopt.scope = scope;\n \trev.diffopt.flags.recursive = 1;\n \trev.diffopt.rotate_to_strict = 1;\n \ndiff --git a/cache.h b/cache.h\nindex 26ed03bd6de..e3bb2f3dde0 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1082,6 +1082,11 @@ extern int core_apply_sparse_checkout;\n extern int core_sparse_checkout_cone;\n extern int sparse_expect_files_outside_of_patterns;\n \n+enum sparse_scope {\n+\tSPARSE_SCOPE_ALL = 0,\n+\tSPARSE_SCOPE_SPARSE,\n+};\n+\n /*\n * Returns the boolean value of $GIT_OPTIONAL_LOCKS (or the default value).\n */\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 2edea41a234..69770ca62a6 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -445,6 +445,13 @@ static void do_oneway_diff(struct unpack_trees_options *o,\n \n \tmatch_missing = revs->match_missing;\n \n+\tif (revs->diffopt.scope == SPARSE_SCOPE_SPARSE &&\n+\t ((o->index_only && revs->pending.objects[0].name &&\n+\t strcmp(revs->pending.objects[0].name, \"HEAD\") &&\n+\t !index_file_in_sparse_specification(idx ? idx : tree, &revs->diffopt.change_index_files)) ||\n+\t (!o->index_only && !worktree_file_in_sparse_specification(idx))))\n+\t\treturn;\n+\n \tif (cached && idx && ce_stage(idx)) {\n \t\tstruct diff_filepair *pair;\n \t\tpair = diff_unmerge(&revs->diffopt, idx->name);\n@@ -598,6 +605,43 @@ void diff_get_merge_base(const struct rev_info *revs, struct object_id *mb)\n \tfree_commit_list(merge_bases);\n }\n \n+static void diff_collect_updated_cb(struct diff_queue_struct *q,\n+\t\t\t\t\t struct diff_options *options,\n+\t\t\t\t\t void *data) {\n+\tint i;\n+\tstruct strset *change_index_files = (struct strset *)data;\n+\n+\tfor (i = 0; i < q->nr; i++) {\n+\t\tstruct diff_filepair *p = q->queue[i];\n+\n+\t\tstrset_add(change_index_files, p->two->path);\n+\t\tif (p->status == DIFF_STATUS_RENAMED)\n+\t\t\tstrset_add(change_index_files, p->one->path);\n+\t}\n+}\n+\n+void diff_collect_changes_index(struct pathspec *pathspec, struct strset *change_index_files)\n+{\n+\tstruct rev_info rev;\n+\tstruct setup_revision_opt opt;\n+\n+\trepo_init_revisions(the_repository, &rev, NULL);\n+\tmemset(&opt, 0, sizeof(opt));\n+\topt.def = \"HEAD\";\n+\tsetup_revisions(0, NULL, &rev, &opt);\n+\n+\trev.diffopt.ita_invisible_in_index = 1;\n+\trev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;\n+\trev.diffopt.format_callback = diff_collect_updated_cb;\n+\trev.diffopt.format_callback_data = change_index_files;\n+\trev.diffopt.flags.recursive = 1;\n+\n+\tcopy_pathspec(&rev.prune_data, pathspec);\n+\trun_diff_index(&rev, 1);\n+\trelease_revisions(&rev);\n+}\n+\n+\n int run_diff_index(struct rev_info *revs, unsigned int option)\n {\n \tstruct object_array_entry *ent;\ndiff --git a/diff.c b/diff.c\nindex 9f9a92ec9d2..cdbcacd7332 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4663,6 +4663,7 @@ void repo_diff_setup(struct repository *r, struct diff_options *options)\n \toptions->color_moved = diff_color_moved_default;\n \toptions->color_moved_ws_handling = diff_color_moved_ws_default;\n \n+\tstrset_init(&options->change_index_files);\n \tprep_parse_options(options);\n }\n \n@@ -6511,6 +6512,7 @@ void diff_free(struct diff_options *options)\n \tdiff_free_ignore_regex(options);\n \tclear_pathspec(&options->pathspec);\n \tFREE_AND_NULL(options->parseopts);\n+\tstrset_clear(&options->change_index_files);\n }\n \n void diff_flush(struct diff_options *options)\ndiff --git a/diff.h b/diff.h\nindex fd33caeb25d..4098a0cb123 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -8,6 +8,7 @@\n #include \"pathspec.h\"\n #include \"object.h\"\n #include \"oidset.h\"\n+#include \"strmap.h\"\n \n /**\n * The diff API is for programs that compare two sets of files (e.g. two trees,\n@@ -285,6 +286,9 @@ struct diff_options {\n \t/* diff-filter bits */\n \tunsigned int filter, filter_not;\n \n+\t/* diff sparse-checkout scope */\n+\tenum sparse_scope scope;\n+\n \tint use_color;\n \n \t/* Number of context lines to generate in patch output. */\n@@ -397,6 +401,7 @@ struct diff_options {\n \tstruct option *parseopts;\n \tstruct strmap *additional_path_headers;\n \n+\tstruct strset change_index_files;\n \tint no_free;\n };\n \n@@ -696,4 +701,7 @@ void print_stat_summary(FILE *fp, int files,\n \t\t\tint insertions, int deletions);\n void setup_diff_pager(struct diff_options *);\n \n+void diff_collect_changes_index(struct pathspec *pathspec, struct strset *files);\n+int diff_opt_sparse_scope(const struct option *option, const char *optarg, int unset);\n+\n #endif /* DIFF_H */\ndiff --git a/dir.c b/dir.c\nindex d604d1bab98..010e243f24a 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -1503,6 +1503,26 @@ int path_in_cone_mode_sparse_checkout(const char *path,\n \treturn path_in_sparse_checkout_1(path, istate, 1);\n }\n \n+int path_in_sparse_patterns(const char *path) {\n+\treturn path_in_sparse_checkout_1(path, the_repository->index, core_sparse_checkout_cone);\n+}\n+\n+/* Expand sparse-checkout specification (worktree) */\n+int worktree_file_in_sparse_specification(const struct cache_entry *worktree_check_ce)\n+{\n+\treturn worktree_check_ce && !ce_skip_worktree(worktree_check_ce);\n+}\n+\n+/* Expand sparse-checkout specification (index) */\n+int index_file_in_sparse_specification(const struct cache_entry *ce, struct strset *change_index_files)\n+{\n+\tif (!ce->ce_namelen)\n+\t\treturn 0;\n+\tif (change_index_files && strset_contains(change_index_files, ce->name))\n+\t\treturn 1;\n+\treturn path_in_sparse_patterns(ce->name);\n+}\n+\n static struct path_pattern *last_matching_pattern_from_lists(\n \t\tstruct dir_struct *dir, struct index_state *istate,\n \t\tconst char *pathname, int pathlen,\ndiff --git a/dir.h b/dir.h\nindex 674747d93af..254268bb9a0 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -4,6 +4,7 @@\n #include \"cache.h\"\n #include \"hashmap.h\"\n #include \"strbuf.h\"\n+#include \"strmap.h\"\n \n /**\n * The directory listing API is used to enumerate paths in the work tree,\n@@ -401,6 +402,9 @@ int path_in_sparse_checkout(const char *path,\n \t\t\t struct index_state *istate);\n int path_in_cone_mode_sparse_checkout(const char *path,\n \t\t\t\t struct index_state *istate);\n+int path_in_sparse_patterns(const char *path);\n+int index_file_in_sparse_specification(const struct cache_entry *ce, struct strset *change_index_files);\n+int worktree_file_in_sparse_specification(const struct cache_entry *worktree_check_ce);\n \n struct dir_entry *dir_add_ignored(struct dir_struct *dir,\n \t\t\t\t struct index_state *istate,\ndiff --git a/t/t4070-diff-sparse-checkout-scope.sh b/t/t4070-diff-sparse-checkout-scope.sh\nnew file mode 100755\nindex 00000000000..b8e3bfbaf5b\n--- /dev/null\n+++ b/t/t4070-diff-sparse-checkout-scope.sh\n@@ -0,0 +1,286 @@\n+#!/bin/sh\n+\n+test_description='diff sparse-checkout scope'\n+\n+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tmkdir in out1 out2 &&\n+\t\tfor i in $(test_seq 6)\n+\t\tdo\n+\t\t\techo \"in $i\" >in/\"$i\" &&\n+\t\t\techo \"out1 $i\" >out1/\"$i\" &&\n+\t\t\techo \"out2 $i\" >out2/\"$i\" || return 1\n+\t\tdone &&\n+\t\tgit add in out1 out2 &&\n+\t\tgit commit -m init &&\n+\t\tfor i in $(test_seq 6)\n+\t\tdo\n+\t\t\techo \"in $i\" >>in/\"$i\" &&\n+\t\t\techo \"out1 $i\" >>out1/\"$i\" || return 1\n+\t\tdone &&\n+\t\tgit add in out1 &&\n+\t\tgit commit -m change &&\n+\t\tgit sparse-checkout set \"in\"\n+\t)\n+'\n+\n+reset_sparse_checkout_state() {\n+\tgit -C repo reset --hard HEAD &&\n+\tgit -C repo sparse-checkout reapply\n+}\n+\n+reset_and_change_index() {\n+\treset_sparse_checkout_state &&\n+\t# add new ce\n+\toid=$(echo \"new thing\" | git -C repo hash-object --stdin -w) &&\n+\tgit -C repo update-index --add --cacheinfo 100644 $oid in/7 &&\n+\tgit -C repo update-index --add --cacheinfo 100644 $oid out1/7 &&\n+\t# rm ce\n+\tgit -C repo update-index --remove in/6 &&\n+\tgit -C repo update-index --remove out1/6 &&\n+\t# modify ce\n+\tgit -C repo update-index --cacheinfo 100644 $oid out1/5 &&\n+\t# mv ce1 -> ce2\n+\toid=$(git -C repo ls-files --format=\"%(objectname)\" in/4) &&\n+\tgit -C repo update-index --add --cacheinfo 100644 $oid in/8 &&\n+\tgit -C repo update-index --remove in/4 &&\n+\toid=$(git -C repo ls-files --format=\"%(objectname)\" out1/4) &&\n+\tgit -C repo update-index --add --cacheinfo 100644 $oid out1/8 &&\n+\tgit -C repo update-index --remove out1/4 &&\n+\t# chmod ce\n+\tgit -C repo update-index --chmod +x in/3 &&\n+\tgit -C repo update-index --chmod +x out1/3\n+}\n+\n+reset_and_change_worktree() {\n+\treset_sparse_checkout_state &&\n+\trm -rf repo/out1 repo/out2 &&\n+\tmkdir repo/out1 repo/out2 &&\n+\t# add new file\n+\techo \"in 7\" >repo/in/7 &&\n+\techo \"out1 7\" >repo/out1/7 &&\n+\tgit -C repo add --sparse in/7 out1/7 &&\n+\t# create out old file\n+\t>repo/out1/6 &&\n+\t# rm file\n+\trm repo/in/6 &&\n+\t# modify file\n+\techo \"out1 x\" >repo/out1/5 &&\n+\t# mv file1 -> file2\n+\tmv repo/in/4 repo/in/3 &&\n+\t# chmod file\n+\tchmod +x repo/in/2 &&\n+\t# add new file, mark skipworktree\n+\techo \"in 8\" >repo/in/8 &&\n+\techo \"out1 8\" >repo/out1/8 &&\n+\techo \"out2 8\" >repo/out2/8 &&\n+\tgit -C repo add --sparse in/8 out1/8 out2/8 &&\n+\tgit -C repo update-index --skip-worktree in/8 &&\n+\tgit -C repo update-index --skip-worktree out1/8 &&\n+\tgit -C repo update-index --skip-worktree out2/8 &&\n+\trm repo/in/8 repo/out1/8\n+}\n+\n+# git diff --cached REV\n+\n+test_expect_success 'git diff --cached --scope=all' '\n+\treset_and_change_index &&\n+\tcat >expected <<-EOF &&\n+M\tin/1\n+M\tin/2\n+M\tin/3\n+M\tin/4\n+M\tin/5\n+M\tin/6\n+A\tin/7\n+A\tin/8\n+M\tout1/1\n+M\tout1/2\n+M\tout1/3\n+M\tout1/5\n+D\tout1/6\n+A\tout1/7\n+R050\tout1/4\tout1/8\n+\tEOF\n+\tgit -C repo diff --name-status --cached --scope=all HEAD~ >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'git diff --cached --scope=sparse' '\n+\treset_and_change_index &&\n+\tcat >expected <<-EOF &&\n+M\tin/1\n+M\tin/2\n+M\tin/3\n+M\tin/4\n+M\tin/5\n+M\tin/6\n+A\tin/7\n+A\tin/8\n+M\tout1/3\n+M\tout1/5\n+D\tout1/6\n+A\tout1/7\n+R050\tout1/4\tout1/8\n+\tEOF\n+\tgit -C repo diff --name-status --cached --scope=sparse HEAD~ >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+# git diff REV\n+\n+test_expect_success 'git diff REVISION --scope=all' '\n+\treset_and_change_worktree &&\n+\tcat >expected <<-EOF &&\n+M\tin/1\n+M\tin/2\n+M\tin/3\n+D\tin/4\n+M\tin/5\n+D\tin/6\n+A\tin/7\n+M\tout1/1\n+M\tout1/2\n+M\tout1/3\n+M\tout1/4\n+M\tout1/5\n+M\tout1/6\n+A\tout1/7\n+A\tout2/8\n+\tEOF\n+\tgit -C repo diff --name-status --scope=all HEAD~ >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'git diff REVISION --scope=sparse' '\n+\treset_and_change_worktree &&\n+\tcat >expected <<-EOF &&\n+M\tin/1\n+M\tin/2\n+M\tin/3\n+D\tin/4\n+M\tin/5\n+D\tin/6\n+A\tin/7\n+M\tout1/5\n+M\tout1/6\n+A\tout1/7\n+A\tout2/8\n+\tEOF\n+\tgit -C repo diff --name-status --scope=sparse HEAD~ >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+# git diff REV1 REV2\n+\n+test_expect_success 'git diff two REVISION --scope=all' '\n+\treset_sparse_checkout_state &&\n+\tcat >expected <<-EOF &&\n+M\tin/1\n+M\tin/2\n+M\tin/3\n+M\tin/4\n+M\tin/5\n+M\tin/6\n+M\tout1/1\n+M\tout1/2\n+M\tout1/3\n+M\tout1/4\n+M\tout1/5\n+M\tout1/6\n+\tEOF\n+\tgit -C repo diff --name-status --scope=all HEAD~ HEAD >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'git diff two REVISION --scope=sparse' '\n+\treset_sparse_checkout_state &&\n+\tcat >expected <<-EOF &&\n+M\tin/1\n+M\tin/2\n+M\tin/3\n+M\tin/4\n+M\tin/5\n+M\tin/6\n+\tEOF\n+\tgit -C repo diff --name-status --scope=sparse HEAD~ HEAD >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+# git diff-index\n+\n+test_expect_success 'git diff-index --cached --scope=all' '\n+\treset_and_change_index &&\n+\tcat >expected <<-EOF &&\n+M\tin/1\n+M\tin/2\n+M\tin/3\n+M\tin/4\n+M\tin/5\n+M\tin/6\n+A\tin/7\n+A\tin/8\n+M\tout1/1\n+M\tout1/2\n+M\tout1/3\n+D\tout1/4\n+M\tout1/5\n+D\tout1/6\n+A\tout1/7\n+A\tout1/8\n+\tEOF\n+\tgit -C repo diff-index --name-status --cached --scope=all HEAD~ >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'git diff-index --cached --scope=sparse' '\n+\treset_and_change_index &&\n+\tcat >expected <<-EOF &&\n+M\tin/1\n+M\tin/2\n+M\tin/3\n+M\tin/4\n+M\tin/5\n+M\tin/6\n+A\tin/7\n+A\tin/8\n+M\tout1/3\n+D\tout1/4\n+M\tout1/5\n+D\tout1/6\n+A\tout1/7\n+A\tout1/8\n+\tEOF\n+\tgit -C repo diff-index --name-status --cached --scope=sparse HEAD~ >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+# git diff-tree\n+\n+test_expect_success 'git diff-tree --scope=all' '\n+\treset_sparse_checkout_state &&\n+\tcat >expected <<-EOF &&\n+M\tin\n+M\tout1\n+\tEOF\n+\tgit -C repo diff-tree --name-status --scope=all HEAD~ HEAD >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'git diff-tree --scope=sparse' '\n+\treset_sparse_checkout_state &&\n+\tcat >expected <<-EOF &&\n+M\tin\n+\tEOF\n+\tgit -C repo diff-tree --name-status --scope=sparse HEAD~ HEAD >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_done\ndiff --git a/tree-diff.c b/tree-diff.c\nindex 69031d7cbae..921665d0286 100644\n--- a/tree-diff.c\n+++ b/tree-diff.c\n@@ -5,6 +5,7 @@\n #include \"diff.h\"\n #include \"diffcore.h\"\n #include \"tree.h\"\n+#include \"dir.h\"\n \n /*\n * internal mode marker, saying a tree entry != entry of tp[imin]\n@@ -76,6 +77,16 @@ static int tree_entry_pathcmp(struct tree_desc *t1, struct tree_desc *t2)\n static int emit_diff_first_parent_only(struct diff_options *opt, struct combine_diff_path *p)\n {\n \tstruct combine_diff_parent *p0 = &p->parent[0];\n+\n+\tif (opt->scope == SPARSE_SCOPE_SPARSE) {\n+\t\tstruct strbuf sb = STRBUF_INIT;\n+\n+\t\tstrbuf_addstr(&sb, p->path);\n+\t\tif (S_ISDIR(p->mode) || S_ISDIR(p0->mode))\n+\t\t\tstrbuf_addch(&sb, '/');\n+\t\tif (!path_in_sparse_patterns(sb.buf))\n+\t\t\treturn 0;\n+\t}\n \tif (p->mode && p0->mode) {\n \t\topt->change(opt, p0->mode, p->mode, &p0->oid, &p->oid,\n \t\t\t1, 1, p->path, 0, 0);\n\nbase-commit: c000d916380bb59db69c78546928eadd076b9c7d\n-- \ngitgitgadget\n"},{"id":"468167","messageId":"pull.1398.v3.git.1669723221.gitgitgadget@gmail.com","threadId":"58721","inReplyTo":"pull.1398.v2.git.1669344333627.gitgitgadget@gmail.com","subject":"[PATCH v3 0/2] [RFC] diff: introduce scope option","fromName":"ZheNing Hu via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-29T12:00:19Z","receivedAt":"2022-11-29T12:01:31Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"In [1], we discovered that users working on different sparse-checkout\nspecification may download unnecessary blobs from each other's specification\nin collaboration. In [2] Junio suggested that maybe we can restrict some git\ncommand's filespec in sparse-checkout specification to elegantly solve this\nproblem above. In [3]: Newren and Derrick Stolee prefer to name the option\n--scope={sparse, all}.\n\nSo this patch is attempt to do this thing on git diff:\n\nv1:\n\n 1. add --restrict option to git diff, which restrict diff filespec in\n sparse-checkout specification. [4]\n\nv2.\n\n 1. rename --restrict to --scope={sparse, all}, support --no-scope.\n 2. add config: diff.scope={sparse,all}.\n\nv3.\n\n 1. with the help of newren's review, fix the wrong --scope behavior, its\n previous meaning was misrepresented as sparse patterns, and now it is\n fixed to match sparse specification. [5]\n 2. remove wrong diff.scope config.\n 3. apply --scope to git diff, git diff-index, git diff-tree.\n\nv4.\n\n 1. create a OPT_SPARSE_SCOPE macro for easier add --scope option to other\n git commands later.\n 2. introduce --scope option to \"git grep \".\n\nSince I split --scope into a separate option, this option will not be\ndirectly inherited by git commands such as git log, git format-patch, etc.\nIf necessary, we can add it to git log or other commands in a similar way\nlater.\n\nGlobal scope config haven’t implement yet... Since we haven't decided on an\nappropriate name for scope config. [6]\n\nNote why I don't add --scope to \"git grep --cached\", because we can't use \"\"\nwith \"--cached\", whether we use --scope=sparse or not , the resulting set of\nfiles it gets is the same.\n\n[1]:\nhttps://lore.kernel.org/git/CAOLTT8SHo66kGbvWr=+LQ9UVd1NHgqGGEYK2qq6==QgRCgLZqQ@mail.gmail.com/\n[2]: https://lore.kernel.org/git/xmqqzgeqw0sy.fsf@gitster.g/ [3]:\nhttps://lore.kernel.org/git/07a25d48-e364-0d9b-6ffa-41a5984eb5db@github.com/\n[4]:\nhttps://lore.kernel.org/git/pull.1368.git.1664036052741.gitgitgadget@gmail.com/\n[5]:\nhttps://lore.kernel.org/git/CAOLTT8TceM-NpV2_hUCZj2Dx=W30f_9SHW8CcRH-pw32BRd-oA@mail.gmail.com/\n[6]:\nhttps://lore.kernel.org/git/CABPp-BGHMsMxP6e7p0HAZA=ugk+GY3XW6_EaTN=HzaLQYAzQYA@mail.gmail.com/\n\nZheNing Hu (2):\n [RFC] diff: introduce --scope option\n [RPC] grep: introduce --scope option\n\n Documentation/diff-options.txt | 33 ++++\n Documentation/git-grep.txt | 24 +++\n builtin/diff-index.c | 24 ++-\n builtin/diff-tree.c | 11 ++\n builtin/diff.c | 23 ++-\n builtin/grep.c | 10 +\n cache.h | 5 +\n diff-lib.c | 43 +++++\n diff.c | 2 +\n diff.h | 7 +\n dir.c | 52 ++++++\n dir.h | 4 +\n grep.h | 2 +\n parse-options.h | 7 +\n t/t1090-sparse-checkout-scope.sh | 303 +++++++++++++++++++++++++++++++\n tree-diff.c | 7 +\n 16 files changed, 551 insertions(+), 6 deletions(-)\n\n\nbase-commit: 815c1e82021edbd99a2c423cf27f28863f28cef3\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1398%2Fadlternative%2Fzh%2Fdiff-scope-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1398/adlternative/zh/diff-scope-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/1398\n\nContributor requested no range-diff. You can review it using these commands:\n git fetch https://github.com/gitgitgadget/git c000d916 1d1b6618\n git range-diff c000d916..471a0691 083e0127..1d1b6618\n-- \ngitgitgadget\n"},{"id":"468168","messageId":"1d1b66188f5c221ecdc165e0dae82932a65a3fbb.1669723221.git.gitgitgadget@gmail.com","threadId":"58721","inReplyTo":"pull.1398.v3.git.1669723221.gitgitgadget@gmail.com","subject":"[PATCH v3 2/2] [RPC] grep: introduce --scope option","fromName":"ZheNing Hu via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-29T12:00:21Z","receivedAt":"2022-11-29T12:01:32Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"From: ZheNing Hu \n\nBecause we may want to restrict grep's file scope\nto sparse specification, Apply the --scope option\nwe implemented in diff to grep as well.\n\n`--scope=sparse` mean that the search file scope\nrestrict to sparse specification when we grep\nsomething in commit history, and `--scope=all`\nmean that the search file scope will be full-tree.\n\nNote that `--scope` option only oly takes effect\nwhen \"git grep \" is specified.\n\nSigned-off-by: ZheNing Hu \n---\n Documentation/git-grep.txt | 24 ++++++++++++++++++++++++\n builtin/grep.c | 10 ++++++++++\n grep.h | 2 ++\n t/t1090-sparse-checkout-scope.sh | 27 +++++++++++++++++++++++++++\n 4 files changed, 63 insertions(+)\n\ndiff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt\nindex dabdbe8471d..b556f657306 100644\n--- a/Documentation/git-grep.txt\n+++ b/Documentation/git-grep.txt\n@@ -28,6 +28,7 @@ SYNOPSIS\n \t [-f ] [-e] \n \t [--and|--or|--not|(|)|-e ...]\n \t [--recurse-submodules] [--parent-basename ]\n+\t [--scope=(sparse|all)]\n \t [ [--[no-]exclude-standard] [--cached | --no-index | --untracked] | ...]\n \t [--] [...]\n \n@@ -296,6 +297,29 @@ question doesn't support them.\n \tDo not output matched lines; instead, exit with status 0 when\n \tthere is a match and with non-zero status when there isn't.\n \n+--scope=(sparse|all)::\n+\tRestrict or not restrict grep path scope in sparse specification.\n+\tThe variants are as follows:\n+\n++\n+--\n+`sparse`;;\n+\tWhen grep in commit history, restrict the scope of file path\n+\tto the sparse specification. See sparse specification in\n+\tlink:technical/sparse-checkout.html [the sparse-checkout design\n+\tdocument] for more information.\n+`all`;;\n+\tWhen grep in commit history, the file path scope is full-tree.\n+\tThis is consistent with the current default behavior.\n+--\n++\n+\n+Note that `--scope` option only take effect if git command specify ``.\n+\n+The behavior of this `--scope` option is experimental and may change\n+in the future. See link:technical/sparse-checkout.html [the sparse-checkout\n+design document] for more information.\n+\n ...::\n \tInstead of searching tracked files in the working tree, search\n \tblobs in the given trees.\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex f7821c5fbba..8f7944fb924 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -640,6 +640,15 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,\n \t\t}\n \n \t\tstrbuf_add(base, entry.path, te_len);\n+\t\tif (opt->scope == SPARSE_SCOPE_SPARSE &&\n+\t\t\tbase->len != tn_len &&\n+\t\t\t!path_in_sparse_patterns(base->buf + tn_len,\n+\t\t\t\t\tS_ISDIR(entry.mode) ||\n+\t\t\t\t\tS_ISGITLINK(entry.mode))) {\n+\t\t\tstrbuf_setlen(base, old_baselen);\n+\t\t\tcontinue;\n+\t\t}\n+\n \n \t\tif (S_ISREG(entry.mode)) {\n \t\t\thit |= grep_oid(opt, &entry.oid, base->buf, tn_len,\n@@ -999,6 +1008,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\t\t PARSE_OPT_NOCOMPLETE),\n \t\tOPT_INTEGER('m', \"max-count\", &opt.max_count,\n \t\t\tN_(\"maximum number of results per file\")),\n+\t\tOPT_SPARSE_SCOPE(&opt.scope),\n \t\tOPT_END()\n \t};\n \tgrep_prefix = prefix;\ndiff --git a/grep.h b/grep.h\nindex 6075f997e68..05010d4b166 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -22,6 +22,7 @@ typedef int pcre2_general_context;\n #endif\n #include \"thread-utils.h\"\n #include \"userdiff.h\"\n+#include \"cache.h\"\n \n struct repository;\n \n@@ -175,6 +176,7 @@ struct grep_opt {\n \n \tvoid (*output)(struct grep_opt *opt, const void *data, size_t size);\n \tvoid *output_priv;\n+\tenum sparse_scope scope;\n };\n \n #define GREP_OPT_INIT { \\\ndiff --git a/t/t1090-sparse-checkout-scope.sh b/t/t1090-sparse-checkout-scope.sh\nindex e6ec8e8c1e4..c8f68fc7afa 100755\n--- a/t/t1090-sparse-checkout-scope.sh\n+++ b/t/t1090-sparse-checkout-scope.sh\n@@ -382,4 +382,31 @@ M\tin\n \ttest_cmp expected actual\n '\n \n+# git grep TREE\n+\n+test_expect_success 'git grep --scope=all' '\n+\treset_sparse_checkout_state &&\n+\tcat >expected <<-EOF &&\n+HEAD~:in/1\n+HEAD~:out1/1\n+HEAD~:out1/2\n+HEAD~:out1/3\n+HEAD~:out1/4\n+HEAD~:out1/5\n+HEAD~:out1/6\n+HEAD~:out2/1\n+\tEOF\n+\tgit -C repo grep --name-only --scope=all 1 HEAD~ >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'git grep --scope=sparse' '\n+\treset_sparse_checkout_state &&\n+\tcat >expected <<-EOF &&\n+HEAD~:in/1\n+\tEOF\n+\tgit -C repo grep --name-only --scope=sparse 1 HEAD~ >actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_done\n-- \ngitgitgadget\n"},{"id":"468169","messageId":"115d12819c35ee754e7401c5cac31408563c2416.1669723221.git.gitgitgadget@gmail.com","threadId":"58721","inReplyTo":"pull.1398.v3.git.1669723221.gitgitgadget@gmail.com","subject":"[PATCH v3 1/2] [RFC] diff: introduce --scope option","fromName":"ZheNing Hu via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-29T12:00:20Z","receivedAt":"2022-11-29T12:01:36Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"From: ZheNing Hu \n\nMany of git commands, such as \"git grep\", \"git diff\", they\nwill search the \"full-tree\" scope of the entire git repository,\nwhich is reasonable under normal circumstances, but if the user\nuses sparse checkout in a git monorepo, it's very possible that\nhe just wants to use files within the sparse specification,\nperhaps because:\n\n* He wants to be able to focus on his subprojects, the output\nof other subprojects will only interfere with him.\n\n* He's using partial cloning at the same time, and he doesn't\nwant to be able to execute the above git commands download a\nlarge number of blobs which out of sparse specification, which\nis a waste of time and may cause the size of the git repository\nto gradually expand.\n\nSo we need a way to restrict git commands to the sparse\nspecification. Implementing \"diff --scope\" is the first step\nin this plan. We are looking for a suitable option to choose:\nrestrict the path scope of diff to the sparse specification\nor keep the full tree scope (default action now). \"--scope=sparse\",\n\"--scope=all\" are the parameters corresponding to these two\ncases.\n\nIt is worth noting that \"--scope\" option only works on diff\ncommands specify \"--cached\" or \"REVISION\", because normal\n\"git diff\" has retrict the scope of diff files to the sparse\nspecificaiton by default, while \"git diff --cached\" or\n\"git diff REVSION\" will compare to the commit history, and\n\"--scope\" options can works here to restrict or not.\n\nAdd \"--scope\" option to git \"diff-index\" and \"git diff-tree\"\ntoo, because they also meet the above: specify \"--cached\",\nor \"REVISION\". Meanwhile, \"git diff-no-index\", \"git diff-files\"\ndon't have this option.\n\nSigned-off-by: ZheNing Hu \n---\n Documentation/diff-options.txt | 33 ++++\n builtin/diff-index.c | 24 ++-\n builtin/diff-tree.c | 11 ++\n builtin/diff.c | 23 ++-\n cache.h | 5 +\n diff-lib.c | 43 +++++\n diff.c | 2 +\n diff.h | 7 +\n dir.c | 52 ++++++\n dir.h | 4 +\n parse-options.h | 7 +\n t/t1090-sparse-checkout-scope.sh | 276 +++++++++++++++++++++++++++++++\n tree-diff.c | 7 +\n 13 files changed, 488 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex 3674ac48e92..778b22ae982 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -195,6 +195,39 @@ For instance, if you configured the `diff.algorithm` variable to a\n non-default value and want to use the default one, then you\n have to use `--diff-algorithm=default` option.\n \n+ifdef::git-diff[]\n+ifdef::git-diff-index[]\n+ifdef::git-diff-tree[]\n+\n+--scope=[sparse|all]::\n+\tRestrict or not restrict diff path scope in sparse specification.\n+\tThe variants are as follows:\n+\n++\n+--\n+`sparse`;;\n+\tWhen using diff to compare commit history, restrict the\n+\tscope of file path comparisons to the sparse specification.\n+\tSee sparse specification in link:technical/sparse-checkout.html\n+\t[the sparse-checkout design document] for more information.\n+`all`;;\n+\tWhen using diff to compare commit history, the file comparison\n+\tscope is full-tree. This is consistent with the current default\n+\tbehavior.\n+--\n++\n+\n+Note that `--scope` option only take effect if diff command specify\n+`--cached` or `REVISION`.\n+\n+The behavior of this `--scope` option is experimental and may change\n+in the future. See link:technical/sparse-checkout.html [the sparse-checkout\n+design document] for more information.\n+\n+endif::git-diff-tree[]\n+endif::git-diff-index[]\n+endif::git-diff[]\n+\n --stat[=[,[,]]]::\n \tGenerate a diffstat. By default, as much space as necessary\n \twill be used for the filename part, and the rest for the graph\ndiff --git a/builtin/diff-index.c b/builtin/diff-index.c\nindex 35dc9b23eef..27a510c30da 100644\n--- a/builtin/diff-index.c\n+++ b/builtin/diff-index.c\n@@ -20,6 +20,11 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)\n \tint i;\n \tint result;\n \n+\tstruct option sparse_scope_options[] = {\n+\t\tOPT_SPARSE_SCOPE(&rev.diffopt.scope),\n+\t\tOPT_END()\n+\t};\n+\n \tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n \t\tusage(diff_cache_usage);\n \n@@ -35,6 +40,13 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)\n \tdiff_merges_suppress_m_parsing();\n \n \targc = setup_revisions(argc, argv, &rev, NULL);\n+\n+\targc = parse_options(argc, argv, prefix, sparse_scope_options, NULL,\n+\t\t\t PARSE_OPT_KEEP_DASHDASH |\n+\t\t\t PARSE_OPT_KEEP_UNKNOWN_OPT |\n+\t\t\t PARSE_OPT_KEEP_ARGV0 |\n+\t\t\t PARSE_OPT_NO_INTERNAL_HELP);\n+\n \tfor (i = 1; i < argc; i++) {\n \t\tconst char *arg = argv[i];\n \n@@ -65,9 +77,15 @@ int cmd_diff_index(int argc, const char **argv, const char *prefix)\n \t\t\tperror(\"repo_read_index_preload\");\n \t\t\treturn -1;\n \t\t}\n-\t} else if (repo_read_index(the_repository) < 0) {\n-\t\tperror(\"repo_read_index\");\n-\t\treturn -1;\n+\t} else {\n+\t\tif (repo_read_index(the_repository) < 0) {\n+\t\t\tperror(\"read_cache\");\n+\t\t\treturn -1;\n+\t\t}\n+\t\tif (rev.diffopt.scope == SPARSE_SCOPE_SPARSE &&\n+\t\t strcmp(rev.pending.objects[0].name, \"HEAD\"))\n+\t\t\tdiff_collect_changes_index(&rev.diffopt.pathspec,\n+\t\t\t\t\t\t &rev.diffopt.change_index_files);\n \t}\n \tresult = run_diff_index(&rev, option);\n \tresult = diff_result_code(&rev.diffopt, result);\ndiff --git a/builtin/diff-tree.c b/builtin/diff-tree.c\nindex 25b853b85ca..31cac0c2614 100644\n--- a/builtin/diff-tree.c\n+++ b/builtin/diff-tree.c\n@@ -115,6 +115,11 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)\n \tint read_stdin = 0;\n \tint merge_base = 0;\n \n+\tstruct option sparse_scope_options[] = {\n+\t\tOPT_SPARSE_SCOPE(&opt->diffopt.scope),\n+\t\tOPT_END()\n+\t};\n+\n \tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n \t\tusage(diff_tree_usage);\n \n@@ -131,6 +136,12 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)\n \tprefix = precompose_argv_prefix(argc, argv, prefix);\n \targc = setup_revisions(argc, argv, opt, &s_r_opt);\n \n+\targc = parse_options(argc, argv, prefix, sparse_scope_options, NULL,\n+\t\t\t PARSE_OPT_KEEP_DASHDASH |\n+\t\t\t PARSE_OPT_KEEP_UNKNOWN_OPT |\n+\t\t\t PARSE_OPT_KEEP_ARGV0 |\n+\t\t\t PARSE_OPT_NO_INTERNAL_HELP);\n+\n \tmemset(&w, 0, sizeof(w));\n \tuserformat_find_requirements(NULL, &w);\n \ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex 163f2c6a874..8c2a847ec94 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -162,9 +162,15 @@ static int builtin_diff_index(struct rev_info *revs,\n \t\t\tperror(\"repo_read_index_preload\");\n \t\t\treturn -1;\n \t\t}\n-\t} else if (repo_read_index(the_repository) < 0) {\n-\t\tperror(\"repo_read_cache\");\n-\t\treturn -1;\n+\t} else {\n+\t\tif (repo_read_index(the_repository) < 0) {\n+\t\t\tperror(\"read_cache\");\n+\t\t\treturn -1;\n+\t\t}\n+\t\tif (revs->diffopt.scope == SPARSE_SCOPE_SPARSE &&\n+\t\t strcmp(revs->pending.objects[0].name, \"HEAD\"))\n+\t\t\tdiff_collect_changes_index(&revs->diffopt.pathspec,\n+\t\t\t\t\t\t &revs->diffopt.change_index_files);\n \t}\n \treturn run_diff_index(revs, option);\n }\n@@ -403,6 +409,11 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \tint result = 0;\n \tstruct symdiff sdiff;\n \n+\tstruct option sparse_scope_options[] = {\n+\t\tOPT_SPARSE_SCOPE(&rev.diffopt.scope),\n+\t\tOPT_END()\n+\t};\n+\n \t/*\n \t * We could get N tree-ish in the rev.pending_objects list.\n \t * Also there could be M blobs there, and P pathspecs. --cached may\n@@ -507,6 +518,12 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t\tdiff_setup_done(&rev.diffopt);\n \t}\n \n+\targc = parse_options(argc, argv, prefix, sparse_scope_options, NULL,\n+\t\t\t PARSE_OPT_KEEP_DASHDASH |\n+\t\t\t PARSE_OPT_KEEP_UNKNOWN_OPT |\n+\t\t\t PARSE_OPT_KEEP_ARGV0 |\n+\t\t\t PARSE_OPT_NO_INTERNAL_HELP);\n+\n \trev.diffopt.flags.recursive = 1;\n \trev.diffopt.rotate_to_strict = 1;\n \ndiff --git a/cache.h b/cache.h\nindex 07d40b0964b..2731656573e 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1058,6 +1058,11 @@ extern int core_apply_sparse_checkout;\n extern int core_sparse_checkout_cone;\n extern int sparse_expect_files_outside_of_patterns;\n \n+enum sparse_scope {\n+\tSPARSE_SCOPE_ALL = 0,\n+\tSPARSE_SCOPE_SPARSE,\n+};\n+\n /*\n * Returns the boolean value of $GIT_OPTIONAL_LOCKS (or the default value).\n */\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 2edea41a234..d660ecf0c62 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -445,6 +445,13 @@ static void do_oneway_diff(struct unpack_trees_options *o,\n \n \tmatch_missing = revs->match_missing;\n \n+\tif (revs->diffopt.scope == SPARSE_SCOPE_SPARSE &&\n+\t ((o->index_only && revs->pending.objects[0].name &&\n+\t strcmp(revs->pending.objects[0].name, \"HEAD\") &&\n+\t !index_file_in_sparse_specification(idx ? idx : tree, &revs->diffopt.change_index_files)) ||\n+\t (!o->index_only && !worktree_file_in_sparse_specification(idx))))\n+\t\treturn;\n+\n \tif (cached && idx && ce_stage(idx)) {\n \t\tstruct diff_filepair *pair;\n \t\tpair = diff_unmerge(&revs->diffopt, idx->name);\n@@ -598,6 +605,42 @@ void diff_get_merge_base(const struct rev_info *revs, struct object_id *mb)\n \tfree_commit_list(merge_bases);\n }\n \n+static void diff_collect_updated_cb(struct diff_queue_struct *q,\n+\t\t\t\t\t struct diff_options *options,\n+\t\t\t\t\t void *data) {\n+\tint i;\n+\tstruct strset *change_index_files = (struct strset *)data;\n+\n+\tfor (i = 0; i < q->nr; i++) {\n+\t\tstruct diff_filepair *p = q->queue[i];\n+\n+\t\tstrset_add(change_index_files, p->two->path);\n+\t\tif (p->status == DIFF_STATUS_RENAMED)\n+\t\t\tstrset_add(change_index_files, p->one->path);\n+\t}\n+}\n+\n+void diff_collect_changes_index(struct pathspec *pathspec, struct strset *change_index_files)\n+{\n+\tstruct rev_info rev;\n+\tstruct setup_revision_opt opt;\n+\n+\trepo_init_revisions(the_repository, &rev, NULL);\n+\tmemset(&opt, 0, sizeof(opt));\n+\topt.def = \"HEAD\";\n+\tsetup_revisions(0, NULL, &rev, &opt);\n+\n+\trev.diffopt.ita_invisible_in_index = 1;\n+\trev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;\n+\trev.diffopt.format_callback = diff_collect_updated_cb;\n+\trev.diffopt.format_callback_data = change_index_files;\n+\trev.diffopt.flags.recursive = 1;\n+\n+\tcopy_pathspec(&rev.prune_data, pathspec);\n+\trun_diff_index(&rev, 1);\n+\trelease_revisions(&rev);\n+}\n+\n int run_diff_index(struct rev_info *revs, unsigned int option)\n {\n \tstruct object_array_entry *ent;\ndiff --git a/diff.c b/diff.c\nindex 1054a4b7329..c719e9779a9 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4663,6 +4663,7 @@ void repo_diff_setup(struct repository *r, struct diff_options *options)\n \toptions->color_moved = diff_color_moved_default;\n \toptions->color_moved_ws_handling = diff_color_moved_ws_default;\n \n+\tstrset_init(&options->change_index_files);\n \tprep_parse_options(options);\n }\n \n@@ -6514,6 +6515,7 @@ void diff_free(struct diff_options *options)\n \tdiff_free_ignore_regex(options);\n \tclear_pathspec(&options->pathspec);\n \tFREE_AND_NULL(options->parseopts);\n+\tstrset_clear(&options->change_index_files);\n }\n \n void diff_flush(struct diff_options *options)\ndiff --git a/diff.h b/diff.h\nindex fd33caeb25d..31b255744db 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -8,6 +8,7 @@\n #include \"pathspec.h\"\n #include \"object.h\"\n #include \"oidset.h\"\n+#include \"strmap.h\"\n \n /**\n * The diff API is for programs that compare two sets of files (e.g. two trees,\n@@ -285,6 +286,9 @@ struct diff_options {\n \t/* diff-filter bits */\n \tunsigned int filter, filter_not;\n \n+\t/* diff sparse-checkout scope */\n+\tenum sparse_scope scope;\n+\n \tint use_color;\n \n \t/* Number of context lines to generate in patch output. */\n@@ -397,6 +401,7 @@ struct diff_options {\n \tstruct option *parseopts;\n \tstruct strmap *additional_path_headers;\n \n+\tstruct strset change_index_files;\n \tint no_free;\n };\n \n@@ -696,4 +701,6 @@ void print_stat_summary(FILE *fp, int files,\n \t\t\tint insertions, int deletions);\n void setup_diff_pager(struct diff_options *);\n \n+void diff_collect_changes_index(struct pathspec *pathspec, struct strset *files);\n+\n #endif /* DIFF_H */\ndiff --git a/dir.c b/dir.c\nindex d604d1bab98..5d47d5abf1a 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -18,6 +18,7 @@\n #include \"ewah/ewok.h\"\n #include \"fsmonitor.h\"\n #include \"submodule-config.h\"\n+#include \"parse-options.h\"\n \n /*\n * Tells read_directory_recursive how a file or directory should be treated.\n@@ -1503,6 +1504,57 @@ int path_in_cone_mode_sparse_checkout(const char *path,\n \treturn path_in_sparse_checkout_1(path, istate, 1);\n }\n \n+int path_in_sparse_patterns(const char *path, int is_dir) {\n+\tstruct strbuf sb = STRBUF_INIT;\n+\n+\tstrbuf_addstr(&sb, path);\n+\tif (!sb.len)\n+\t\treturn 0;\n+\tif (is_dir && sb.buf[sb.len - 1] != '/')\n+\t\tstrbuf_addch(&sb, '/');\n+\tif (!path_in_sparse_checkout_1(sb.buf,\n+\t\t\t\t the_repository->index,\n+\t\t\t\t core_sparse_checkout_cone))\n+\t\treturn 0;\n+\tstrbuf_release(&sb);\n+\treturn 1;\n+}\n+\n+/* Expand sparse-checkout specification (worktree) */\n+int worktree_file_in_sparse_specification(const struct cache_entry *worktree_check_ce)\n+{\n+\treturn worktree_check_ce && !ce_skip_worktree(worktree_check_ce);\n+}\n+\n+/* Expand sparse-checkout specification (index) */\n+int index_file_in_sparse_specification(const struct cache_entry *ce, struct strset *change_index_files)\n+{\n+\tif (!ce->ce_namelen)\n+\t\treturn 0;\n+\tif (change_index_files && strset_contains(change_index_files, ce->name))\n+\t\treturn 1;\n+\treturn path_in_sparse_patterns(ce->name, 0);\n+}\n+\n+int opt_sparse_scope(const struct option *option,\n+\t\t\t\tconst char *optarg, int unset)\n+{\n+\tenum sparse_scope *scope = option->value;\n+\n+\tBUG_ON_OPT_NEG_NOARG(unset, optarg);\n+\n+\tif (!core_apply_sparse_checkout)\n+\t\treturn error(_(\"this git repository don't \"\n+\t\t\t \"use sparse-checkout, --scope option cannot be used\"));\n+\tif (!strcmp(optarg, \"all\"))\n+\t\t*scope = SPARSE_SCOPE_ALL;\n+\telse if (!strcmp(optarg, \"sparse\"))\n+\t\t*scope = SPARSE_SCOPE_SPARSE;\n+\telse\n+\t\treturn error(_(\"invalid --scope value: %s\"), optarg);\n+\treturn 0;\n+}\n+\n static struct path_pattern *last_matching_pattern_from_lists(\n \t\tstruct dir_struct *dir, struct index_state *istate,\n \t\tconst char *pathname, int pathlen,\ndiff --git a/dir.h b/dir.h\nindex 674747d93af..14c1bac6e20 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -4,6 +4,7 @@\n #include \"cache.h\"\n #include \"hashmap.h\"\n #include \"strbuf.h\"\n+#include \"strmap.h\"\n \n /**\n * The directory listing API is used to enumerate paths in the work tree,\n@@ -401,6 +402,9 @@ int path_in_sparse_checkout(const char *path,\n \t\t\t struct index_state *istate);\n int path_in_cone_mode_sparse_checkout(const char *path,\n \t\t\t\t struct index_state *istate);\n+int path_in_sparse_patterns(const char *path, int is_dir);\n+int index_file_in_sparse_specification(const struct cache_entry *ce, struct strset *change_index_files);\n+int worktree_file_in_sparse_specification(const struct cache_entry *worktree_check_ce);\n \n struct dir_entry *dir_add_ignored(struct dir_struct *dir,\n \t\t\t\t struct index_state *istate,\ndiff --git a/parse-options.h b/parse-options.h\nindex b6ef86e0d15..37ca2714f87 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -356,6 +356,13 @@ int parse_opt_passthru_argv(const struct option *, const char *, int);\n /* value is enum branch_track* */\n int parse_opt_tracking_mode(const struct option *, const char *, int);\n \n+int opt_sparse_scope(const struct option *option,\n+\t\t\t\tconst char *optarg, int unset);\n+\n+#define OPT_SPARSE_SCOPE(var) OPT_CALLBACK_F(0, \"scope\", (var), N_(\"[sparse|all]\"), \\\n+\t\t\t\tN_(\"restrict path scope in sparse specification\"), \\\n+\t\t\t\tPARSE_OPT_NONEG, opt_sparse_scope)\n+\n #define OPT__VERBOSE(var, h) OPT_COUNTUP('v', \"verbose\", (var), (h))\n #define OPT__QUIET(var, h) OPT_COUNTUP('q', \"quiet\", (var), (h))\n #define OPT__VERBOSITY(var) \\\ndiff --git a/t/t1090-sparse-checkout-scope.sh b/t/t1090-sparse-checkout-scope.sh\nindex 3a14218b245..e6ec8e8c1e4 100755\n--- a/t/t1090-sparse-checkout-scope.sh\n+++ b/t/t1090-sparse-checkout-scope.sh\n@@ -106,4 +106,280 @@ test_expect_success 'in partial clone, sparse checkout only fetches needed blobs\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'setup two' '\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tmkdir in out1 out2 &&\n+\t\tfor i in $(test_seq 6)\n+\t\tdo\n+\t\t\techo \"in $i\" >in/\"$i\" &&\n+\t\t\techo \"out1 $i\" >out1/\"$i\" &&\n+\t\t\techo \"out2 $i\" >out2/\"$i\" || return 1\n+\t\tdone &&\n+\t\tgit add in out1 out2 &&\n+\t\tgit commit -m init &&\n+\t\tfor i in $(test_seq 6)\n+\t\tdo\n+\t\t\techo \"in $i\" >>in/\"$i\" &&\n+\t\t\techo \"out1 $i\" >>out1/\"$i\" || return 1\n+\t\tdone &&\n+\t\tgit add in out1 &&\n+\t\tgit commit -m change &&\n+\t\tgit sparse-checkout set \"in\"\n+\t)\n+'\n+\n+reset_sparse_checkout_state() {\n+\tgit -C repo reset --hard HEAD &&\n+\tgit -C repo sparse-checkout reapply\n+}\n+\n+reset_and_change_index() {\n+\treset_sparse_checkout_state &&\n+\t# add new ce\n+\toid=$(echo \"new thing\" | git -C repo hash-object --stdin -w) &&\n+\tgit -C repo update-index --add --cacheinfo 100644 $oid in/7 &&\n+\tgit -C repo update-index --add --cacheinfo 100644 $oid out1/7 &&\n+\t# rm ce\n+\tgit -C repo update-index --remove in/6 &&\n+\tgit -C repo update-index --remove out1/6 &&\n+\t# modify ce\n+\tgit -C repo update-index --cacheinfo 100644 $oid out1/5 &&\n+\t# mv ce1 -> ce2\n+\toid=$(git -C repo ls-files --format=\"%(objectname)\" in/4) &&\n+\tgit -C repo update-index --add --cacheinfo 100644 $oid in/8 &&\n+\tgit -C repo update-index --remove in/4 &&\n+\toid=$(git -C repo ls-files --format=\"%(objectname)\" out1/4) &&\n+\tgit -C repo update-index --add --cacheinfo 100644 $oid out1/8 &&\n+\tgit -C repo update-index --remove out1/4 &&\n+\t# chmod ce\n+\tgit -C repo update-index --chmod +x in/3 &&\n+\tgit -C repo update-index --chmod +x out1/3\n+}\n+\n+reset_and_change_worktree() {\n+\treset_sparse_checkout_state &&\n+\trm -rf repo/out1 repo/out2 &&\n+\tmkdir repo/out1 repo/out2 &&\n+\t# add new file\n+\techo \"in 7\" >repo/in/7 &&\n+\techo \"out1 7\" >repo/out1/7 &&\n+\tgit -C repo add --sparse in/7 out1/7 &&\n+\t# create out old file\n+\t>repo/out1/6 &&\n+\t# rm file\n+\trm repo/in/6 &&\n+\t# modify file\n+\techo \"out1 x\" >repo/out1/5 &&\n+\t# mv file1 -> file2\n+\tmv repo/in/4 repo/in/3 &&\n+\t# chmod file\n+\tchmod +x repo/in/2 &&\n+\t# add new file, mark skipworktree\n+\techo \"in 8\" >repo/in/8 &&\n+\techo \"out1 8\" >repo/out1/8 &&\n+\techo \"out2 8\" >repo/out2/8 &&\n+\tgit -C repo add --sparse in/8 out1/8 out2/8 &&\n+\tgit -C repo update-index --skip-worktree in/8 &&\n+\tgit -C repo update-index --skip-worktree out1/8 &&\n+\tgit -C repo update-index --skip-worktree out2/8 &&\n+\trm repo/in/8 repo/out1/8\n+}\n+\n+# git diff --cached REV\n+\n+test_expect_success 'git diff --cached --scope=all' '\n+\treset_and_change_index &&\n+\tcat >expected <<-EOF &&\n+M\tin/1\n+M\tin/2\n+M\tin/3\n+M\tin/4\n+M\tin/5\n+M\tin/6\n+A\tin/7\n+A\tin/8\n+M\tout1/1\n+M\tout1/2\n+M\tout1/3\n+M\tout1/5\n+D\tout1/6\n+A\tout1/7\n+R050\tout1/4\tout1/8\n+\tEOF\n+\tgit -C repo diff --name-status --cached --scope=all HEAD~ >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'git diff --cached --scope=sparse' '\n+\treset_and_change_index &&\n+\tcat >expected <<-EOF &&\n+M\tin/1\n+M\tin/2\n+M\tin/3\n+M\tin/4\n+M\tin/5\n+M\tin/6\n+A\tin/7\n+A\tin/8\n+M\tout1/3\n+M\tout1/5\n+D\tout1/6\n+A\tout1/7\n+R050\tout1/4\tout1/8\n+\tEOF\n+\tgit -C repo diff --name-status --cached --scope=sparse HEAD~ >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+# git diff REV\n+\n+test_expect_success 'git diff REVISION --scope=all' '\n+\treset_and_change_worktree &&\n+\tcat >expected <<-EOF &&\n+M\tin/1\n+M\tin/2\n+M\tin/3\n+D\tin/4\n+M\tin/5\n+D\tin/6\n+A\tin/7\n+M\tout1/1\n+M\tout1/2\n+M\tout1/3\n+M\tout1/4\n+M\tout1/5\n+M\tout1/6\n+A\tout1/7\n+A\tout2/8\n+\tEOF\n+\tgit -C repo diff --name-status --scope=all HEAD~ >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'git diff REVISION --scope=sparse' '\n+\treset_and_change_worktree &&\n+\tcat >expected <<-EOF &&\n+M\tin/1\n+M\tin/2\n+M\tin/3\n+D\tin/4\n+M\tin/5\n+D\tin/6\n+A\tin/7\n+M\tout1/5\n+M\tout1/6\n+A\tout1/7\n+A\tout2/8\n+\tEOF\n+\tgit -C repo diff --name-status --scope=sparse HEAD~ >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+# git diff REV1 REV2\n+\n+test_expect_success 'git diff two REVISION --scope=all' '\n+\treset_sparse_checkout_state &&\n+\tcat >expected <<-EOF &&\n+M\tin/1\n+M\tin/2\n+M\tin/3\n+M\tin/4\n+M\tin/5\n+M\tin/6\n+M\tout1/1\n+M\tout1/2\n+M\tout1/3\n+M\tout1/4\n+M\tout1/5\n+M\tout1/6\n+\tEOF\n+\tgit -C repo diff --name-status --scope=all HEAD~ HEAD >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'git diff two REVISION --scope=sparse' '\n+\treset_sparse_checkout_state &&\n+\tcat >expected <<-EOF &&\n+M\tin/1\n+M\tin/2\n+M\tin/3\n+M\tin/4\n+M\tin/5\n+M\tin/6\n+\tEOF\n+\tgit -C repo diff --name-status --scope=sparse HEAD~ HEAD >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+# git diff-index\n+\n+test_expect_success 'git diff-index --cached --scope=all' '\n+\treset_and_change_index &&\n+\tcat >expected <<-EOF &&\n+M\tin/1\n+M\tin/2\n+M\tin/3\n+M\tin/4\n+M\tin/5\n+M\tin/6\n+A\tin/7\n+A\tin/8\n+M\tout1/1\n+M\tout1/2\n+M\tout1/3\n+D\tout1/4\n+M\tout1/5\n+D\tout1/6\n+A\tout1/7\n+A\tout1/8\n+\tEOF\n+\tgit -C repo diff-index --name-status --cached --scope=all HEAD~ >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'git diff-index --cached --scope=sparse' '\n+\treset_and_change_index &&\n+\tcat >expected <<-EOF &&\n+M\tin/1\n+M\tin/2\n+M\tin/3\n+M\tin/4\n+M\tin/5\n+M\tin/6\n+A\tin/7\n+A\tin/8\n+M\tout1/3\n+D\tout1/4\n+M\tout1/5\n+D\tout1/6\n+A\tout1/7\n+A\tout1/8\n+\tEOF\n+\tgit -C repo diff-index --name-status --cached --scope=sparse HEAD~ >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+# git diff-tree\n+\n+test_expect_success 'git diff-tree --scope=all' '\n+\treset_sparse_checkout_state &&\n+\tcat >expected <<-EOF &&\n+M\tin\n+M\tout1\n+\tEOF\n+\tgit -C repo diff-tree --name-status --scope=all HEAD~ HEAD >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'git diff-tree --scope=sparse' '\n+\treset_sparse_checkout_state &&\n+\tcat >expected <<-EOF &&\n+M\tin\n+\tEOF\n+\tgit -C repo diff-tree --name-status --scope=sparse HEAD~ HEAD >actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_done\ndiff --git a/tree-diff.c b/tree-diff.c\nindex 69031d7cbae..72ca0ab38bc 100644\n--- a/tree-diff.c\n+++ b/tree-diff.c\n@@ -5,6 +5,7 @@\n #include \"diff.h\"\n #include \"diffcore.h\"\n #include \"tree.h\"\n+#include \"dir.h\"\n \n /*\n * internal mode marker, saying a tree entry != entry of tp[imin]\n@@ -76,6 +77,12 @@ static int tree_entry_pathcmp(struct tree_desc *t1, struct tree_desc *t2)\n static int emit_diff_first_parent_only(struct diff_options *opt, struct combine_diff_path *p)\n {\n \tstruct combine_diff_parent *p0 = &p->parent[0];\n+\n+\tif (opt->scope == SPARSE_SCOPE_SPARSE &&\n+\t !path_in_sparse_patterns(p->path,\n+\t\t\t\t S_ISDIR(p->mode) ||\n+\t\t\t\t S_ISDIR(p0->mode)))\n+\t\treturn 0;\n \tif (p->mode && p0->mode) {\n \t\topt->change(opt, p0->mode, p->mode, &p0->oid, &p->oid,\n \t\t\t1, 1, p->path, 0, 0);\n-- \ngitgitgadget\n\n"}]}