Re: [PATCH] diff: introduce --restrict option

[ZheNing Hu <adlternative@gmail.com>]

Elijah Newren <newren@gmail.com> 于2022年9月25日周日 05:32写道：

>
> 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.)
>

I'd love to see some improvements of monorepo on git, So I'm happy to
help (or helped by others) in this area. :-)

> On Sat, Sep 24, 2022 at 9:14 AM ZheNing Hu via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> >
> > From: ZheNing Hu <adlternative@gmail.com>
> >
> > 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"?
>

Yeah, "specification", "filterspec"... They might be better words.

> > 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.
>
> ?
>

Yes, that's good.

> > Signed-off-by: ZheNing Hu <adlternative@gmail.com>
> > ---
> >     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.
>

Make sense. The patch should add an RFC header, because I think
there are a lot of other git command that might need to use this diff --restrict
option, e.g. git log... and want you mentioned is git pull.

Do we make it work by git config "diff.restrict=true|false" , or do we
just use this
diff option to other git commands, like git log --restrict (like git
log --diff-filter)?

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

Good advice.

> >     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/xmqqzgeqw0sy.fsf@gitster.g/
> >
> > 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(+)
> >
> > diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
> > index 3674ac48e92..8ee5b6b4603 100644
> > --- a/Documentation/diff-options.txt
> > +++ b/Documentation/diff-options.txt
>
> 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.
>

I'm a little confused here: git log/git show/git format-patch already
naturally inherits the --restrict option from git diff. If we don't really want
the --restrict option in the documentation of git format-patch, does that
mean we should let git format-patch disable --restrict?

> > @@ -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.
>

Yes, it does get a little cluttered. “sparse” was probably the most appropriate
name, but some other command like “git rev-list --sparse” or
"git pack-objects --sparse" has taken precedence and conveyed another
level of meaning. So "restrict" may be an appropriate word in this case.

> 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.
>

Well, here you answered my question above, I also understood the points
that plagued us in moving forward on this feature:

"Which git commands should we use this -[no]-restrict on?"

> 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.
>

command-specific can make some common git command (i.e.
git format-patch, git stash) have consistent behavior, then use it if
we really need to work on sparse-checkout specification, e.g.
git log, git pull.

I will check your RFC patch later.

> 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.
>

Ah, the long patch set, I wonder how many collisions happened :)

> [1] https://lore.kernel.org/git/xmqqh719pcoo.fsf@gitster.g/
> [2] https://lore.kernel.org/git/a86af661-cf58-a4e5-0214-a67d3a794d7e@github.com/
> [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/20220923041842.27817-1-shaoxuan.yuan02@gmail.com/
>
> >  -S<string>::
> >         Look for differences that change the number of occurrences of
> >         the specified string (i.e. addition/deletion) in a file.
> > diff --git a/diff.c b/diff.c
> > index 648f6717a55..95e13607041 100644
> > --- a/diff.c
> > +++ b/diff.c
> > @@ -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,
> > +                               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.
>

Agree.

> > +}
> > +
> >  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,
> > +                              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?
>

Agree.

> > +                              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;
> > +}
> > +
> > +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?
>

Yes, it is true that simplification can be done here.

> > +}
> > +
> >  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);
> > +       }
> > +       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.
>

Yes, the temporary implementation was made by learning from the
-diff-filter implementation. I may have to look at the code elsewhere
if it needs to be filtered at the beginning of the traversal of the file.

> > +
> >  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 --git a/diff.h b/diff.h
> > index 8ae18e5ab1e..f651216da13 100644
> > --- a/diff.h
> > +++ b/diff.h
> > @@ -396,6 +396,7 @@ struct diff_options {
> >         struct repository *repo;
> >         struct option *parseopts;
> >         struct strmap *additional_path_headers;
> > +       struct pattern_list *sparse_checkout_patterns;
> >
> >         int no_free;
> >  };
> > diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
> > index b9350c075c2..4c416ef8e82 100755
> > --- a/t/t1092-sparse-checkout-compatibility.sh
> > +++ b/t/t1092-sparse-checkout-compatibility.sh
>
> 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.
>

Agree.

> > @@ -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 &&
> > +        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?

Yes, a test compare between --restrict and --no-restrict will be better.

Thanks for review~~~
--
ZheNing Hu