| Message ID | pull.1101.v3.git.1640727143.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Sparse checkout: fix bug with worktree of bare repo | expand |
On Tue, Dec 28, 2021 at 1:32 PM Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com> wrote: > > This series is based on a merge of en/sparse-checkout-set, > en/worktree-chatty-to-stderr, and ds/sparse-checkout-malformed-pattern-fix. I think you mean es/worktree-chatty-to-stderr (not 'en/') > This patch series includes a fix to the bug reported by Sean Allred [1] and > diagnosed by Eric Sunshine [2]. > > The root cause is that 'git sparse-checkout init' writes to the worktree > config without checking that core.bare might need to be set. This only > matters when the base repository is bare, since creating the config.worktree > file and enabling extensions.worktreeConfig will cause Git to treat the base > repo's core.bare=false as important for this worktree. > > This series fixes this, but also puts in place some helpers to prevent this > from happening in the future. While here, some of the config paths are > modified to take a repository struct. > > * 'git sparse-checkout' will now modify the worktree config, if enabled. It > will no longer auto-upgrade users into using worktree config. This sounds dangerous to me. > * The new 'git worktree init-worktree-config' will upgrade users to using > worktree config. It will relocate core.bare and core.worktree if > necessary. This sounds like giving users an extra step to unbreak themselves, instead of just having the commands do it. (And might risk also breaking things in a different direction? I'll have to read over the patches to see...) > * 'git worktree add' will copy the sparse-checkout patterns from the > current worktree to the new one. If worktree config is enabled, then the > config settings from the current worktree are copied to the new > worktree's config file. This sounds awesome. Wahoo! (core.worktree should be cleared, though, and core.bare should either be cleared or set to false if found in the worktree config.) ... > Range-diff vs v2: > > 1: 889e69dc45d = 1: 749ba67d21e setup: use a repository when upgrading format > 2: 3e01356815a = 2: 61b96937016 config: make some helpers repo-aware > 3: ed8e2a7b19d ! 3: e2a0a458115 worktree: add upgrade_to_worktree_config() ... > @@ Commit message > Helped-by: Eric Sunshine <sunshine@sunshineco.com> > Signed-off-by: Derrick Stolee <dstolee@microsoft.com> > > - ## worktree.c ## > + ## Documentation/git-worktree.txt ## > +@@ Documentation/git-worktree.txt: SYNOPSIS > + -------- > + [verse] > + 'git worktree add' [-f] [--detach] [--checkout] [--lock [--reason <string>]] [-b <new-branch>] <path> [<commit-ish>] > ++'git worktree init-worktree-config' > + 'git worktree list' [-v | --porcelain] > + 'git worktree lock' [--reason <string>] <worktree> > + 'git worktree move' <worktree> <new-path> > +@@ Documentation/git-worktree.txt: checked out in the new working tree, if it's not checked out anywhere > + else, otherwise the command will refuse to create the working tree (unless > + `--force` is used). > + > ++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 s/cause/prevent/ ? > ++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. Yet, with the very common case (in fact, I'd go so far as to say the nearly universal case) of having both core.bare=false and no core.worktree set, we never had any such problems with the former logic. This seems like a serious regression to me. I'll keep reading. ... > +@@ Documentation/git-worktree.txt: versions will refuse to access repositories with this extension. > + > + Note that in this file, the exception for `core.bare` and `core.worktree` > + is gone. If they exist in `$GIT_DIR/config`, you must move > +-them to the `config.worktree` of the main working tree. You may also > +-take this opportunity to review and move other configuration that you > +-do not want to share to all working trees: > ++them to the `config.worktree` of the main working tree. These keys are > ++moved automatically when you use the `git worktree init-worktree-config` > ++command. > ++ > ++You may also take this opportunity to review and move other configuration > ++that you do not want to share to all working trees: > + > + - `core.worktree` and `core.bare` should never be shared I'm fine with the wording, but technically core.bare=false is fine to share, it's just core.bare=true that is not. (And yes, I agree that core.worktree is never safe to share) ... > 5: 06457fafa78 ! 5: b200819c1bb sparse-checkout: use repo_config_set_worktree_gently() > @@ Commit message > sparse-checkout: use repo_config_set_worktree_gently() > ... > + Let the sparse-checkout builtin use this helper instead of attempting to > + initialize the worktree config on its own. This changes behavior of 'git > + sparse-checkout set' in a few important ways: > > - The fix is to have this assignment into config.worktree be handled by > - the repo_config_set_worktree_gently() helper. > + 1. Git will no longer upgrade the repository format and add the > + worktree config extension. The user should run 'git worktree > + init-worktree-config' to enable this feature. > + > + 2. If worktree config is disabled, then this command will set the > + core.sparseCheckout (and possibly core.sparseCheckoutCone and > + index.sparse) values in the common config file. Yikes. > + 3. If the main worktree is bare, then this command will not put the > + worktree in a broken state. > + > + The main reason to use worktree-specific config for the sparse-checkout > + builtin was to avoid enabling sparse-checkout patterns in one and > + causing a loss of files in another. If a worktree does not have a > + sparse-checkout patterns file, then the sparse-checkout logic will not > + kick in on that worktree. > + > + This new logic introduces a new user pattern that could lead to some > + confusion. Suppose a user has not upgraded to worktree config and > + follows these steps in order: > + > + 1. Enable sparse-checkout in a worktree. > + > + 2. Disable sparse-checkout in that worktree without deleting that > + worktree's sparse-checkout file. > + > + 3. Enable sparse-checkout in another worktree. > + > + 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@mail.gmail.com/) Some more details about sparse-checkouts at $DAYJOB: * We have numerous users who used to have their own small little repos, and want to still have that experience. sparse-checkouts were a way to provide a small-repo feel for them, and in particular to speed up IDEs that otherwise insist on indexing everything no matter how much we try to tell them to only pay attention to part of the files (and similar speed issues with the build system since gradle configuration is so slow). They want every worktree to be sparse and never face the full repo. I talk about these users the most, because we've mostly satisfied the other users. * We have numerous users who work mostly on specific modules, but occasionally do cross-cutting work. They will have most worktrees be sparse, but keep at least one that is full. They may also undo sparsity briefly and redo it in various worktrees. * We also have a few users who work primarily on cross-cutting features and just keep the full checkout and ignore the sparse-checkout stuff. * There is also a special tool, write-locks.sh or some name like that, which must unsparsify, run something like `./gradle --write-locks` and other stuff, and then re-sparsify. The tool does not work without the full worktree. Now, not all users need to run this, and in fact most users who do probably are the same ones that "occasionally do cross-cutting work", but it's a little bit wider group than that. So we have additional unsparsify/resparsify steps, and it's further complicated by the fact that users may not even know that we do the unsparsification and resparsification behind the scenes for them (our `sparsify` wrapper has a stash-like feature, though I think only allowing for one on the stash, which is called behind the scenes.) The write-locks logic is super slow, which kind of hides the otherwise slow unsparsify/resparsify steps. (For each of the above type of users, we have many folks who don't use worktrees, but they're not relevant to this discussion.) We also have third party git tools, whether jgit (usually via gradle plugins), whatever IDEs tend to use (eclipse used egit, not sure what intellij or visual studio use), probably various special one-off scripts that read a bit more git configuration than they should and do various tasks, and a wide range of git versions in use (though any given user will likely only use one git version, and we are perfectly happy to specify a minimum version for sparse-checkout usage). So that's the range of users for this discussion, but I think we also need to flesh out the caveats of your change since you missed a few: * Even if users haven't sparsified/unsparsified/re-sparsified in another worktree (i.e. they have a worktree that has always been full), the sparse-checkout init/set in another worktree _still_ causes problems because when users switch to the 'full' worktree, contrib/completion/git-prompt.sh will report it as sparse. Users are going to get confused and report bugs just based on their prompt even if it's technically a "dense" worktree. * The cone-mode and sparse-index are also shared between worktrees, which may not present many problems for me right now at $DAYJOB, but could be problematic for other users out there. 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@mail.gmail.com/ 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....". 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
On 12/29/2021 4:39 AM, Elijah Newren wrote: > On Tue, Dec 28, 2021 at 1:32 PM Derrick Stolee via GitGitGadget > <gitgitgadget@gmail.com> wrote: >> >> This series is based on a merge of en/sparse-checkout-set, >> en/worktree-chatty-to-stderr, and ds/sparse-checkout-malformed-pattern-fix. > > I think you mean es/worktree-chatty-to-stderr (not 'en/') Yes. Thanks. >> This patch series includes a fix to the bug reported by Sean Allred [1] and >> diagnosed by Eric Sunshine [2]. >> >> The root cause is that 'git sparse-checkout init' writes to the worktree >> config without checking that core.bare might need to be set. This only >> matters when the base repository is bare, since creating the config.worktree >> file and enabling extensions.worktreeConfig will cause Git to treat the base >> repo's core.bare=false as important for this worktree. >> >> This series fixes this, but also puts in place some helpers to prevent this >> from happening in the future. While here, some of the config paths are >> modified to take a repository struct. >> >> * 'git sparse-checkout' will now modify the worktree config, if enabled. It >> will no longer auto-upgrade users into using worktree config. > > This sounds dangerous to me. > ... >> + Let the sparse-checkout builtin use this helper instead of attempting to >> + initialize the worktree config on its own. This changes behavior of 'git >> + sparse-checkout set' in a few important ways: >> >> - The fix is to have this assignment into config.worktree be handled by >> - the repo_config_set_worktree_gently() helper. >> + 1. Git will no longer upgrade the repository format and add the >> + worktree config extension. The user should run 'git worktree >> + init-worktree-config' to enable this feature. >> + >> + 2. If worktree config is disabled, then this command will set the >> + core.sparseCheckout (and possibly core.sparseCheckoutCone and >> + index.sparse) values in the common config file. > > Yikes. > >> + 3. If the main worktree is bare, then this command will not put the >> + worktree in a broken state. >> + >> + The main reason to use worktree-specific config for the sparse-checkout >> + builtin was to avoid enabling sparse-checkout patterns in one and >> + causing a loss of files in another. If a worktree does not have a >> + sparse-checkout patterns file, then the sparse-checkout logic will not >> + kick in on that worktree. >> + >> + This new logic introduces a new user pattern that could lead to some >> + confusion. Suppose a user has not upgraded to worktree config and >> + follows these steps in order: >> + >> + 1. Enable sparse-checkout in a worktree. >> + >> + 2. Disable sparse-checkout in that worktree without deleting that >> + worktree's sparse-checkout file. >> + >> + 3. Enable sparse-checkout in another worktree. >> + >> + 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@mail.gmail.com/) Thank you for these comments and the detailed descriptions of things from your $DAYJOB. That's helpful context and I'm happy to switch back to enabling the extension in the sparse-checkout builtin. I might need to rearrange the code so there is an API in worktree.c instead of just the subcommand in builtin/worktree.c, but that's pretty minor. I'll keep Eric's earlier suggestion to have the upgrade be a separate call from the repo_config_set_worktree_gently(). Thanks, -Stolee
On Wed, Dec 29, 2021 at 4:40 AM Elijah Newren <newren@gmail.com> wrote:> > On Tue, Dec 28, 2021 at 1:32 PM Derrick Stolee via GitGitGadget > <gitgitgadget@gmail.com> 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@mail.gmail.com/) 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@mail.gmail.com/ [2]: https://lore.kernel.org/git/1404891197-18067-32-git-send-email-pclouds@gmail.com/ > 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@mail.gmail.com/ > 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).
On Wed, Dec 29, 2021 at 12:39 PM Derrick Stolee <stolee@gmail.com> wrote: > On 12/29/2021 4:39 AM, Elijah Newren wrote: > > 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@mail.gmail.com/) > > Thank you for these comments and the detailed descriptions of things > from your $DAYJOB. That's helpful context and I'm happy to switch back > to enabling the extension in the sparse-checkout builtin. I might need > to rearrange the code so there is an API in worktree.c instead of just > the subcommand in builtin/worktree.c, but that's pretty minor. I'll > keep Eric's earlier suggestion to have the upgrade be a separate call > from the repo_config_set_worktree_gently(). Thanks.
On 12/30/2021 2:40 AM, Eric Sunshine wrote: > On Wed, Dec 29, 2021 at 4:40 AM Elijah Newren <newren@gmail.com> wrote:> Taking time to focus only on this outline here: >> 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. I'm not sure I agree with the "currently broken in all git versions" because when extensions.worktreeConfig is not enabled, the core.bare and core.worktree settings are ignored by the worktrees. This upgrade during 'add' is the only thing I am not so sure about. >> * 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. This should happen no matter the case of core.bare and core.worktree existing, right? >> * `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 and here, 'git worktree add' should always copy the info/sparse-checkout file (if core.sparseCheckout is enabled) and copy the config.worktree settings if extensions.worktreeConfig is enabled (and filter out core.bare and core.worktree in the process). > 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). I'm happy to drop the subcommand in favor of some documentation in git-config.txt (Documentation/config/extensions.txt to be exact). Thanks, -Stolee
On Wed, Dec 29, 2021 at 11:40 PM Eric Sunshine <sunshine@sunshineco.com> wrote: > > On Wed, Dec 29, 2021 at 4:40 AM Elijah Newren <newren@gmail.com> wrote:> > > On Tue, Dec 28, 2021 at 1:32 PM Derrick Stolee via GitGitGadget > > <gitgitgadget@gmail.com> 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.) Yes, thanks for the clarifications. ... > > 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). I'm glad it helps, and that we appear to be moving towards consensus. :-)
On Thu, Dec 30, 2021 at 9:41 AM Derrick Stolee <stolee@gmail.com> wrote: > > On 12/30/2021 2:40 AM, Eric Sunshine wrote: > > On Wed, Dec 29, 2021 at 4:40 AM Elijah Newren <newren@gmail.com> wrote:> > > Taking time to focus only on this outline here: > > >> 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. > > I'm not sure I agree with the "currently broken in all git versions" > because when extensions.worktreeConfig is not enabled, the core.bare > and core.worktree settings are ignored by the worktrees. This upgrade > during 'add' is the only thing I am not so sure about. Oh, you're right; I mis-spoke. If someone has core.bare=true and has multiple worktrees, AND never attempts to use sparse-checkouts OR otherwise set extensions.worktreeConfig, then git still works due to git's special-case logic that will override core.bare in this configuration. It's just setting them up for a ticking time bomb, waiting for them to either use an external tool that doesn't share that special case override-core.bare logic, or for the user to decide to set extensions.worktreeConfig directly or use sparse-checkouts. > >> * 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. > > This should happen no matter the case of core.bare and core.worktree > existing, right? Hmm. I think that's safe for people who cloned and used `git worktree add` with newer git versions, since `git worktree add` will have moved core.bare and core.worktree to the config.worktree file when those have non-default values. But, we might want to help out the folks who have existing repos with which they have used older git versions. So, we could have `git sparse-checkout {init,set}` check for non-default values of core.bare/core.worktree in the shared config file, and, if found, exit with an error which point users at some relevant documentation (which may just suggest 'git worktree add temporary && git worktree remove temporary' as a workaround for those caught in such a state.) > >> * `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 > > and here, 'git worktree add' should always copy the info/sparse-checkout > file (if core.sparseCheckout is enabled) and copy the config.worktree > settings if extensions.worktreeConfig is enabled (and filter out > core.bare and core.worktree in the process). Right. > > 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). > > I'm happy to drop the subcommand in favor of some documentation in > git-config.txt (Documentation/config/extensions.txt to be exact). You may also want to make the two existing references to extensions.worktreeConfig within Documentation/git-config.txt point users at the extended documentation you add to Documentation/config/extensions.txt. (Remember, I found a reference to extensions.worktreeConfig, tried it, and started using and recommending it without it ever occurring to me that there was a more detailed explanation elsewhere, so only adding to Documentation/config/extensions.txt might run into the same discoverability issues.)
On Thu, Dec 30, 2021 at 2:29 PM Elijah Newren <newren@gmail.com> wrote: > On Thu, Dec 30, 2021 at 9:41 AM Derrick Stolee <stolee@gmail.com> wrote: > > On 12/30/2021 2:40 AM, Eric Sunshine wrote: > > > On Wed, Dec 29, 2021 at 4:40 AM Elijah Newren <newren@gmail.com> wrote:> > > >> * 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. > > > > I'm not sure I agree with the "currently broken in all git versions" > > because when extensions.worktreeConfig is not enabled, the core.bare > > and core.worktree settings are ignored by the worktrees. This upgrade > > during 'add' is the only thing I am not so sure about. > > Oh, you're right; I mis-spoke. If someone has core.bare=true and has > multiple worktrees, AND never attempts to use sparse-checkouts OR > otherwise set extensions.worktreeConfig, then git still works due to > git's special-case logic that will override core.bare in this > configuration. It's just setting them up for a ticking time bomb, > waiting for them to either use an external tool that doesn't share > that special case override-core.bare logic, or for the user to decide > to set extensions.worktreeConfig directly or use sparse-checkouts. So, how does this alter the proposed logic? Or does it? Does the above condition get revised to: if extensions.worktreeConfig=true and (.git/config contains core.bare=true or core.worktree): relocate core.bare/core.worktree to .git/config.worktree That is, we need to relocate core.bare and core.worktree from .git/config to .git/config.worktree if and only if extensions.worktreeConfig=true (because, due to the special-case handling, those two keys don't interfere with anything when extensions.worktreeConfig is not true). This, of course, doesn't help the case if someone has existing worktrees and decides to flip extensions.worktreeConfig to true without doing the manual bookkeeping, but that case has always been broken (and is documented, though not necessarily where people will look). The new `git worktree add` logic, however, will fix that brokenness automatically when a new worktree is added. > > >> * 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. > > > > This should happen no matter the case of core.bare and core.worktree > > existing, right? > > Hmm. I think that's safe for people who cloned and used `git worktree > add` with newer git versions, since `git worktree add` will have moved > core.bare and core.worktree to the config.worktree file when those > have non-default values. > > But, we might want to help out the folks who have existing repos with > which they have used older git versions. So, we could have `git > sparse-checkout {init,set}` check for non-default values of > core.bare/core.worktree in the shared config file, and, if found, exit > with an error which point users at some relevant documentation (which > may just suggest 'git worktree add temporary && git worktree remove > temporary' as a workaround for those caught in such a state.) I'm probably missing something obvious, but rather than error out, can't we just automatically relocate core.bare and core.worktree from .git/config to .git/config.worktree in this case?
This series is based on a merge of en/sparse-checkout-set, en/worktree-chatty-to-stderr, and ds/sparse-checkout-malformed-pattern-fix. This patch series includes a fix to the bug reported by Sean Allred [1] and diagnosed by Eric Sunshine [2]. The root cause is that 'git sparse-checkout init' writes to the worktree config without checking that core.bare might need to be set. This only matters when the base repository is bare, since creating the config.worktree file and enabling extensions.worktreeConfig will cause Git to treat the base repo's core.bare=false as important for this worktree. This series fixes this, but also puts in place some helpers to prevent this from happening in the future. While here, some of the config paths are modified to take a repository struct. * 'git sparse-checkout' will now modify the worktree config, if enabled. It will no longer auto-upgrade users into using worktree config. * The new 'git worktree init-worktree-config' will upgrade users to using worktree config. It will relocate core.bare and core.worktree if necessary. * 'git worktree add' will copy the sparse-checkout patterns from the current worktree to the new one. If worktree config is enabled, then the config settings from the current worktree are copied to the new worktree's config file. Updates in v3 ============= * This is now rebased onto a merge of: * en/sparse-checkout-set (for dependence on 'git sparse-checkout set --cone'), * en/worktree-chatty-to-stderr (for an adjacent change in Documentation/git-worktree.txt), and * ds/sparse-checkout-malformed-pattern-fix (since it needed a fixup! with the --worktree option in a test) * The strategy is changed significantly: we simultaneously stop auto-upgrading to worktree config in 'git sparse-checkout set' while also providing a clear way for users to self-upgrade in 'git worktree init-worktree-config'. This upgrade moves core.bare and core.worktree, if they exist, and does not do anything if worktree config is already enabled. * The sparse-checkout builtin will write to the worktree config, if it is enabled. The helper it uses now will fallback to the common config file if worktree config is not enabled. * The 'git worktree add' command is updated to copy the sparse-checkout patterns and config from the current worktree into the new one. [1] https://lore.kernel.org/git/CABceR4bZmtC4rCwgxZ1BBYZP69VOUca1f_moJoP989vTUZWu9Q@mail.gmail.com/ [2] https://lore.kernel.org/git/CAPig+cQ6U_yFw-X2OWrizB1rbCvc4bNxuSzKFzmoLNnm0GH8Eg@mail.gmail.com/ Update in v2 ============ * Eric correctly pointed out that I was writing core.bare incorrectly. It should move out of the core config and into the core repository's worktree config. * Patch 3 is new, separating the "upgrade" logic out of config.c, but it is still called by the config helper to make it painless to write worktree config. Thanks, -Stolee Derrick Stolee (6): setup: use a repository when upgrading format config: make some helpers repo-aware worktree: add 'init-worktree-config' subcommand config: add repo_config_set_worktree_gently() sparse-checkout: use repo_config_set_worktree_gently() worktree: copy sparse-checkout patterns and config on add Documentation/git-worktree.txt | 22 +++- builtin/sparse-checkout.c | 25 ++--- builtin/worktree.c | 123 +++++++++++++++++++++++ config.c | 50 ++++++++- config.h | 15 +++ list-objects-filter-options.c | 2 +- repository.h | 2 +- setup.c | 6 +- sparse-index.c | 10 +- t/t1091-sparse-checkout-builtin.sh | 110 ++++++++++++++++++-- t/t2407-worktree-init-worktree-config.sh | 68 +++++++++++++ 11 files changed, 385 insertions(+), 48 deletions(-) create mode 100755 t/t2407-worktree-init-worktree-config.sh base-commit: 998dc12e841b4b17dd5a4700bb443fa215505e3d Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1101%2Fderrickstolee%2Fsparse-checkout%2Fbare-worktree-bug-v3 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1101/derrickstolee/sparse-checkout/bare-worktree-bug-v3 Pull-Request: https://github.com/gitgitgadget/git/pull/1101 Range-diff vs v2: 1: 889e69dc45d = 1: 749ba67d21e setup: use a repository when upgrading format 2: 3e01356815a = 2: 61b96937016 config: make some helpers repo-aware 3: ed8e2a7b19d ! 3: e2a0a458115 worktree: add upgrade_to_worktree_config() @@ Metadata Author: Derrick Stolee <dstolee@microsoft.com> ## Commit message ## - worktree: add upgrade_to_worktree_config() + worktree: add 'init-worktree-config' subcommand - Some features, such as the sparse-checkout builtin, require using the + Some features, such as the sparse-checkout builtin, currently use the worktree config extension. It might seem simple to upgrade the - repository format and add extensions.worktreeConfig, and that is what - happens in the sparse-checkout builtin. + repository format and add extensions.worktreeConfig, which is what + happens in the sparse-checkout builtin. However, this is overly + simplistic and can cause issues in some cases. We will transition away + from making this upgrade automatically, but first we will make an easy + way for users to upgrade their repositories correctly. Transitioning from one config file to multiple has some strange side-effects. In particular, if the base repository is bare and the worktree is not, Git knows to treat the worktree as non-bare as a special case when not using worktree config. Once worktree config is enabled, Git stops that special case since the core.bare setting could - apply at the worktree config level. This opens the door for bare - worktrees. + apply at the worktree config level. - To help resolve this transition, create upgrade_to_worktree_config() to - navigate the intricacies of this operation. In particular, we need to - look for core.bare=true within the base config file and move that - setting into the core repository's config.worktree file. + Similarly, the core.worktree config setting is a precursor to the 'git + worktree' feature, allowing config to point to a different worktree, + presumably temporarily. This is special-cased to be ignored in a + worktree, but that case is dropped when worktree config is enabled. + + To help resolve this transition, create the 'git worktree + init-worktree-config' helper. This new subcommand does the following: + + 1. Set core.repositoryFormatVersion to 1 in the common config file. + 2. Set extensions.worktreeConfig to true in the common config file. + 3. If core.bare is true in the common config file, then move that + setting to the main worktree's config file. + 4. Move the core.worktree config value to the main worktree's config + file. + + If the repository is already configured to use worktree config, then + none of these steps happen. This preserves any state that the user might + have created on purpose. + + Update the documentation to mention this subcommand as the proper way to + upgrade to worktree config files. To gain access to the core repository's config and config.worktree file, we reference a repository struct's 'commondir' member. If the repository @@ Commit message Helped-by: Eric Sunshine <sunshine@sunshineco.com> Signed-off-by: Derrick Stolee <dstolee@microsoft.com> - ## worktree.c ## + ## Documentation/git-worktree.txt ## +@@ Documentation/git-worktree.txt: SYNOPSIS + -------- + [verse] + 'git worktree add' [-f] [--detach] [--checkout] [--lock [--reason <string>]] [-b <new-branch>] <path> [<commit-ish>] ++'git worktree init-worktree-config' + 'git worktree list' [-v | --porcelain] + 'git worktree lock' [--reason <string>] <worktree> + 'git worktree move' <worktree> <new-path> +@@ Documentation/git-worktree.txt: checked out in the new working tree, if it's not checked out anywhere + else, otherwise the command will refuse to create the working tree (unless + `--force` is used). + ++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. ++ + list:: + + List details of each working tree. The main working tree is listed first, +@@ Documentation/git-worktree.txt: already present in the config file, they will be applied to the main + working trees only. + + In order to have configuration specific to working trees, you can turn +-on the `worktreeConfig` extension, e.g.: ++on the `worktreeConfig` extension, using this command: + + ------------ +-$ git config extensions.worktreeConfig true ++$ git worktree init-worktree-config + ------------ + + In this mode, specific configuration stays in the path pointed by `git +@@ Documentation/git-worktree.txt: versions will refuse to access repositories with this extension. + + Note that in this file, the exception for `core.bare` and `core.worktree` + is gone. If they exist in `$GIT_DIR/config`, you must move +-them to the `config.worktree` of the main working tree. You may also +-take this opportunity to review and move other configuration that you +-do not want to share to all working trees: ++them to the `config.worktree` of the main working tree. These keys are ++moved automatically when you use the `git worktree init-worktree-config` ++command. ++ ++You may also take this opportunity to review and move other configuration ++that you do not want to share to all working trees: + + - `core.worktree` and `core.bare` should never be shared + + + ## builtin/worktree.c ## @@ - #include "worktree.h" - #include "dir.h" - #include "wt-status.h" -+#include "config.h" - void free_worktrees(struct worktree **worktrees) - { -@@ worktree.c: int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath, - *wtpath = path; - return 0; + static const char * const worktree_usage[] = { + N_("git worktree add [<options>] <path> [<commit-ish>]"), ++ N_("git worktree init-worktree-config"), + N_("git worktree list [<options>]"), + N_("git worktree lock [<options>] <path>"), + N_("git worktree move <worktree> <new-path>"), +@@ builtin/worktree.c: static int repair(int ac, const char **av, const char *prefix) + return rc; } + ++static int move_config_setting(const char *key, const char *value, ++ const char *from_file, const char *to_file) ++{ ++ if (git_config_set_in_file_gently(to_file, key, value)) ++ return error(_("unable to set %s in '%s'"), key, to_file); ++ if (git_config_set_in_file_gently(from_file, key, NULL)) ++ return error(_("unable to unset %s in '%s'"), key, from_file); ++ return 0; ++} + -+int upgrade_to_worktree_config(struct repository *r) ++static int init_worktree_config(int ac, const char **av, const char *prefix) +{ -+ int res; ++ struct repository *r = the_repository; ++ struct option options[] = { ++ OPT_END() ++ }; ++ int res = 0; + int bare = 0; + struct config_set cs = { 0 }; -+ char *base_config_file = xstrfmt("%s/config", r->commondir); -+ char *base_worktree_file = xstrfmt("%s/config.worktree", r->commondir); ++ const char *core_worktree; ++ char *common_config_file = xstrfmt("%s/config", r->commondir); ++ char *main_worktree_file = xstrfmt("%s/config.worktree", r->commondir); ++ ++ /* Report error on any arguments */ ++ ac = parse_options(ac, av, prefix, options, worktree_usage, 0); ++ if (ac) ++ usage_with_options(worktree_usage, options); + + git_configset_init(&cs); -+ git_configset_add_file(&cs, base_config_file); ++ git_configset_add_file(&cs, common_config_file); + + /* -+ * If the base repository is bare, then we need to move core.bare=true -+ * out of the base config file and into the base repository's -+ * config.worktree file. ++ * If the format and extension are already enabled, then we can ++ * skip the upgrade process. + */ -+ if (!git_configset_get_bool(&cs, "core.bare", &bare) && bare) { -+ if ((res = git_config_set_in_file_gently(base_worktree_file, -+ "core.bare", "true"))) { -+ error(_("unable to set core.bare=true in '%s'"), base_worktree_file); -+ goto cleanup; -+ } ++ if (repository_format_worktree_config) ++ return 0; + -+ if ((res = git_config_set_in_file_gently(base_config_file, -+ "core.bare", NULL))) { -+ error(_("unable to unset core.bare=true in '%s'"), base_config_file); -+ goto cleanup; -+ } -+ } + if (upgrade_repository_format(r, 1) < 0) { + res = error(_("unable to upgrade repository format to enable worktreeConfig")); + goto cleanup; @@ worktree.c: int should_prune_worktree(const char *id, struct strbuf *reason, cha + goto cleanup; + } + ++ /* ++ * If core.bare is true in the common config file, then we need to ++ * move it to the base worktree's config file or it will break all ++ * worktrees. If it is false, then leave it in place because it ++ * _could_ be negating a global core.bare=true. ++ */ ++ if (!git_configset_get_bool(&cs, "core.bare", &bare) && bare) { ++ if ((res = move_config_setting("core.bare", "true", ++ common_config_file, ++ main_worktree_file))) ++ goto cleanup; ++ } ++ /* ++ * If core.worktree is set, then the base worktree is located ++ * somewhere different than the parent of the common Git dir. ++ * Relocate that value to avoid breaking all worktrees with this ++ * upgrade to worktree config. ++ */ ++ if (!git_configset_get_string_tmp(&cs, "core.worktree", &core_worktree)) { ++ if ((res = move_config_setting("core.worktree", core_worktree, ++ common_config_file, ++ main_worktree_file))) ++ goto cleanup; ++ } ++ +cleanup: + git_configset_clear(&cs); -+ free(base_config_file); -+ free(base_worktree_file); -+ trace2_printf("returning %d", res); ++ free(common_config_file); ++ free(main_worktree_file); + return res; +} ++ + int cmd_worktree(int ac, const char **av, const char *prefix) + { + struct option options[] = { +@@ builtin/worktree.c: int cmd_worktree(int ac, const char **av, const char *prefix) + prefix = ""; + if (!strcmp(av[1], "add")) + return add(ac - 1, av + 1, prefix); ++ if (!strcmp(av[1], "init-worktree-config")) ++ return init_worktree_config(ac - 1, av + 1, prefix); + if (!strcmp(av[1], "prune")) + return prune(ac - 1, av + 1, prefix); + if (!strcmp(av[1], "list")) - ## worktree.h ## -@@ worktree.h: void strbuf_worktree_ref(const struct worktree *wt, - struct strbuf *sb, - const char *refname); - -+/** -+ * Upgrade the config of the current repository and its base (if different -+ * from this repository) to use worktree-config. This might adjust config -+ * in both repositories, including: -+ * -+ * 1. Upgrading the repository format version to 1. -+ * 2. Adding extensions.worktreeConfig to the base config file. -+ * 3. Moving core.bare=true from the base config file to the base -+ * repository's config.worktree file. -+ */ -+int upgrade_to_worktree_config(struct repository *r); -+ - #endif + ## t/t2407-worktree-init-worktree-config.sh (new) ## +@@ ++#!/bin/sh ++ ++test_description='test git worktree init-worktree-config' ++ ++. ./test-lib.sh ++ ++test_expect_success setup ' ++ git init base && ++ test_commit -C base commit && ++ git -C base worktree add --detach worktree ++' ++ ++reset_config_when_finished () { ++ test_when_finished git -C base config --unset core.repositoryFormatVersion && ++ test_when_finished git -C base config --unset extensions.worktreeConfig && ++ rm -rf base/.git/config.worktree && ++ rm -rf base/.git/worktrees/worktree/config.worktree ++} ++ ++test_expect_success 'upgrades repo format and adds extension' ' ++ reset_config_when_finished && ++ git -C base worktree init-worktree-config >out 2>err && ++ test_must_be_empty out && ++ test_must_be_empty err && ++ test_cmp_config -C base 1 core.repositoryFormatVersion && ++ test_cmp_config -C base true extensions.worktreeConfig ++' ++ ++test_expect_success 'relocates core.worktree' ' ++ reset_config_when_finished && ++ mkdir dir && ++ git -C base config core.worktree ../../dir && ++ git -C base worktree init-worktree-config >out 2>err && ++ test_must_be_empty out && ++ test_must_be_empty err && ++ test_cmp_config -C base 1 core.repositoryFormatVersion && ++ test_cmp_config -C base true extensions.worktreeConfig && ++ test_cmp_config -C base ../../dir core.worktree && ++ test_must_fail git -C worktree core.worktree ++' ++ ++test_expect_success 'relocates core.bare' ' ++ reset_config_when_finished && ++ git -C base config core.bare true && ++ git -C base worktree init-worktree-config >out 2>err && ++ test_must_be_empty out && ++ test_must_be_empty err && ++ test_cmp_config -C base 1 core.repositoryFormatVersion && ++ test_cmp_config -C base true extensions.worktreeConfig && ++ test_cmp_config -C base true core.bare && ++ test_must_fail git -C worktree core.bare ++' ++ ++test_expect_success 'skips upgrade is already upgraded' ' ++ reset_config_when_finished && ++ git -C base worktree init-worktree-config && ++ git -C base config core.bare true && ++ ++ # this should be a no-op, even though core.bare ++ # makes the worktree be broken. ++ git -C base worktree init-worktree-config >out 2>err && ++ test_must_be_empty out && ++ test_must_be_empty err && ++ test_must_fail git -C base config --worktree core.bare && ++ git -C base config core.bare ++' ++ ++test_done 4: 22896e9bb04 ! 4: 45316cd01c9 config: add repo_config_set_worktree_gently() @@ Metadata ## Commit message ## config: add repo_config_set_worktree_gently() - The previous change added upgrade_to_worktree_config() to assist - creating a worktree-specific config for the first time. However, this - requires every config writer to care about that upgrade before writing - to the worktree-specific config. In addition, callers need to know how - to generate the name of the config.worktree file and pass it to the - config API. + Some config settings, such as those for sparse-checkout, are likely + intended to only apply to one worktree at a time. To make this write + easier, add a new config API method, repo_config_set_worktree_gently(). - To assist, create a new repo_config_set_worktree_gently() method in the - config API that handles the upgrade_to_worktree_config() method in - addition to assigning the value in the worktree-specific config. This - will be consumed by an upcoming change. + This method will attempt to write to the worktree-specific config, but + will instead write to the common config file if worktree config is not + enabled. The next change will introduce a consumer of this method. Signed-off-by: Derrick Stolee <dstolee@microsoft.com> @@ config.c: int git_config_set_gently(const char *key, const char *value) +int repo_config_set_worktree_gently(struct repository *r, + const char *key, const char *value) +{ -+ return upgrade_to_worktree_config(r) || -+ git_config_set_multivar_in_file_gently( -+ repo_git_path(r, "config.worktree"), -+ key, value, NULL, 0); ++ /* Only use worktree-specific config if it is is already enabled. */ ++ if (repository_format_worktree_config) { ++ char *file = repo_git_path(r, "config.worktree"); ++ int ret = git_config_set_multivar_in_file_gently( ++ file, key, value, NULL, 0); ++ free(file); ++ return ret; ++ } ++ return repo_config_set_gently(r, key, value); +} + void git_config_set(const char *key, const char *value) { repo_config_set(the_repository, key, value); +@@ config.c: int repo_config_set_multivar_gently(struct repository *r, const char *key, + flags); + } + ++int repo_config_set_gently(struct repository *r, ++ const char *key, const char *value) ++{ ++ return repo_config_set_multivar_gently(r, key, value, NULL, 0); ++} ++ + void git_config_set_multivar(const char *key, const char *value, + const char *value_pattern, unsigned flags) + { ## config.h ## @@ config.h: void git_config_set_in_file(const char *, const char *, const char *); @@ config.h: void git_config_set_in_file(const char *, const char *, const char *); int git_config_set_gently(const char *, const char *); +/** -+ * Write a config value into the config.worktree file for the current -+ * worktree. This will initialize extensions.worktreeConfig if necessary, -+ * which might trigger some changes to the root repository's config file. ++ * Write a config value that should apply to the current worktree. If ++ * extensions.worktreeConfig is enabled, then the write will happen in the ++ * current worktree's config. Otherwise, write to the common config file. + */ +int repo_config_set_worktree_gently(struct repository *, const char *, const char *); + /** * write config values to `.git/config`, takes a key/value pair as parameter. */ +@@ config.h: int git_config_set_multivar_gently(const char *, const char *, const char *, uns + void git_config_set_multivar(const char *, const char *, const char *, unsigned); + int repo_config_set_multivar_gently(struct repository *, const char *, const char *, const char *, unsigned); + void repo_config_set_multivar(struct repository *, const char *, const char *, const char *, unsigned); ++int repo_config_set_gently(struct repository *, const char *, const char *); + int git_config_set_multivar_in_file_gently(const char *, const char *, const char *, const char *, unsigned); + + /** 5: 06457fafa78 ! 5: b200819c1bb sparse-checkout: use repo_config_set_worktree_gently() @@ Commit message sparse-checkout: use repo_config_set_worktree_gently() The previous change added repo_config_set_worktree_gently() to assist - writing config values into the worktree.config file, especially when - that may not have been initialized. + writing config values into the worktree.config file, if enabled. - When the base repo is bare, running 'git sparse-checkout init' in a - worktree will create the config.worktree file for the worktree, but that - will start causing the worktree to parse the bare repo's core.bare=true - value and start treating the worktree as bare. This causes more problems - as other commands are run in that worktree. + Let the sparse-checkout builtin use this helper instead of attempting to + initialize the worktree config on its own. This changes behavior of 'git + sparse-checkout set' in a few important ways: - The fix is to have this assignment into config.worktree be handled by - the repo_config_set_worktree_gently() helper. + 1. Git will no longer upgrade the repository format and add the + worktree config extension. The user should run 'git worktree + init-worktree-config' to enable this feature. + + 2. If worktree config is disabled, then this command will set the + core.sparseCheckout (and possibly core.sparseCheckoutCone and + index.sparse) values in the common config file. + + 3. If the main worktree is bare, then this command will not put the + worktree in a broken state. + + The main reason to use worktree-specific config for the sparse-checkout + builtin was to avoid enabling sparse-checkout patterns in one and + causing a loss of files in another. If a worktree does not have a + sparse-checkout patterns file, then the sparse-checkout logic will not + kick in on that worktree. + + This new logic introduces a new user pattern that could lead to some + confusion. Suppose a user has not upgraded to worktree config and + follows these steps in order: + + 1. Enable sparse-checkout in a worktree. + + 2. Disable sparse-checkout in that worktree without deleting that + worktree's sparse-checkout file. + + 3. Enable sparse-checkout in another worktree. + + 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 + upgrade to worktree specific config on purpose. Users already in this + state used the sparse-checkout builtin with a version that upgraded to + worktree config, anyway. Reported-by: Sean Allred <allred.sean@gmail.com> Helped-by: Eric Sunshine <sunshine@sunshineco.com> @@ sparse-index.c: static int convert_to_sparse_rec(struct index_state *istate, return res; ## t/t1091-sparse-checkout-builtin.sh ## -@@ t/t1091-sparse-checkout-builtin.sh: test_expect_success 'git sparse-checkout init' ' - check_files repo a +@@ t/t1091-sparse-checkout-builtin.sh: test_expect_success 'interaction with clone --no-checkout (unborn index)' ' ' -+test_expect_success 'init in a worktree of a bare repo' ' -+ test_when_finished rm -rf bare worktree && -+ git clone --bare repo bare && -+ git -C bare worktree add ../worktree && -+ ( -+ cd worktree && -+ git sparse-checkout init && -+ test_must_fail git config core.bare && -+ git sparse-checkout set /* + test_expect_success 'set enables config' ' +- git init empty-config && ++ git init initial-config && + ( +- cd empty-config && ++ cd initial-config && ++ test_commit file file && ++ mkdir dir && ++ test_commit dir dir/file && ++ git worktree add --detach ../initial-worktree && ++ git sparse-checkout set --cone + ) && -+ git -C bare config --list --show-origin >actual && -+ grep "file:config.worktree core.bare=true" actual ++ test_cmp_config -C initial-config true core.sparseCheckout && ++ test_cmp_config -C initial-worktree true core.sparseCheckout && ++ test_cmp_config -C initial-config true core.sparseCheckoutCone && ++ test_cmp_config -C initial-worktree true core.sparseCheckoutCone && ++ ++ # initial-config has a sparse-checkout file ++ # that only contains files at root. ++ ls initial-config >only-file && ++ cat >expect <<-EOF && ++ file ++ EOF ++ test_cmp expect only-file && ++ ++ # initial-worktree does not have its own sparse-checkout ++ # file, so the repply does not modify the worktree at all. ++ git -C initial-worktree sparse-checkout reapply && ++ ls initial-worktree >all && ++ cat >expect <<-EOF && ++ dir ++ file ++ EOF ++ test_cmp expect all +' + - test_expect_success 'git sparse-checkout list after init' ' - git -C repo sparse-checkout list >actual && - cat >expect <<-\EOF && ++test_expect_success 'set enables worktree config, if enabled' ' ++ git init worktree-config && ++ ( ++ cd worktree-config && + test_commit test file && +- test_path_is_missing .git/config.worktree && +- git sparse-checkout set nothing && +- test_path_is_file .git/config.worktree && +- test_cmp_config true core.sparseCheckout +- ) ++ git worktree add --detach ../worktree-config2 && ++ git worktree init-worktree-config && ++ git sparse-checkout set --cone && ++ git config --worktree core.sparseCheckout && ++ git config --worktree core.sparseCheckoutCone ++ ) && ++ test_cmp_config -C worktree-config true core.sparseCheckout && ++ test_must_fail git -C worktree-config2 core.sparseCheckout && ++ test_cmp_config -C worktree-config true core.sparseCheckoutCone && ++ test_must_fail git -C worktree-config2 core.sparseCheckoutCone + ' + + test_expect_success 'set sparse-checkout using builtin' ' +@@ t/t1091-sparse-checkout-builtin.sh: test_expect_success 'add to sparse-checkout' ' + ' + + test_expect_success 'cone mode: match patterns' ' ++ git -C repo worktree init-worktree-config && + git -C repo config --worktree core.sparseCheckoutCone true && + rm -rf repo/a repo/folder1 repo/folder2 && + git -C repo read-tree -mu HEAD 2>err && @@ t/t1091-sparse-checkout-builtin.sh: test_expect_success 'sparse-index enabled and disabled' ' test-tool -C repo read-cache --table >cache && ! grep " tree " cache && @@ t/t1091-sparse-checkout-builtin.sh: test_expect_success 'sparse-index enabled an ) ' +@@ t/t1091-sparse-checkout-builtin.sh: test_expect_success 'fail when lock is taken' ' + ' + + test_expect_success '.gitignore should not warn about cone mode' ' ++ git -C repo worktree init-worktree-config && + git -C repo config --worktree core.sparseCheckoutCone true && + echo "**/bin/*" >repo/.gitignore && + git -C repo reset --hard 2>err && -: ----------- > 6: fcece09546c worktree: copy sparse-checkout patterns and config on add