Re: [PATCH v3 0/7] sparse-checkout: add 'clean' command

From: Elijah Newren <newren@gmail.com>
Date: Oct 7, 2025, 23:07 UTC
Message-ID: <CABPp-BEEHsFwE-bDjcUoDtAYm9pvVN0tGUaoh0KPEJu23LywOQ@mail.gmail.com>
In-Reply-To: <pull.1941.v3.git.1757673011.gitgitgadget@gmail.com>

On Fri, Sep 12, 2025 at 3:30 AM Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 40 quoted lines

>
> NEW: This series is rebased on a recent master to remove dependence on the
> updates to the global variables used by the sparse-checkout system.
>
> When using cone-mode sparse-checkout, users specify which tracked
> directories they want (recursively) and any directory not part of the parent
> paths for those directories are considered "out of scope". When changing
> sparse-checkouts, there are a variety of reasons why these "out of scope"
> directories could remain, including:
>
>  * The user has .gitignore or .git/info/exclude files that tell Git to not
>    remove files of a certain type.
>  * Some filesystem blocker prevented the removal of a tracked file. This is
>    usually more of an issue on Windows where a read handle will block file
>    deletion.
>
> Typically, this would not mean too much for the user experience. A few extra
> filesystem checks might be required to satisfy git status commands, but the
> scope of the performance hit is relative to how many cruft files are left
> over in this situation.
>
> However, when using the sparse index, these tracked sparse directories cause
> significant performance issues. When noticing that the index contains a
> sparse directory but that directory exists on disk, Git needs to expand that
> sparse directory to determine which files are tracked or untracked. The
> current mechanism expands the entire index to a full one, an expensive
> operation that scales with the total number of paths at HEAD and not just
> the number of cruft files left over.
>
> Advice was added in 9479a31d603 (advice: warn when sparse index expands,
> 2024-07-08) to help users determine that they were in this state. However,
> the advice doesn't actually recommend helpful ways to get out of this state.
> Recommending "git clean" on its own is incomplete, as typically users
> actually need 'git clean -dfx' to clear out the ignored or excluded files.
> Even then, they may need 'git sparse-checkout reapply' afterwards to clear
> the sparse directories.
>
> The advice was successful in helping to alert users to the problem, which is
> how I got wind of many of these cases for how users get into this state.
> It's now time to give them a tool that helps them out of this state.

...in v2, I found some cases where the tool doesn't help them get out of this state. In v3, you documented those cases, and didn't attempt to provide a combined tool. I'm a little disappointed at the end-state, because it means we tell users to use a combination of commands, and they may have to figure out the order to run those commands in. However, I think with the documentation you've got, we've at least improved on the status quo, so we could always make further improvements later.

There was an error message and an advice message that I think could be touched up to improve on this (commented on both in v3), otherwise I think this series is good enough to merge down.

[...]

> This option would be preferred to something like 'git clean -dfx' since it
> does not clear the excluded files that are still within the sparse-checkout.
> Instead, it performs the exact filesystem operations required to refresh the
> sparse index performance back to what is expected.

This paragraph is the same from v2 of the cover letter, but we know this paragraph to be false -- the new command only works in a subset of applicable cases, otherwise an additional command (sparse-checkout reapply) is also needed. So, it feels like this paragraph should be updated.

Show 5 quoted lines

> Updates in V3
> =============
>
> Huge thanks to Elijah for such a detailed review. Apologies for the delay in
> responding.

Likewise...it's nearly been a month since you sent this.  :-(

Show 9 quoted lines

>  * Removed dependency on stalled series around updating the sparse-checkout
>    globals.
>  * Commit message and documentation is updated to better describe the
>    conditions that qualify a file or directory for removal.
>  * Tests are expanded significantly to include special cases and
>    aftereffects.
>  * A note is added around possible future expansion of the --verbose option
>    to include more detailed status information on the files that would be
>    deleted.

All much appreciated.

Show 7 quoted lines

>  * Due to a situation where a file appears as "modified and deleted" after
>    the more aggressive updating of the tree, the previous patch 8 is removed
>    (for now). I may reconsider and send a version in the future that avoids
>    this issue. Tests from the earlier patches are more expanded in such a
>    way that the aggressive implementation requires test changes that reveal
>    this problem. See [1] for a copy of this change and how it impacts the
>    latest tests.

Yeah, I think I was hoping that patch 8 would instead be modified to handle the additional cases (or more patches added to make it all work out), but punting that for future work seems viable too.

In summary, I think this series is close to ready to merge, but I think a couple wording improvements to an error message and advice message that I called out in separate emails on this series makes sense to fix up first.

Thanks for working on this!