Re: [PATCH] rm: honor sparse checkout patterns

From: Elijah Newren <newren@gmail.com>
Date: Nov 20, 2020, 17:06 UTC
Message-ID: <CABPp-BGMX3wb7LiS1HkJpGveoW3J1oR0vVHbKTF5+qYLRF+59g@mail.gmail.com>
In-Reply-To: <CABPp-BHvZyb4cF29HqDYgMHTMEr2LdvKYATWqADRyhqJzB=Liw@mail.gmail.com>

Hi,

On Mon, Nov 16, 2020 at 9:20 PM Elijah Newren <newren@gmail.com> wrote:

Show 18 quoted lines

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

Show 9 quoted lines

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

Show 21 quoted lines

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