diff: introduce --restrict option

From: ZheNing Hu <[email protected]>

When we use sparse-checkout, we often want the set of files that some commands operate on to be restricted to the sparse-checkout cone(s).

So introduce the --restrict option to git diff, which can restrict diff filespec to the sparse-checkout cone(s).

Signed-off-by ZheNing Hu <[email protected]>

---
diff: introduce --restrict option

In [1], we discovered that users working on different sparse-checkout cone(s) may download unnecessary blobs from each other's cone(s) in collaboration. And in [2] Junio suggested that maybe we can restrict some git command's filespec in sparse-checkout cone(s) to elegantly solve this problem above.

Documentation/diff-options.txt +0 -0

 -S<string>::
        Look for differences that change the number of occurrences of
        the specified string (i.e. addition/deletion) in a file.

diff.c +1 -0

index 648f6717a55..95e13607041 100644
@@ -4923,6 +4923,19 @@ static int diff_opt_diff_filter(const struct option *option,
        return 0;
 }
 
+static int diff_opt_diff_restrict(const struct option *option,

diff.c +7 -0

+                              PARSE_OPT_NONEG | PARSE_OPT_OPTARG, diff_opt_diff_restrict),
                { OPTION_CALLBACK, 0, "output", options, N_("<file>"),
                  N_("output to a specific file"),
                  PARSE_OPT_NONEG, NULL, 0, diff_opt_output },

@@ -6601,6 +6617,24 @@ free_queue:
        }
 }
 
+
+static int match_sparse_checkout_patterns_by_spec(const struct diff_options *options, struct diff_filespec *spec) {
+       int dtype = DT_REG;
+
+       if (!spec)
+               return 0;

diff.c +13 -0

+}
+
 static int match_filter(const struct diff_options *options, const struct diff_filepair *p)
 {
        return (((p->status == DIFF_STATUS_MODIFIED) &&

@@ -6612,6 +6646,28 @@ static int match_filter(const struct diff_options *options, const struct diff_fi
                 filter_bit_tst(p->status, options)));
 }
 
+static void diffcore_apply_restrict(struct diff_options *options)
+{
+       int i;
+       struct diff_queue_struct *q = &diff_queued_diff;
+       struct diff_queue_struct outq;
+
+       DIFF_QUEUE_CLEAR(&outq);
+
+       if (!options->sparse_checkout_patterns)
+               return;
+

t/t1092-sparse-checkout-compatibility.sh +22 -0

@@ -504,6 +504,36 @@ test_expect_success 'diff --cached' '
        test_all_match git diff --cached
 '
 
+test_expect_success 'diff --restrict' '
+       init_repos &&
+
+       test_all_match mkdir modules &&
+       test_all_match touch modules/a &&
+       test_all_match touch deep/b &&
+       test_all_match git rm deep/a &&
+       test_all_match git add --sparse modules deep &&
+       run_on_all git diff --restrict --staged --stat &&
+       cat >expect <<-EOF &&
+        deep/a | 1 -
+        deep/b | 0
+        2 files changed, 1 deletion(-)
+       EOF
+       test_cmp expect sparse-checkout-out &&
+       cat >expect <<-EOF &&
+        deep/a | 1 -
+        deep/b | 0
+        2 files changed, 1 deletion(-)
+       EOF
+       test_cmp expect sparse-index-out &&
+       cat >expect <<-EOF &&

Hi ZheNing,

Thanks for working on this! I've long wanted these options for a few commands, but just hadn't gotten back to it, in part because of several lingering questions we never resolved.

I'm adding Matheus and Shaoxuan to cc, who both have had similar patches (for grep) derailed because we never agreed on option names and flags and defaults and whatnot. (I'll post a big RFC I've been working on this week later today so we can discuss that, but thought Matheus and Shaoxuan should see this patch too.)

Quoted 6 lines · ZheNing Hu via GitGitGadget

From: ZheNing Hu <[email protected]>

When we use sparse-checkout, we often want the set of files that some commands operate on to be restricted to the sparse-checkout cone(s).

There are also non-cone users of sparse-checkout, so perhaps "cone(s)" => "specification"?

Quoted 2 lines

So introduce the --restrict option to git diff, which can restrict diff filespec to the sparse-checkout cone(s).

"can"? Is there times when it doesn't?

"diff filespec" might be okay, but I'd personally prefer talking about the "diff output".

And again, there are --no-cone users of sparse checkouts so we need to avoid "cone(s)" here.

Perhaps something like:

To allow this, introduce a --restrict option to git diff, which
restricts the diff output to those files matching the sparsity specification.

?

Quoted 9 lines

Signed-off-by ZheNing Hu <[email protected]>

---
diff: introduce --restrict option

In [1], we discovered that users working on different sparse-checkout cone(s) may download unnecessary blobs from each other's cone(s) in collaboration. And in [2] Junio suggested that maybe we can restrict some git command's filespec in sparse-checkout cone(s) to elegantly solve this problem above.

But this doesn't solve that problem, because there's no way to get pull to notify merge to pass this extra option to diff...right? I mean it's a step in that direction because you've added the new capability, but you'd also need to add a config option which would turn this option on by default, and suggest to partial clone + sparse-checkout users that they set that config option. Of course, if we do that, we'd also need a --no-restrict option to git-diff to override such a config option.

And if we add a config option, we may want to start the output with a warning about the output being restricted.

Quoted 21 lines

    So this patch is attempt to do this thing on git diff:

    v1:

     1. add --restrict option to git diff, which restrict diff filespec in
        sparse-checkout cone(s).

    [1]:
    https://lore.kernel.org/git/CAOLTT8SHo66kGbvWr=+LQ9UVd1NHgqGGEYK2qq6==QgRCgLZqQ@mail.gmail.com/
    [2]: https://lore.kernel.org/git/[email protected]/

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1368%2Fadlternative%2Fzh%2Fdiff-restrict-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1368/adlternative/zh/diff-restrict-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1368

 Documentation/diff-options.txt           |  4 ++
 diff.c                                   | 57 ++++++++++++++++++++++++
 diff.h                                   |  1 +
 t/t1092-sparse-checkout-compatibility.sh | 30 +++++++++++++
 4 files changed, 92 insertions(+)

Documentation/diff-options.txt

Quoted 4 lines

Documentation/diff-options.txt +0 -0

index 3674ac48e92..8ee5b6b4603 100644

Note that diff-options.txt is included by not only git-diff.txt, but also git-log.txt, git-show.txt, git-format-patch.txt, and a few others. It appears your changes are specific to git-diff, though. I think it'd be nice if they more generally applied to git-log.txt and git-show.txt, but we'd have to be careful since they definitely should not apply to git-format-patch.txt.

Documentation/diff-options.txt Also, these upper-case letters can be downcased to exclude. E.g. · line 637

Quoted 8 lines

Documentation/diff-options.txt +4 -0

@@ -631,6 +631,10 @@ Also, these upper-case letters can be downcased to exclude.  E.g.
 Note that not all diffs can feature all types. For instance, copied and
 renamed entries cannot appear if detection for those types is disabled.
 
+--restrict::
+       Restrict the diff filespec in the sparse-checkout cone(s).
+       See linkgit:git-sparse-checkout[1] for more details.
+

It would be nice to also provide a --no-restrict at the same time, especially if we even think we might want to make --restrict the default in the future[1] or if we want to add config option now or soon to treat --restrict as the default when that config option is set[2].

We need to get the name nailed down too. I'm slightly leaning towards the name you have used here, but we should note that we currently have other commands using --sparse (which confusingly maps to --no-restrict), some using --ignore-skip-worktree-bits (also as an equivalent for --no-restrict), we have had people propose patches using --ignore-sparsity (again as an equivalent for --no-restrict), and we've seen a few other suggestions for names like --full-tree/--sparse-tree, --dense, and maybe others.

There's another question surrounding the option name too -- whether these options should be global for git (like --work-tree), or command specific. That is a bit muddled by the fact that different subcommands should have different defaults (checkout definitely should default to --restrict or else sparse checkouts would be essentially worthless, but stash and apply need to default to --no-restrict or we'll drop some of the user's changes. And it gets worse because we

also have two variants of not-quite-restrict-but-close for different command classes, and not by accident.). It may also be affected by the fact that we'd only want to control a subset of commands with a config option (namely, the querying commands (log, diff against history, grep against history, etc.) and perhaps the path-modifying commands (add/rm/mv)), whereas the rest (like checkout or apply) we would only want to allow control with an explicit command line flag added by the user.

I'm slightly leaning towards command-specific, using --[no-]restrict, and defaulting diff for now to --no-restrict...but we really should get buy-in on all of this. I've been working on a big RFC patch creating a Documentation/technical/sparse-checkout.txt file so we can hammer down these questions.

Sorry if that feels like it's derailing you. It also derailed Matheus' changes to grep that he worked on for nearly a year from 2020-2021[3], and derailed Shaoxuan's similar changes to grep just recently[4], so I'm aware it's a big problem. Hopefully the RFC will let us resolve the questions and unblock these kinds of patches.

[1] https://lore.kernel.org/git/[email protected]/
[2] https://lore.kernel.org/git/[email protected]/
[3] https://lore.kernel.org/git/5f3f7ac77039d41d1692ceae4b0c5df3bb45b74a.1612901326.git.matheus.bernardino@usp.br/
-- see his comment section in particular
[4] https://lore.kernel.org/git/[email protected]/

Quoted 11 lines

Documentation/diff-options.txt +0 -0

 -S<string>::
        Look for differences that change the number of occurrences of
        the specified string (i.e. addition/deletion) in a file.

diff.c +0 -0

index 648f6717a55..95e13607041 100644
@@ -4923,6 +4923,19 @@ static int diff_opt_diff_filter(const struct option *option,
        return 0;
 }
 

diff.c static int diff_opt_diff_filter(const struct option *option, · line 4936

Quoted 11 lines

diff.c +11 -0

+static int diff_opt_diff_restrict(const struct option *option,
+                               const char *optarg, int unset)
+{
+       struct diff_options *opt = option->value;
+
+       BUG_ON_OPT_NEG(unset);
+       CALLOC_ARRAY(opt->sparse_checkout_patterns, 1);
+
+       if (get_sparse_checkout_patterns(opt->sparse_checkout_patterns) < 0)
+               FREE_AND_NULL(opt->sparse_checkout_patterns);
+       return 0;

Should this be protected by "if (core_apply_sparse_checkout)"? Digging into get_sparse_checkout_patterns(), it appears it will throw a warning if ${GIT_DIR}/info/sparse-checkout doesn't exist, which would be annoying for non-sparse-checkout users.

Quoted 10 lines

diff.c +3 -0

+}
+
 static void enable_patch_output(int *fmt)
 {
        *fmt &= ~DIFF_FORMAT_NO_OUTPUT;

@@ -5660,6 +5673,9 @@ static void prep_parse_options(struct diff_options *options)
                OPT_CALLBACK_F(0, "diff-filter", options, N_("[(A|C|D|M|R|T|U|X|B)...[*]]"),
                               N_("select files by diff type"),
                               PARSE_OPT_NONEG, diff_opt_diff_filter),
+               OPT_CALLBACK_F(0, "restrict", options, NULL,

diff.c static void prep_parse_options(struct diff_options *options) · line 5677

Quoted 1 line

diff.c +1 -0

+                              N_("restrict files in sparse-checkout patterns"),

I'd suggest at least replacing "in" with "to". I'd also like to avoid the word "patterns" since it hints at non-cone mode (in cone mode, users specify directories, not patterns, even if patterns exist behind the scenes). Maybe we could change the phrase to something like "restrict paths to those matching sparse-checkout specification" would be better?

Quoted 19 lines

diff.c +12 -0

+                              PARSE_OPT_NONEG | PARSE_OPT_OPTARG, diff_opt_diff_restrict),
                { OPTION_CALLBACK, 0, "output", options, N_("<file>"),
                  N_("output to a specific file"),
                  PARSE_OPT_NONEG, NULL, 0, diff_opt_output },

@@ -6601,6 +6617,24 @@ free_queue:
        }
 }
 
+
+static int match_sparse_checkout_patterns_by_spec(const struct diff_options *options, struct diff_filespec *spec) {
+       int dtype = DT_REG;
+
+       if (!spec)
+               return 0;
+
+       return path_matches_pattern_list(spec->path, strlen(spec->path),
+                                        "", &dtype, options->sparse_checkout_patterns,
+                                        the_repository->index) > 0;
+}

diff.c free_queue: · line 6635

Quoted 5 lines

diff.c +5 -0

+
+static int match_sparse_checkout_patterns(const struct diff_options *options, const struct diff_filepair *p)
+{
+       return match_sparse_checkout_patterns_by_spec(options, p->one) ||
+              match_sparse_checkout_patterns_by_spec(options, p->two);

Most of the time, p->one will match p->two. Do we want to optimize that case to not make both of these calls but just one?

Quoted 27 lines

diff.c +20 -0

+}
+
 static int match_filter(const struct diff_options *options, const struct diff_filepair *p)
 {
        return (((p->status == DIFF_STATUS_MODIFIED) &&

@@ -6612,6 +6646,28 @@ static int match_filter(const struct diff_options *options, const struct diff_fi
                 filter_bit_tst(p->status, options)));
 }
 
+static void diffcore_apply_restrict(struct diff_options *options)
+{
+       int i;
+       struct diff_queue_struct *q = &diff_queued_diff;
+       struct diff_queue_struct outq;
+
+       DIFF_QUEUE_CLEAR(&outq);
+
+       if (!options->sparse_checkout_patterns)
+               return;
+
+       for (i = 0; i < q->nr; i++) {
+               struct diff_filepair *p = q->queue[i];
+               if (match_sparse_checkout_patterns(options, p))
+                       diff_q(&outq, p);
+               else
+                       diff_free_filepair(p);
+       }

diff.c static int match_filter(const struct diff_options *options, const struct diff_fi · line 6669

Quoted 3 lines

diff.c +3 -0

+       free(q->queue);
+       *q = outq;
+}

So you're post-processing the results. I think it'd be better to avoid even walking into the trees outside the sparsity paths, much like paths listed on the command line do. That'd also be more reusable for log when we add a similar option for it.

Quoted 22 lines

diff.c +2 -0

+
 static void diffcore_apply_filter(struct diff_options *options)
 {
        int i;

@@ -6827,6 +6883,7 @@ void diffcore_std(struct diff_options *options)
                /* See try_to_follow_renames() in tree-diff.c */
                diff_resolve_rename_copy();
        diffcore_apply_filter(options);
+       diffcore_apply_restrict(options);
 
        if (diff_queued_diff.nr && !options->flags.diff_from_contents)
                options->flags.has_changes = 1;

diff.h +1 -0

index 8ae18e5ab1e..f651216da13 100644
@@ -396,6 +396,7 @@ struct diff_options {
        struct repository *repo;
        struct option *parseopts;
        struct strmap *additional_path_headers;
+       struct pattern_list *sparse_checkout_patterns;
 

t/t1092-sparse-checkout-compatibility.sh

Quoted 6 lines

diff.h +0 -0

        int no_free;
 };

t/t1092-sparse-checkout-compatibility.sh +0 -0

index b9350c075c2..4c416ef8e82 100755

The description of this file is
test_description='compare full workdir to sparse workdir'
which seems like a misfit for the tests you are adding. I'd suggest placing them somewhere else. t1090 might be a good candidate.

Quoted 26 lines

t/t1092-sparse-checkout-compatibility.sh +22 -0

@@ -504,6 +504,36 @@ test_expect_success 'diff --cached' '
        test_all_match git diff --cached
 '
 
+test_expect_success 'diff --restrict' '
+       init_repos &&
+
+       test_all_match mkdir modules &&
+       test_all_match touch modules/a &&
+       test_all_match touch deep/b &&
+       test_all_match git rm deep/a &&
+       test_all_match git add --sparse modules deep &&
+       run_on_all git diff --restrict --staged --stat &&
+       cat >expect <<-EOF &&
+        deep/a | 1 -
+        deep/b | 0
+        2 files changed, 1 deletion(-)
+       EOF
+       test_cmp expect sparse-checkout-out &&
+       cat >expect <<-EOF &&
+        deep/a | 1 -
+        deep/b | 0
+        2 files changed, 1 deletion(-)
+       EOF
+       test_cmp expect sparse-index-out &&
+       cat >expect <<-EOF &&

t/t1092-sparse-checkout-compatibility.sh test_expect_success 'diff --cached' ' · line 538

Quoted 10 lines

t/t1092-sparse-checkout-compatibility.sh +8 -0

+        deep/a    | 1 -
+        deep/b    | 0
+        modules/a | 0
+        3 files changed, 1 deletion(-)
+       EOF
+       test_cmp expect full-checkout-out
+'
+
 # NEEDSWORK: sparse-checkout behaves differently from full-checkout when
 # running this test with 'df-conflict-2' after 'df-conflict-1'.

Just a guess, but is this NEEDSWORK perhaps reflecting the fact that the test is in the wrong file?

The whole point of --restrict is to make the output you'd get within a sparse-checkout NOT match what you'd get in a full checkout, because you are restricting the output to a subset. If you want to compare to a full-checkout and make sure the output matches, you'd want to use --no-restrict, right?