Re: [PATCH] rm: honor sparse checkout patterns

[Elijah Newren <newren@gmail.com>]

Hi,

On Mon, Nov 16, 2020 at 9:20 PM Elijah Newren <newren@gmail.com> wrote:
>
> On Mon, Nov 16, 2020 at 12:14 PM Junio C Hamano <gitster@pobox.com> wrote:
> >
> > Matheus Tavares <matheus.bernardino@usp.br> writes:
> >
> > > Make git-rm honor the 'sparse.restrictCmds' setting, by restricting its
> > > operation to the paths that match both the command line pathspecs and
> > > the repository's sparsity patterns.
> >
> > > This better matches the expectations
> > > of users with sparse-checkout definitions, while still allowing them
> > > to optionally enable the old behavior with 'sparse.restrictCmds=false'
> > > or the global '--no-restrict-to-sparse-paths' option.
> >
> > Hmph.  Is "rm" the only oddball that ignores the sparse setting?
>
> This might make you much less happy, but in general none of the
> commands pay attention to the setting; I think a line or two in

This isn't quite right; as noted at the just submitted [1], there are
three different classes of ways that existing commands at least
partially pay attention to the setting.

[1] https://lore.kernel.org/git/5143cba7047d25137b3d7f8c7811a875c1931aee.1605891222.git.gitgitgadget@gmail.com/

> merge-recursive.c is the only part of the codebase outside of
> unpack_trees() that pays any attention to it at all.  This was noted
> as a problem in the initial review of the sparse-checkout series at
> [1], and was the biggest factor behind me requesting the following
> being added to the manpage for sparse-checkout[2]:
>
> THIS COMMAND IS EXPERIMENTAL. ITS BEHAVIOR, AND THE BEHAVIOR OF OTHER
> COMMANDS IN THE PRESENCE OF SPARSE-CHECKOUTS, WILL LIKELY CHANGE IN
> THE FUTURE.

The fact that commands have only somewhat paid attention to this
setting is still a problem, though.  In fact, it was apparently a
known problem as far back as 2009 just from looking at the short list
of TODOs at the end of that file.

> > >  to the paths specified by the sparsity patterns, or to the intersection of
> > >  those paths and any (like `*.c`) that the user might also specify on the
> > >  command line. When false, the affected commands will work on full trees,
> > > -ignoring the sparsity patterns. For now, only git-grep honors this setting.
> > > +ignoring the sparsity patterns. For now, only git-grep and git-rm honor this
> > > +setting.
> >
> > I am not sure if this is a good direction to go---can we make an
> > inventory of all commands that affect working tree files and see
> > which ones need the same treatment before going forward with just
> > "grep" and "rm"?  Documenting the decision on the ones that will not
> > get the same treatment may also be a good idea.  What I am aiming
> > for is to prevent users from having to know in which versions of Git
> > they can rely on the sparsity patterns with what commands, and doing
> > things piecemeal like these two topics would be a road to confusion.
>
> It's not just commands which affect the working tree that need to be
> inventoried and adjusted.  We've made lists of commands in the past:
>
> [3] https://lore.kernel.org/git/CABPp-BEbNCYk0pCuEDQ_ViB2=varJPBsVODxNvJs0EVRyBqjBg@mail.gmail.com/
> [4] https://lore.kernel.org/git/xmqqy2y3ejwe.fsf@gitster-ct.c.googlers.com/

So, I think there are a few other commands that need to be modified
the same way rm is here by Matheus, a longer list of commands than
what I previously linked to for other modifications, some warnings and
error messages that need to be cleaned up, and a fair amount of
additional testing needed.  I also think we need to revisit the flag
names for --restrict-to-sparse-paths and
--no-restrict-to-sparse-paths; some feedback I'm getting suggest they
might be more frequently used than I originally suspected and thus we
might want shorter names.  (--sparse and --dense?)  So we probably
want to wait off on both mt/grep-sparse-checkout and
mt/rm-sparse-checkout (sorry Matheus) and maybe my recently submitted
stash changes (though those don't have an exposed
--[no]-restrict-to-sparse-paths flag and are modelled on existing
merge behavior) until we have a bigger plan in place.

But I only dug into it a bit while working on the stash apply bug; I'm
going to dig more (probably just after Thanksgiving) and perhaps make
a Documentation/technical/ file of some sort to propose more plans
here.