Re: [PATCH v2 1/5] sparse-checkout: refactor skip worktree retry logic

From: Elijah Newren <newren@gmail.com>
Date: Jun 28, 2024, 00:31 UTC
Message-ID: <CABPp-BFzxOjGto+1GGk1xq3XX7OQ3mmmRYm0zU+2mCYZ6h_OfQ@mail.gmail.com>
In-Reply-To: <93d0baed0b0f435e5656cef04cf103b5e2e0f41a.1719412192.git.gitgitgadget@gmail.com>

On Wed, Jun 26, 2024 at 7:29 AM Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com> wrote:

>
> From: Derrick Stolee <stolee@gmail.com>
>

[...]

Show 5 quoted lines

> If users are having trouble with the performance of this operation and
> don't care about paths outside of the sparse-checkout, they can disable
> them using the sparse.expectFilesOutsideOfPatterns config option
> introduced in ecc7c8841d (repo_read_index: add config to expect files
> outside sparse patterns, 2022-02-25).

So, I had some heartburn with this paragraph when reading v1, but decided to focus on other stuff. But it still really bugs me while reading v2. So...

The purpose for the sparse.expectFilesOutsideOfPatterns option is very specifically for the virtual-filesystem usecase (Google, specifically) where sparse files aren't meant to stay sparse but be brought to life the instant they are accessed (via a specialized filesystem and kernel modules and whatnot). Using it for any other reason, such as a workaround for performance issues here, seems risky to me and I don't like seeing it suggested. The "if users...don't care about paths outside of the sparse-checkout" really doesn't do it justice. See the "User-facing issues" section of https://lore.kernel.org/git/b263cc75b7d4f426fb051d2cae5ae0fabeebb9c9.1642092230.git.gitgitgadget@gmail.com/; we're foisting all those bugs back on users if they don't have such a specialized filesystem and turn this knob on. And then we'll get many bug reports that are close to impossible to track down, and virtually impossible to fix even if we do track it down. I had for a while suggested numerous fixes we needed in order to make SKIP_WORKTREE-but-actually-present files work better, which required fixes all over the codebase and which was serving as an impediment e.g. to Victoria's desired sparse-index changes. We gave up on those other efforts long ago in favor of this much cleaner and simpler solution of just clearing the SKIP_WORKTREE bit if the file wasn't missing.

Further, I've seen people read git.git commit messages on Stack Overflow and make suggestions based off them, so I'm a bit worried this paragraph as worded will end up causing active harm, when I think it's original intent was merely to provide full context around the history of the clear_skip_worktree_from_present_files_sparse() function. While this bit of information is part of the history of this function, I'm not sure it's really all that relevant to your series.

Could we omit this paragraph, or if you want to keep it, reword it in some way that doesn't make it look like some tradeoff that isn't that big of a deal for non-specialized-vfs users?