{"thread":{"id":"58510","subject":"[PATCH] diff: introduce --restrict option","startedAt":"2022-09-24T16:14:27Z","lastAt":"2022-09-27T15:04:38Z","messageCount":3,"participants":["ZheNing Hu via GitGitGadget","Elijah Newren","ZheNing Hu"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"463572","messageId":"pull.1368.git.1664036052741.gitgitgadget@gmail.com","threadId":"58510","inReplyTo":null,"subject":"[PATCH] diff: introduce --restrict option","fromName":"ZheNing Hu via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-24T16:14:12Z","receivedAt":"2022-09-24T16:14:27Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"From: ZheNing Hu <adlternative@gmail.com>\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 cone(s).\n\nSo introduce the `--restrict` option to git diff, which can\nrestrict diff filespec to the sparse-checkout cone(s).\n\nSigned-off-by: ZheNing Hu <adlternative@gmail.com>\n---\n    diff: introduce --restrict option\n    \n    In [1], we discovered that users working on different sparse-checkout\n    cone(s) may download unnecessary blobs from each other's cone(s) in\n    collaboration. And in [2] Junio suggested that maybe we can restrict\n    some git command's filespec in sparse-checkout cone(s) to elegantly\n    solve this problem above.\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 cone(s).\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/\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1368%2Fadlternative%2Fzh%2Fdiff-restrict-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1368/adlternative/zh/diff-restrict-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1368\n\n Documentation/diff-options.txt           |  4 ++\n diff.c                                   | 57 ++++++++++++++++++++++++\n diff.h                                   |  1 +\n t/t1092-sparse-checkout-compatibility.sh | 30 +++++++++++++\n 4 files changed, 92 insertions(+)\n\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex 3674ac48e92..8ee5b6b4603 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -631,6 +631,10 @@ Also, these upper-case letters can be downcased to exclude.  E.g.\n Note that not all diffs can feature all types. For instance, copied and\n renamed entries cannot appear if detection for those types is disabled.\n \n+--restrict::\n+\tRestrict the diff filespec in the sparse-checkout cone(s).\n+\tSee linkgit:git-sparse-checkout[1] for more details.\n+\n -S<string>::\n \tLook for differences that change the number of occurrences of\n \tthe specified string (i.e. addition/deletion) in a file.\ndiff --git a/diff.c b/diff.c\nindex 648f6717a55..95e13607041 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4923,6 +4923,19 @@ static int diff_opt_diff_filter(const struct option *option,\n \treturn 0;\n }\n \n+static int diff_opt_diff_restrict(const struct option *option,\n+\t\t\t\tconst char *optarg, int unset)\n+{\n+\tstruct diff_options *opt = option->value;\n+\n+\tBUG_ON_OPT_NEG(unset);\n+\tCALLOC_ARRAY(opt->sparse_checkout_patterns, 1);\n+\n+\tif (get_sparse_checkout_patterns(opt->sparse_checkout_patterns) < 0)\n+\t\tFREE_AND_NULL(opt->sparse_checkout_patterns);\n+\treturn 0;\n+}\n+\n static void enable_patch_output(int *fmt)\n {\n \t*fmt &= ~DIFF_FORMAT_NO_OUTPUT;\n@@ -5660,6 +5673,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, \"restrict\", options, NULL,\n+\t\t\t       N_(\"restrict files in sparse-checkout patterns\"),\n+\t\t\t       PARSE_OPT_NONEG | PARSE_OPT_OPTARG, diff_opt_diff_restrict),\n \t\t{ OPTION_CALLBACK, 0, \"output\", options, N_(\"<file>\"),\n \t\t  N_(\"output to a specific file\"),\n \t\t  PARSE_OPT_NONEG, NULL, 0, diff_opt_output },\n@@ -6601,6 +6617,24 @@ free_queue:\n \t}\n }\n \n+\n+static int match_sparse_checkout_patterns_by_spec(const struct diff_options *options, struct diff_filespec *spec) {\n+\tint dtype = DT_REG;\n+\n+\tif (!spec)\n+\t\treturn 0;\n+\n+\treturn path_matches_pattern_list(spec->path, strlen(spec->path),\n+\t\t\t\t\t \"\", &dtype, options->sparse_checkout_patterns,\n+\t\t\t\t\t the_repository->index) > 0;\n+}\n+\n+static int match_sparse_checkout_patterns(const struct diff_options *options, const struct diff_filepair *p)\n+{\n+\treturn match_sparse_checkout_patterns_by_spec(options, p->one) ||\n+\t       match_sparse_checkout_patterns_by_spec(options, p->two);\n+}\n+\n static int match_filter(const struct diff_options *options, const struct diff_filepair *p)\n {\n \treturn (((p->status == DIFF_STATUS_MODIFIED) &&\n@@ -6612,6 +6646,28 @@ static int match_filter(const struct diff_options *options, const struct diff_fi\n \t\t filter_bit_tst(p->status, options)));\n }\n \n+static void diffcore_apply_restrict(struct diff_options *options)\n+{\n+\tint i;\n+\tstruct diff_queue_struct *q = &diff_queued_diff;\n+\tstruct diff_queue_struct outq;\n+\n+\tDIFF_QUEUE_CLEAR(&outq);\n+\n+\tif (!options->sparse_checkout_patterns)\n+\t\treturn;\n+\n+\tfor (i = 0; i < q->nr; i++) {\n+\t\tstruct diff_filepair *p = q->queue[i];\n+\t\tif (match_sparse_checkout_patterns(options, p))\n+\t\t\tdiff_q(&outq, p);\n+\t\telse\n+\t\t\tdiff_free_filepair(p);\n+\t}\n+\tfree(q->queue);\n+\t*q = outq;\n+}\n+\n static void diffcore_apply_filter(struct diff_options *options)\n {\n \tint i;\n@@ -6827,6 +6883,7 @@ void diffcore_std(struct diff_options *options)\n \t\t/* See try_to_follow_renames() in tree-diff.c */\n \t\tdiff_resolve_rename_copy();\n \tdiffcore_apply_filter(options);\n+\tdiffcore_apply_restrict(options);\n \n \tif (diff_queued_diff.nr && !options->flags.diff_from_contents)\n \t\toptions->flags.has_changes = 1;\ndiff --git a/diff.h b/diff.h\nindex 8ae18e5ab1e..f651216da13 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -396,6 +396,7 @@ struct diff_options {\n \tstruct repository *repo;\n \tstruct option *parseopts;\n \tstruct strmap *additional_path_headers;\n+\tstruct pattern_list *sparse_checkout_patterns;\n \n \tint no_free;\n };\ndiff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\nindex b9350c075c2..4c416ef8e82 100755\n--- a/t/t1092-sparse-checkout-compatibility.sh\n+++ b/t/t1092-sparse-checkout-compatibility.sh\n@@ -504,6 +504,36 @@ test_expect_success 'diff --cached' '\n \ttest_all_match git diff --cached\n '\n \n+test_expect_success 'diff --restrict' '\n+\tinit_repos &&\n+\n+\ttest_all_match mkdir modules &&\n+\ttest_all_match touch modules/a &&\n+\ttest_all_match touch deep/b &&\n+\ttest_all_match git rm deep/a &&\n+\ttest_all_match git add --sparse modules deep &&\n+\trun_on_all git diff --restrict --staged --stat &&\n+\tcat >expect <<-EOF &&\n+\t deep/a | 1 -\n+\t deep/b | 0\n+\t 2 files changed, 1 deletion(-)\n+\tEOF\n+\ttest_cmp expect sparse-checkout-out &&\n+\tcat >expect <<-EOF &&\n+\t deep/a | 1 -\n+\t deep/b | 0\n+\t 2 files changed, 1 deletion(-)\n+\tEOF\n+\ttest_cmp expect sparse-index-out &&\n+\tcat >expect <<-EOF &&\n+\t deep/a    | 1 -\n+\t deep/b    | 0\n+\t modules/a | 0\n+\t 3 files changed, 1 deletion(-)\n+\tEOF\n+\ttest_cmp expect full-checkout-out\n+'\n+\n # NEEDSWORK: sparse-checkout behaves differently from full-checkout when\n # running this test with 'df-conflict-2' after 'df-conflict-1'.\n test_expect_success 'diff with renames and conflicts' '\n\nbase-commit: dda7228a83e2e9ff584bf6adbf55910565b41e14\n-- \ngitgitgadget\n"},{"id":"463584","messageId":"CABPp-BE_VUd=kjMb=T0WDvj=kn8fgN92-DtS3HdX+cZPjmhS_w@mail.gmail.com","threadId":"58510","inReplyTo":"pull.1368.git.1664036052741.gitgitgadget@gmail.com","subject":"Re: [PATCH] diff: introduce --restrict option","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-09-24T21:32:36Z","receivedAt":"2022-09-24T21:32:54Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi ZheNing,\n\nThanks for working on this!  I've long wanted these options for a few\ncommands, but just hadn't gotten back to it, in part because of\nseveral lingering questions we never resolved.\n\nI'm adding Matheus and Shaoxuan to cc, who both have had similar\npatches (for grep) derailed because we never agreed on option names\nand flags and defaults and whatnot.  (I'll post a big RFC I've been\nworking on this week later today so we can discuss that, but thought\nMatheus and Shaoxuan should see this patch too.)\n\nOn Sat, Sep 24, 2022 at 9:14 AM ZheNing Hu via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n>\n> From: ZheNing Hu <adlternative@gmail.com>\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 cone(s).\n\nThere are also non-cone users of sparse-checkout, so perhaps \"cone(s)\"\n=> \"specification\"?\n\n> So introduce the `--restrict` option to git diff, which can\n> restrict diff filespec to the sparse-checkout cone(s).\n\n\"can\"?  Is there times when it doesn't?\n\n\"diff filespec\" might be okay, but I'd personally prefer talking about\nthe \"diff output\".\n\nAnd again, there are `--no-cone` users of sparse checkouts so we need\nto avoid \"cone(s)\" here.\n\nPerhaps something like:\n\n    To allow this, introduce a `--restrict` option to git diff, which\nrestricts the diff output to those files matching the sparsity\nspecification.\n\n?\n\n> Signed-off-by: ZheNing Hu <adlternative@gmail.com>\n> ---\n>     diff: introduce --restrict option\n>\n>     In [1], we discovered that users working on different sparse-checkout\n>     cone(s) may download unnecessary blobs from each other's cone(s) in\n>     collaboration. And in [2] Junio suggested that maybe we can restrict\n>     some git command's filespec in sparse-checkout cone(s) to elegantly\n>     solve this problem above.\n\nBut this doesn't solve that problem, because there's no way to get\npull to notify merge to pass this extra option to diff...right?  I\nmean it's a step in that direction because you've added the new\ncapability, but you'd also need to add a config option which would\nturn this option on by default, and suggest to partial clone +\nsparse-checkout users that they set that config option.  Of course, if\nwe do that, we'd also need a `--no-restrict` option to git-diff to\noverride such a config option.\n\nAnd if we add a config option, we may want to start the output with a\nwarning about the output being restricted.\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 cone(s).\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/\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1368%2Fadlternative%2Fzh%2Fdiff-restrict-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1368/adlternative/zh/diff-restrict-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1368\n>\n>  Documentation/diff-options.txt           |  4 ++\n>  diff.c                                   | 57 ++++++++++++++++++++++++\n>  diff.h                                   |  1 +\n>  t/t1092-sparse-checkout-compatibility.sh | 30 +++++++++++++\n>  4 files changed, 92 insertions(+)\n>\n> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\n> index 3674ac48e92..8ee5b6b4603 100644\n> --- a/Documentation/diff-options.txt\n> +++ b/Documentation/diff-options.txt\n\nNote that diff-options.txt is included by not only git-diff.txt, but\nalso git-log.txt, git-show.txt, git-format-patch.txt, and a few\nothers.  It appears your changes are specific to git-diff, though.  I\nthink it'd be nice if they more generally applied to git-log.txt and\ngit-show.txt, but we'd have to be careful since they definitely should\nnot apply to git-format-patch.txt.\n\n> @@ -631,6 +631,10 @@ Also, these upper-case letters can be downcased to exclude.  E.g.\n>  Note that not all diffs can feature all types. For instance, copied and\n>  renamed entries cannot appear if detection for those types is disabled.\n>\n> +--restrict::\n> +       Restrict the diff filespec in the sparse-checkout cone(s).\n> +       See linkgit:git-sparse-checkout[1] for more details.\n> +\n\nIt would be nice to also provide a --no-restrict at the same time,\nespecially if we even think we might want to make --restrict the\ndefault in the future[1] or if we want to add config option now or\nsoon to treat --restrict as the default when that config option is\nset[2].\n\nWe need to get the name nailed down too.  I'm slightly leaning towards\nthe name you have used here, but we should note that we currently have\nother commands using --sparse (which confusingly maps to\n--no-restrict), some using --ignore-skip-worktree-bits (also as an\nequivalent for --no-restrict), we have had people propose patches\nusing --ignore-sparsity (again as an equivalent for --no-restrict),\nand we've seen a few other suggestions for names like\n--full-tree/--sparse-tree, --dense, and maybe others.\n\nThere's another question surrounding the option name too -- whether\nthese options should be global for git (like --work-tree), or command\nspecific.  That is a bit muddled by the fact that different\nsubcommands should have different defaults (checkout definitely should\ndefault to `--restrict` or else sparse checkouts would be essentially\nworthless, but stash and apply need to default to `--no-restrict` or\nwe'll drop some of the user's changes.  And it gets worse because we\nalso have two variants of not-quite-restrict-but-close for different\ncommand classes, and not by accident.).  It may also be affected by\nthe fact that we'd only want to control a subset of commands with a\nconfig option (namely, the querying commands (log, diff against\nhistory, grep against history, etc.) and perhaps the path-modifying\ncommands (add/rm/mv)), whereas the rest (like checkout or apply) we\nwould only want to allow control with an explicit command line flag\nadded by the user.\n\nI'm slightly leaning towards command-specific, using --[no-]restrict,\nand defaulting diff for now to --no-restrict...but we really should\nget buy-in on all of this.  I've been working on a big RFC patch\ncreating a Documentation/technical/sparse-checkout.txt file so we can\nhammer down these questions.\n\nSorry if that feels like it's derailing you.  It also derailed\nMatheus' changes to grep that he worked on for nearly a year from\n2020-2021[3], and derailed Shaoxuan's similar changes to grep just\nrecently[4], so I'm aware it's a big problem.  Hopefully the RFC will\nlet us resolve the questions and unblock these kinds of patches.\n\n[1] https://lore.kernel.org/git/xmqqh719pcoo.fsf@gitster.g/\n[2] https://lore.kernel.org/git/a86af661-cf58-a4e5-0214-a67d3a794d7e@github.com/\n[3] https://lore.kernel.org/git/5f3f7ac77039d41d1692ceae4b0c5df3bb45b74a.1612901326.git.matheus.bernardino@usp.br/\n-- see his comment section in particular\n[4] https://lore.kernel.org/git/20220923041842.27817-1-shaoxuan.yuan02@gmail.com/\n\n>  -S<string>::\n>         Look for differences that change the number of occurrences of\n>         the specified string (i.e. addition/deletion) in a file.\n> diff --git a/diff.c b/diff.c\n> index 648f6717a55..95e13607041 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -4923,6 +4923,19 @@ static int diff_opt_diff_filter(const struct option *option,\n>         return 0;\n>  }\n>\n> +static int diff_opt_diff_restrict(const struct option *option,\n> +                               const char *optarg, int unset)\n> +{\n> +       struct diff_options *opt = option->value;\n> +\n> +       BUG_ON_OPT_NEG(unset);\n> +       CALLOC_ARRAY(opt->sparse_checkout_patterns, 1);\n> +\n> +       if (get_sparse_checkout_patterns(opt->sparse_checkout_patterns) < 0)\n> +               FREE_AND_NULL(opt->sparse_checkout_patterns);\n> +       return 0;\n\nShould this be protected by \"if (core_apply_sparse_checkout)\"?\nDigging into get_sparse_checkout_patterns(), it appears it will throw\na warning if ${GIT_DIR}/info/sparse-checkout doesn't exist, which\nwould be annoying for non-sparse-checkout users.\n\n> +}\n> +\n>  static void enable_patch_output(int *fmt)\n>  {\n>         *fmt &= ~DIFF_FORMAT_NO_OUTPUT;\n> @@ -5660,6 +5673,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, \"restrict\", options, NULL,\n> +                              N_(\"restrict files in sparse-checkout patterns\"),\n\nI'd suggest at least replacing \"in\" with \"to\".  I'd also like to avoid\nthe word \"patterns\" since it hints at non-cone mode (in cone mode,\nusers specify directories, not patterns, even if patterns exist behind\nthe scenes).  Maybe we could change the phrase to something like\n\"restrict paths to those matching sparse-checkout specification\" would\nbe better?\n\n> +                              PARSE_OPT_NONEG | PARSE_OPT_OPTARG, diff_opt_diff_restrict),\n>                 { OPTION_CALLBACK, 0, \"output\", options, N_(\"<file>\"),\n>                   N_(\"output to a specific file\"),\n>                   PARSE_OPT_NONEG, NULL, 0, diff_opt_output },\n> @@ -6601,6 +6617,24 @@ free_queue:\n>         }\n>  }\n>\n> +\n> +static int match_sparse_checkout_patterns_by_spec(const struct diff_options *options, struct diff_filespec *spec) {\n> +       int dtype = DT_REG;\n> +\n> +       if (!spec)\n> +               return 0;\n> +\n> +       return path_matches_pattern_list(spec->path, strlen(spec->path),\n> +                                        \"\", &dtype, options->sparse_checkout_patterns,\n> +                                        the_repository->index) > 0;\n> +}\n> +\n> +static int match_sparse_checkout_patterns(const struct diff_options *options, const struct diff_filepair *p)\n> +{\n> +       return match_sparse_checkout_patterns_by_spec(options, p->one) ||\n> +              match_sparse_checkout_patterns_by_spec(options, p->two);\n\nMost of the time, p->one will match p->two.  Do we want to optimize\nthat case to not make both of these calls but just one?\n\n> +}\n> +\n>  static int match_filter(const struct diff_options *options, const struct diff_filepair *p)\n>  {\n>         return (((p->status == DIFF_STATUS_MODIFIED) &&\n> @@ -6612,6 +6646,28 @@ static int match_filter(const struct diff_options *options, const struct diff_fi\n>                  filter_bit_tst(p->status, options)));\n>  }\n>\n> +static void diffcore_apply_restrict(struct diff_options *options)\n> +{\n> +       int i;\n> +       struct diff_queue_struct *q = &diff_queued_diff;\n> +       struct diff_queue_struct outq;\n> +\n> +       DIFF_QUEUE_CLEAR(&outq);\n> +\n> +       if (!options->sparse_checkout_patterns)\n> +               return;\n> +\n> +       for (i = 0; i < q->nr; i++) {\n> +               struct diff_filepair *p = q->queue[i];\n> +               if (match_sparse_checkout_patterns(options, p))\n> +                       diff_q(&outq, p);\n> +               else\n> +                       diff_free_filepair(p);\n> +       }\n> +       free(q->queue);\n> +       *q = outq;\n> +}\n\nSo you're post-processing the results.  I think it'd be better to\navoid even walking into the trees outside the sparsity paths, much\nlike paths listed on the command line do.  That'd also be more\nreusable for log when we add a similar option for it.\n\n> +\n>  static void diffcore_apply_filter(struct diff_options *options)\n>  {\n>         int i;\n> @@ -6827,6 +6883,7 @@ void diffcore_std(struct diff_options *options)\n>                 /* See try_to_follow_renames() in tree-diff.c */\n>                 diff_resolve_rename_copy();\n>         diffcore_apply_filter(options);\n> +       diffcore_apply_restrict(options);\n>\n>         if (diff_queued_diff.nr && !options->flags.diff_from_contents)\n>                 options->flags.has_changes = 1;\n> diff --git a/diff.h b/diff.h\n> index 8ae18e5ab1e..f651216da13 100644\n> --- a/diff.h\n> +++ b/diff.h\n> @@ -396,6 +396,7 @@ struct diff_options {\n>         struct repository *repo;\n>         struct option *parseopts;\n>         struct strmap *additional_path_headers;\n> +       struct pattern_list *sparse_checkout_patterns;\n>\n>         int no_free;\n>  };\n> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\n> index b9350c075c2..4c416ef8e82 100755\n> --- a/t/t1092-sparse-checkout-compatibility.sh\n> +++ b/t/t1092-sparse-checkout-compatibility.sh\n\nThe description of this file is\n    test_description='compare full workdir to sparse workdir'\nwhich seems like a misfit for the tests you are adding.  I'd suggest\nplacing them somewhere else.  t1090 might be a good candidate.\n\n> @@ -504,6 +504,36 @@ test_expect_success 'diff --cached' '\n>         test_all_match git diff --cached\n>  '\n>\n> +test_expect_success 'diff --restrict' '\n> +       init_repos &&\n> +\n> +       test_all_match mkdir modules &&\n> +       test_all_match touch modules/a &&\n> +       test_all_match touch deep/b &&\n> +       test_all_match git rm deep/a &&\n> +       test_all_match git add --sparse modules deep &&\n> +       run_on_all git diff --restrict --staged --stat &&\n> +       cat >expect <<-EOF &&\n> +        deep/a | 1 -\n> +        deep/b | 0\n> +        2 files changed, 1 deletion(-)\n> +       EOF\n> +       test_cmp expect sparse-checkout-out &&\n> +       cat >expect <<-EOF &&\n> +        deep/a | 1 -\n> +        deep/b | 0\n> +        2 files changed, 1 deletion(-)\n> +       EOF\n> +       test_cmp expect sparse-index-out &&\n> +       cat >expect <<-EOF &&\n> +        deep/a    | 1 -\n> +        deep/b    | 0\n> +        modules/a | 0\n> +        3 files changed, 1 deletion(-)\n> +       EOF\n> +       test_cmp expect full-checkout-out\n> +'\n> +\n>  # NEEDSWORK: sparse-checkout behaves differently from full-checkout when\n>  # running this test with 'df-conflict-2' after 'df-conflict-1'.\n\nJust a guess, but is this NEEDSWORK perhaps reflecting the fact that\nthe test is in the wrong file?\n\nThe whole point of --restrict is to make the output you'd get within a\nsparse-checkout NOT match what you'd get in a full checkout, because\nyou are restricting the output to a subset.  If you want to compare to\na full-checkout and make sure the output matches, you'd want to use\n--no-restrict, right?\n"},{"id":"463729","messageId":"CAOLTT8RU1uGLOEZR5h4FyZc3jGAFQ5i62zhrXVqn7qZCMu2wxw@mail.gmail.com","threadId":"58510","inReplyTo":"CABPp-BE_VUd=kjMb=T0WDvj=kn8fgN92-DtS3HdX+cZPjmhS_w@mail.gmail.com","subject":"Re: [PATCH] diff: introduce --restrict option","fromName":"ZheNing Hu","fromEmail":"adlternative@gmail.com","sentAt":"2022-09-27T15:04:18Z","receivedAt":"2022-09-27T15:04:38Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Elijah Newren <newren@gmail.com> 于2022年9月25日周日 05:32写道：\n\n>\n> Hi ZheNing,\n>\n> Thanks for working on this!  I've long wanted these options for a few\n> commands, but just hadn't gotten back to it, in part because of\n> several lingering questions we never resolved.\n>\n> I'm adding Matheus and Shaoxuan to cc, who both have had similar\n> patches (for grep) derailed because we never agreed on option names\n> and flags and defaults and whatnot.  (I'll post a big RFC I've been\n> working on this week later today so we can discuss that, but thought\n> Matheus and Shaoxuan should see this patch too.)\n>\n\nI'd love to see some improvements of monorepo on git, So I'm happy to\nhelp (or helped by others) in this area. :-)\n\n> On Sat, Sep 24, 2022 at 9:14 AM ZheNing Hu via GitGitGadget\n> <gitgitgadget@gmail.com> wrote:\n> >\n> > From: ZheNing Hu <adlternative@gmail.com>\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 cone(s).\n>\n> There are also non-cone users of sparse-checkout, so perhaps \"cone(s)\"\n> => \"specification\"?\n>\n\nYeah, \"specification\", \"filterspec\"... They might be better words.\n\n> > So introduce the `--restrict` option to git diff, which can\n> > restrict diff filespec to the sparse-checkout cone(s).\n>\n> \"can\"?  Is there times when it doesn't?\n>\n> \"diff filespec\" might be okay, but I'd personally prefer talking about\n> the \"diff output\".\n>\n> And again, there are `--no-cone` users of sparse checkouts so we need\n> to avoid \"cone(s)\" here.\n>\n> Perhaps something like:\n>\n>     To allow this, introduce a `--restrict` option to git diff, which\n> restricts the diff output to those files matching the sparsity\n> specification.\n>\n> ?\n>\n\nYes, that's good.\n\n> > Signed-off-by: ZheNing Hu <adlternative@gmail.com>\n> > ---\n> >     diff: introduce --restrict option\n> >\n> >     In [1], we discovered that users working on different sparse-checkout\n> >     cone(s) may download unnecessary blobs from each other's cone(s) in\n> >     collaboration. And in [2] Junio suggested that maybe we can restrict\n> >     some git command's filespec in sparse-checkout cone(s) to elegantly\n> >     solve this problem above.\n>\n> But this doesn't solve that problem, because there's no way to get\n> pull to notify merge to pass this extra option to diff...right?  I\n> mean it's a step in that direction because you've added the new\n> capability, but you'd also need to add a config option which would\n> turn this option on by default, and suggest to partial clone +\n> sparse-checkout users that they set that config option.  Of course, if\n> we do that, we'd also need a `--no-restrict` option to git-diff to\n> override such a config option.\n>\n\nMake sense. The patch should add an RFC header, because I think\nthere are a lot of other git command that might need to use this diff --restrict\noption, e.g. git log... and want you mentioned is git pull.\n\nDo we make it work by git config \"diff.restrict=true|false\" , or do we\njust use this\ndiff option to other git commands, like git log --restrict (like git\nlog --diff-filter)?\n\n> And if we add a config option, we may want to start the output with a\n> warning about the output being restricted.\n>\n\nGood advice.\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 cone(s).\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/\n> >\n> > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1368%2Fadlternative%2Fzh%2Fdiff-restrict-v1\n> > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1368/adlternative/zh/diff-restrict-v1\n> > Pull-Request: https://github.com/gitgitgadget/git/pull/1368\n> >\n> >  Documentation/diff-options.txt           |  4 ++\n> >  diff.c                                   | 57 ++++++++++++++++++++++++\n> >  diff.h                                   |  1 +\n> >  t/t1092-sparse-checkout-compatibility.sh | 30 +++++++++++++\n> >  4 files changed, 92 insertions(+)\n> >\n> > diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\n> > index 3674ac48e92..8ee5b6b4603 100644\n> > --- a/Documentation/diff-options.txt\n> > +++ b/Documentation/diff-options.txt\n>\n> Note that diff-options.txt is included by not only git-diff.txt, but\n> also git-log.txt, git-show.txt, git-format-patch.txt, and a few\n> others.  It appears your changes are specific to git-diff, though.  I\n> think it'd be nice if they more generally applied to git-log.txt and\n> git-show.txt, but we'd have to be careful since they definitely should\n> not apply to git-format-patch.txt.\n>\n\nI'm a little confused here: git log/git show/git format-patch already\nnaturally inherits the --restrict option from git diff. If we don't really want\nthe --restrict option in the documentation of git format-patch, does that\nmean we should let git format-patch disable --restrict?\n\n> > @@ -631,6 +631,10 @@ Also, these upper-case letters can be downcased to exclude.  E.g.\n> >  Note that not all diffs can feature all types. For instance, copied and\n> >  renamed entries cannot appear if detection for those types is disabled.\n> >\n> > +--restrict::\n> > +       Restrict the diff filespec in the sparse-checkout cone(s).\n> > +       See linkgit:git-sparse-checkout[1] for more details.\n> > +\n>\n> It would be nice to also provide a --no-restrict at the same time,\n> especially if we even think we might want to make --restrict the\n> default in the future[1] or if we want to add config option now or\n> soon to treat --restrict as the default when that config option is\n> set[2].\n>\n> We need to get the name nailed down too.  I'm slightly leaning towards\n> the name you have used here, but we should note that we currently have\n> other commands using --sparse (which confusingly maps to\n> --no-restrict), some using --ignore-skip-worktree-bits (also as an\n> equivalent for --no-restrict), we have had people propose patches\n> using --ignore-sparsity (again as an equivalent for --no-restrict),\n> and we've seen a few other suggestions for names like\n> --full-tree/--sparse-tree, --dense, and maybe others.\n>\n\nYes, it does get a little cluttered. “sparse” was probably the most appropriate\nname, but some other command like “git rev-list --sparse” or\n\"git pack-objects --sparse\" has taken precedence and conveyed another\nlevel of meaning. So \"restrict\" may be an appropriate word in this case.\n\n> There's another question surrounding the option name too -- whether\n> these options should be global for git (like --work-tree), or command\n> specific.  That is a bit muddled by the fact that different\n> subcommands should have different defaults (checkout definitely should\n> default to `--restrict` or else sparse checkouts would be essentially\n> worthless, but stash and apply need to default to `--no-restrict` or\n> we'll drop some of the user's changes.  And it gets worse because we\n> also have two variants of not-quite-restrict-but-close for different\n> command classes, and not by accident.).  It may also be affected by\n> the fact that we'd only want to control a subset of commands with a\n> config option (namely, the querying commands (log, diff against\n> history, grep against history, etc.) and perhaps the path-modifying\n> commands (add/rm/mv)), whereas the rest (like checkout or apply) we\n> would only want to allow control with an explicit command line flag\n> added by the user.\n>\n\nWell, here you answered my question above, I also understood the points\nthat plagued us in moving forward on this feature:\n\n\"Which git commands should we use this -[no]-restrict on?\"\n\n> I'm slightly leaning towards command-specific, using --[no-]restrict,\n> and defaulting diff for now to --no-restrict...but we really should\n> get buy-in on all of this.  I've been working on a big RFC patch\n> creating a Documentation/technical/sparse-checkout.txt file so we can\n> hammer down these questions.\n>\n\ncommand-specific can make some common git command (i.e.\ngit format-patch, git stash) have consistent behavior, then use it if\nwe really need to work on sparse-checkout specification, e.g.\ngit log, git pull.\n\nI will check your RFC patch later.\n\n> Sorry if that feels like it's derailing you.  It also derailed\n> Matheus' changes to grep that he worked on for nearly a year from\n> 2020-2021[3], and derailed Shaoxuan's similar changes to grep just\n> recently[4], so I'm aware it's a big problem.  Hopefully the RFC will\n> let us resolve the questions and unblock these kinds of patches.\n>\n\nAh, the long patch set, I wonder how many collisions happened :)\n\n> [1] https://lore.kernel.org/git/xmqqh719pcoo.fsf@gitster.g/\n> [2] https://lore.kernel.org/git/a86af661-cf58-a4e5-0214-a67d3a794d7e@github.com/\n> [3] https://lore.kernel.org/git/5f3f7ac77039d41d1692ceae4b0c5df3bb45b74a.1612901326.git.matheus.bernardino@usp.br/\n> -- see his comment section in particular\n> [4] https://lore.kernel.org/git/20220923041842.27817-1-shaoxuan.yuan02@gmail.com/\n>\n> >  -S<string>::\n> >         Look for differences that change the number of occurrences of\n> >         the specified string (i.e. addition/deletion) in a file.\n> > diff --git a/diff.c b/diff.c\n> > index 648f6717a55..95e13607041 100644\n> > --- a/diff.c\n> > +++ b/diff.c\n> > @@ -4923,6 +4923,19 @@ static int diff_opt_diff_filter(const struct option *option,\n> >         return 0;\n> >  }\n> >\n> > +static int diff_opt_diff_restrict(const struct option *option,\n> > +                               const char *optarg, int unset)\n> > +{\n> > +       struct diff_options *opt = option->value;\n> > +\n> > +       BUG_ON_OPT_NEG(unset);\n> > +       CALLOC_ARRAY(opt->sparse_checkout_patterns, 1);\n> > +\n> > +       if (get_sparse_checkout_patterns(opt->sparse_checkout_patterns) < 0)\n> > +               FREE_AND_NULL(opt->sparse_checkout_patterns);\n> > +       return 0;\n>\n> Should this be protected by \"if (core_apply_sparse_checkout)\"?\n> Digging into get_sparse_checkout_patterns(), it appears it will throw\n> a warning if ${GIT_DIR}/info/sparse-checkout doesn't exist, which\n> would be annoying for non-sparse-checkout users.\n>\n\nAgree.\n\n> > +}\n> > +\n> >  static void enable_patch_output(int *fmt)\n> >  {\n> >         *fmt &= ~DIFF_FORMAT_NO_OUTPUT;\n> > @@ -5660,6 +5673,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, \"restrict\", options, NULL,\n> > +                              N_(\"restrict files in sparse-checkout patterns\"),\n>\n> I'd suggest at least replacing \"in\" with \"to\".  I'd also like to avoid\n> the word \"patterns\" since it hints at non-cone mode (in cone mode,\n> users specify directories, not patterns, even if patterns exist behind\n> the scenes).  Maybe we could change the phrase to something like\n> \"restrict paths to those matching sparse-checkout specification\" would\n> be better?\n>\n\nAgree.\n\n> > +                              PARSE_OPT_NONEG | PARSE_OPT_OPTARG, diff_opt_diff_restrict),\n> >                 { OPTION_CALLBACK, 0, \"output\", options, N_(\"<file>\"),\n> >                   N_(\"output to a specific file\"),\n> >                   PARSE_OPT_NONEG, NULL, 0, diff_opt_output },\n> > @@ -6601,6 +6617,24 @@ free_queue:\n> >         }\n> >  }\n> >\n> > +\n> > +static int match_sparse_checkout_patterns_by_spec(const struct diff_options *options, struct diff_filespec *spec) {\n> > +       int dtype = DT_REG;\n> > +\n> > +       if (!spec)\n> > +               return 0;\n> > +\n> > +       return path_matches_pattern_list(spec->path, strlen(spec->path),\n> > +                                        \"\", &dtype, options->sparse_checkout_patterns,\n> > +                                        the_repository->index) > 0;\n> > +}\n> > +\n> > +static int match_sparse_checkout_patterns(const struct diff_options *options, const struct diff_filepair *p)\n> > +{\n> > +       return match_sparse_checkout_patterns_by_spec(options, p->one) ||\n> > +              match_sparse_checkout_patterns_by_spec(options, p->two);\n>\n> Most of the time, p->one will match p->two.  Do we want to optimize\n> that case to not make both of these calls but just one?\n>\n\nYes, it is true that simplification can be done here.\n\n> > +}\n> > +\n> >  static int match_filter(const struct diff_options *options, const struct diff_filepair *p)\n> >  {\n> >         return (((p->status == DIFF_STATUS_MODIFIED) &&\n> > @@ -6612,6 +6646,28 @@ static int match_filter(const struct diff_options *options, const struct diff_fi\n> >                  filter_bit_tst(p->status, options)));\n> >  }\n> >\n> > +static void diffcore_apply_restrict(struct diff_options *options)\n> > +{\n> > +       int i;\n> > +       struct diff_queue_struct *q = &diff_queued_diff;\n> > +       struct diff_queue_struct outq;\n> > +\n> > +       DIFF_QUEUE_CLEAR(&outq);\n> > +\n> > +       if (!options->sparse_checkout_patterns)\n> > +               return;\n> > +\n> > +       for (i = 0; i < q->nr; i++) {\n> > +               struct diff_filepair *p = q->queue[i];\n> > +               if (match_sparse_checkout_patterns(options, p))\n> > +                       diff_q(&outq, p);\n> > +               else\n> > +                       diff_free_filepair(p);\n> > +       }\n> > +       free(q->queue);\n> > +       *q = outq;\n> > +}\n>\n> So you're post-processing the results.  I think it'd be better to\n> avoid even walking into the trees outside the sparsity paths, much\n> like paths listed on the command line do.  That'd also be more\n> reusable for log when we add a similar option for it.\n>\n\nYes, the temporary implementation was made by learning from the\n-diff-filter implementation. I may have to look at the code elsewhere\nif it needs to be filtered at the beginning of the traversal of the file.\n\n> > +\n> >  static void diffcore_apply_filter(struct diff_options *options)\n> >  {\n> >         int i;\n> > @@ -6827,6 +6883,7 @@ void diffcore_std(struct diff_options *options)\n> >                 /* See try_to_follow_renames() in tree-diff.c */\n> >                 diff_resolve_rename_copy();\n> >         diffcore_apply_filter(options);\n> > +       diffcore_apply_restrict(options);\n> >\n> >         if (diff_queued_diff.nr && !options->flags.diff_from_contents)\n> >                 options->flags.has_changes = 1;\n> > diff --git a/diff.h b/diff.h\n> > index 8ae18e5ab1e..f651216da13 100644\n> > --- a/diff.h\n> > +++ b/diff.h\n> > @@ -396,6 +396,7 @@ struct diff_options {\n> >         struct repository *repo;\n> >         struct option *parseopts;\n> >         struct strmap *additional_path_headers;\n> > +       struct pattern_list *sparse_checkout_patterns;\n> >\n> >         int no_free;\n> >  };\n> > diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh\n> > index b9350c075c2..4c416ef8e82 100755\n> > --- a/t/t1092-sparse-checkout-compatibility.sh\n> > +++ b/t/t1092-sparse-checkout-compatibility.sh\n>\n> The description of this file is\n>     test_description='compare full workdir to sparse workdir'\n> which seems like a misfit for the tests you are adding.  I'd suggest\n> placing them somewhere else.  t1090 might be a good candidate.\n>\n\nAgree.\n\n> > @@ -504,6 +504,36 @@ test_expect_success 'diff --cached' '\n> >         test_all_match git diff --cached\n> >  '\n> >\n> > +test_expect_success 'diff --restrict' '\n> > +       init_repos &&\n> > +\n> > +       test_all_match mkdir modules &&\n> > +       test_all_match touch modules/a &&\n> > +       test_all_match touch deep/b &&\n> > +       test_all_match git rm deep/a &&\n> > +       test_all_match git add --sparse modules deep &&\n> > +       run_on_all git diff --restrict --staged --stat &&\n> > +       cat >expect <<-EOF &&\n> > +        deep/a | 1 -\n> > +        deep/b | 0\n> > +        2 files changed, 1 deletion(-)\n> > +       EOF\n> > +       test_cmp expect sparse-checkout-out &&\n> > +       cat >expect <<-EOF &&\n> > +        deep/a | 1 -\n> > +        deep/b | 0\n> > +        2 files changed, 1 deletion(-)\n> > +       EOF\n> > +       test_cmp expect sparse-index-out &&\n> > +       cat >expect <<-EOF &&\n> > +        deep/a    | 1 -\n> > +        deep/b    | 0\n> > +        modules/a | 0\n> > +        3 files changed, 1 deletion(-)\n> > +       EOF\n> > +       test_cmp expect full-checkout-out\n> > +'\n> > +\n> >  # NEEDSWORK: sparse-checkout behaves differently from full-checkout when\n> >  # running this test with 'df-conflict-2' after 'df-conflict-1'.\n>\n> Just a guess, but is this NEEDSWORK perhaps reflecting the fact that\n> the test is in the wrong file?\n>\n> The whole point of --restrict is to make the output you'd get within a\n> sparse-checkout NOT match what you'd get in a full checkout, because\n> you are restricting the output to a subset.  If you want to compare to\n> a full-checkout and make sure the output matches, you'd want to use\n> --no-restrict, right?\n\nYes, a test compare between --restrict and --no-restrict will be better.\n\nThanks for review~~~\n--\nZheNing Hu\n"}]}