Re: [PATCH] rm: honor sparse checkout patterns

From: Elijah Newren <newren@gmail.com>
Date: Nov 17, 2020, 05:20 UTC
Message-ID: <CABPp-BHvZyb4cF29HqDYgMHTMEr2LdvKYATWqADRyhqJzB=Liw@mail.gmail.com>
In-Reply-To: <xmqqeekt83fl.fsf@gitster.c.googlers.com>

Hi,

On Mon, Nov 16, 2020 at 12:14 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 13 quoted lines

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

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

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.