On Wed, Dec 29, 2021 at 4:40 AM Elijah Newren <newren@xxxxxxxxx> wrote:>
> On Tue, Dec 28, 2021 at 1:32 PM Derrick Stolee via GitGitGadget
> <gitgitgadget@xxxxxxxxx> wrote:
> > ++init-worktree-config::
> > ++
> > ++Initialize config settings to enable worktree-specific config settings.
> > ++This will set `core.repositoryFormatversion=1` and enable
> > ++`extensions.worktreeConfig`, which might cause some third-party tools from
> > ++being able to operate on your repository. See CONFIGURATION FILE for more
> > ++details.
>
> So, if users attempt to use `git worktree add` or `git sparse-checkout
> {init,set}` without first running this, they can break other
> worktrees. And if they do run this new command, they potentially
> break third-party tools or older git versions.
When you say "can break other worktrees", you don't necessarily mean
that in general but rather in regard to sparse-checkout -- in
particular, the sparse-checkout config settings and the
`info/sparse-checkout file` -- correct? (Genuine question; I want to
make sure that I'm actually understanding the issues under
discussion.)
> > + After these steps, the first worktree will have sparse-checkout enabled
> > + with whatever patterns exist. The worktree does not immediately have
> > + those patterns applied, but a variety of Git commands would apply the
> > + sparse-checkout patterns and update the worktree state to reflect those
> > + patterns. This situation is likely very rare and the workaround is to
>
> No, it's not even rare, let alone very rare. I'd actually call it
> common. Since 'sparse-checkout disable' does not delete the
> sparse-checkout file, and we've encouraged folks to use the
> sparse-checkout command (or a wrapper thereof) instead of direct
> editing of the sparse-checkout file (and indeed, the sparse-checkout
> command will overwrite the sparse-checkout file which further
> discourages users from feeling they own it), having the file left
> around after disabling is the common case. So, the only question is,
> how often do users disable and re-enable sparse-checkout, and
> potentially only do so in some of their worktrees? At my $DAYJOB,
> that's actually quite common. I got multiple reports quite soon after
> introducing our `sparsify` tool about users doing something like this;
> this is what led me to learn of the extensions.worktreeConfig, and why
> I pointed it out to you on your first submission of
> git-sparse-checkout[1]
> (https://lore.kernel.org/git/CABPp-BFcH5hQqujjmc88L3qGx3QAYZ_chH6PXQXyp13ipfV6hQ@xxxxxxxxxxxxxx/)
I think the link you provide here answers the genuine question I had
asked in [1] where I didn't understand why git-sparse-checkout is
forcing the repository to use per-worktree configuration. I also just
(re)discovered [2] which makes it clear that per-worktree
sparse-checkout was considered important very early on in the
development of "multiple checkouts".
[1]: https://lore.kernel.org/git/CAPig+cRombN-8g0t7Hs9qQypJoY41gK3+kvypH4D0G6EB4JgbQ@xxxxxxxxxxxxxx/
[2]: https://lore.kernel.org/git/1404891197-18067-32-git-send-email-pclouds@xxxxxxxxx/
> So, here's the experience I expect from these patches at $DAYJOB:
> (1) Several users per week hit the case of one worktree being
> sparsified when it wasn't supposed to be.
> (2) These users have no idea how to figure out what they need to do
> to fix it. The init-worktree-config is no more discoverable than the
> documentation on the official steps for enabling
> extensions.worktreeConfig (See
> https://lore.kernel.org/git/CABPp-BGKyDJV9DP+igmCC_Ad0jgvb4aOAYpXWCbx9hW8ShhDQg@xxxxxxxxxxxxxx/
> up through the paragraph, "Further, it's not even clear people would
> look at git-worktree.txt.)
> (3) Even if they do discover it, and run it, it's an extra step they
> never needed to take before. Why are we adding a "unbreak these other
> commands we want to run" step?
> (4) Also, even if they do discover it, and run it, suddenly we are
> setting core.repositoryFormatVersion=1. That scares me. I have years
> of experience at $DAYJOB saying that the tooling we have works fine
> with extensions.worktreeConfig=true. I have none with setting
> core.repositoryFormatVersion=1, but now we're effectively requiring it
> by your documentation. I have no idea how
> jgit/egit/other-random-stuff interacts with that. I'd be willing to
> do some tests with targetted users to try to learn more, but suddenly
> turning it on for everyone in cases that we know worked fine without
> it previously feels unsafe to me. Maybe I'm over-worrying here, but
> see also commit 11664196ac ("Revert "check_repository_format_gently():
> refuse
> extensions for old repositories"", 2020-07-15) -- it just feels a bit
> late to recommend for users, especially when they'll see it as "oh, if
> you don't want this other bug we recently introduced you need to run
> this....".
Point #4 is pretty compelling, and the "ship" of enforcing
`core.repositoryFormatVersion=1` when using `extension` configs has
"already sailed" anyhow, as 11664196ac ("Revert
"check_repository_format_gently(): refuse extensions for old
repositories"", 2020-07-15) clearly indicates.
> So, I'd like to reiterate my earlier suggestion which would avoid
> these regressions while also fixing the reported bug:
> * If core.bare=true or core.worktree is set, then at `git worktree
> add` time, automatically run the logic you have here for
> init-worktree-config. Having either of those config settings with
> multiple worktrees is currently broken in all git versions and likely
> in most all external tools. As such, being aggressive in the new
> config settings to allow new versions of git to work seems totally
> safe to me -- we can't be any more broken than we already were.
> * If core.bare=false and core.worktree is not set, then:
> * `git sparse-checkout {init,set}` should set
> extensions.worktreeConfig if not already set, and always set the
> core.sparse* and index.sparse settings in worktree-specific files.
> * `git worktree add`, if extensions.worktreeConfig is already set,
> will copy both the info/sparse-checkout file and the config.worktree
> settings (module core.bare and core.worktree, if present) to the new
> worktree
Thanks for the clearly written enumeration of how you expect this to
work. This summary pretty well (or entirely) captures the conclusions
I arrived at, as well, after devoting a chunk of time today thinking
through the cases. If I'm understanding everything correctly, the
approach outlined here solves the bare-worktree problem in the least
invasive and least dangerous way (for older Git versions and foreign
tools). And we don't even need the `git worktree init-worktree-config`
subcommand (though we need the underlying functionality).
