Re: [PATCH] rm: honor sparse checkout patterns

[Elijah Newren <newren@gmail.com>]

Hi,

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

However, multiple groups were using sparse checkouts anyway, via
manually editing .git/info/sparse-checkout and running `git read-tree
-mu HEAD`, and adding various wrappers around it, and Derrick and I
thought there was value in getting _something_ out there to smooth it
out a little bit.  I'd still say it's pretty rough around the
edges...but useful nonetheless.

[1] https://lore.kernel.org/git/CABPp-BHJeuEHBDkf93m9sfSZ4rZB7+eFejiAXOsjLEUu5eT5FA@mail.gmail.com/
[2] https://lore.kernel.org/git/CABPp-BEryfaeYhuUsiDTaYdRKpK6GRi7hgZ5XSTVkoHVkx2qQA@mail.gmail.com/

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

But the working-directory related ones are perhaps more problematic.
One additoinal example: I just got a report today that "git stash
apply" dies with a fatal error and the working directory in some
intermediate state when trying to apply a stash when the working
directory has a different set of sparsity paths than when the stash
was created.  (Granted, an error makes sense, but this was throwing
untranslated error messages, meaning they weren't in codepaths that
were meant to be triggered.)  This case may not be an apples to apples
comparison, but the testcase did involve adding new files before
stashing, so the stash apply would have been trying to remove files.
Anyway, I'll send more details on that issue in a separate thread
after I've had some time to dig into it.


Anyway, I'm not sure this helps, because I'm basically saying things
are kind of messy, and we're fixing as we go rather than having a full
implementation and all the fixes.