[PATCH 0/4] Sparse checkout: fix bug with worktree of bare repo

STALE1648d

Revision v1 of 6 in this series.

121 messages, 8 authors, 2022-02-16 · page 2 of 2 · open the first message on its own page

Re: [PATCH v4 1/5] Documentation: add extensions.worktreeConfig details

From: Elijah Newren <hidden>
Date: 2022-01-27 06:44:06

On Tue, Jan 25, 2022 at 10:42 AM Derrick Stolee via GitGitGadget
[off-list ref] wrote:

quoted hunk

From: Derrick Stolee <redacted>

The extensions.worktreeConfig extension was added in 58b284a (worktree:
add per-worktree config files, 2018-10-21) and was somewhat documented
in Documentation/git-config.txt. However, the extensions.worktreeConfig
value was not specified further in the list of possible config keys. The
location of the config.worktree file is not specified, and there are
some precautions that should be mentioned clearly, but are only
mentioned in git-worktree.txt.

Expand the documentation to help users discover the complexities of
extensions.worktreeConfig by adding details and cross links in these
locations (relative to Documentation/):

- config/extensions.txt
- git-config.txt
- git-worktree.txt

The updates focus on items such as

* $GIT_DIR/config.worktree takes precedence over $GIT_COMMON_DIR/config.

* The core.worktree and core.bare=true settings are incorrect to have in
  the common config file when extensions.worktreeConfig is enabled.

* The sparse-checkout settings core.sparseCheckout[Cone] are recommended
  to be set in the worktree config.

As documented in 11664196ac ("Revert "check_repository_format_gently():
refuse extensions for old repositories"", 2020-07-15), this extension
must be considered regardless of the repository format version for
historical reasons.

A future change will update references to extensions.worktreeConfig
within git-sparse-checkout.txt, but a behavior change is needed before
making those updates.

Signed-off-by: Derrick Stolee <redacted>
---
 Documentation/config/extensions.txt | 31 +++++++++++++++++++++++++++++
 Documentation/git-config.txt        |  8 ++++++--
 Documentation/git-worktree.txt      | 11 +++++++---
 3 files changed, 45 insertions(+), 5 deletions(-)

diff --git a/Documentation/config/extensions.txt b/Documentation/config/extensions.txt
index 4e23d73cdca..5999dcb2a1f 100644
--- a/Documentation/config/extensions.txt
+++ b/Documentation/config/extensions.txt

@@ -6,3 +6,34 @@ extensions.objectFormat::
 Note that this setting should only be set by linkgit:git-init[1] or
 linkgit:git-clone[1].  Trying to change it after initialization will not
 work and will produce hard-to-diagnose issues.
+
+extensions.worktreeConfig::
+       If enabled, then worktrees will load config settings from the
+       `$GIT_DIR/config.worktree` file in addition to the
+       `$GIT_COMMON_DIR/config` file. Note that `$GIT_COMMON_DIR` and
+       `$GIT_DIR` are the same for the main worktree, while other
+       worktrees have `$GIT_DIR` equal to
+       `$GIT_COMMON_DIR/worktrees/<worktree-name>/`. The settings in the
+       `config.worktree` file will override settings from any other
+       config files.
++
+When enabling `extensions.worktreeConfig`, you must be careful to move
+certain values from the common config file to the main worktree's
+`config.worktree` file, if present:
++
+* `core.worktree` must be moved from `$GIT_COMMON_DIR/config` to
+  `$GIT_COMMON_DIR/config.worktree`.
+* If `core.bare` is true, then it must be moved from `$GIT_COMMON_DIR/config`
+  to `$GIT_COMMON_DIR/config.worktree`.
++
+It may also be beneficial to adjust the locations of `core.sparseCheckout`
+and `core.sparseCheckoutCone` depending on your desire for customizable
+sparse-checkout settings for each worktree. By default, the `git
+sparse-checkout` builtin enables `extensions.worktreeConfig`, assigns
+these config values on a per-worktree basis, and uses the
+`$GIT_DIR/info/sparse-checkout` file to specify the sparsity for each
+worktree independently. See linkgit:git-sparse-checkout[1] for more
+details.
++
+For historical reasons, `extensions.worktreeConfig` is respected
+regardless of the `core.repositoryFormatVersion` setting.

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 2285effb363..95cefd5e399 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt

@@ -141,9 +141,13 @@ from all available files.
 See also <<FILES>>.

 --worktree::
-       Similar to `--local` except that `.git/config.worktree` is
+       Similar to `--local` except that `$GIT_DIR/config.worktree` is
        read from or written to if `extensions.worktreeConfig` is
-       present. If not it's the same as `--local`.
+       enabled. If not it's the same as `--local`. Note that `$GIT_DIR`
+       is equal to `$GIT_COMMON_DIR` for the main worktree, but is of the
+       form `.git/worktrees/<worktree-name>/` for other worktrees. See

is of the form `$GIT_DIR/worktrees/<worktree-name>/`; .git isn't even
a directory in other worktrees.

quoted hunk

+       linkgit:git-worktree[1] to learn how to enable
+       `extensions.worktreeConfig`.

 -f <config-file>::
 --file <config-file>::

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 9e862fbcf79..ea0ee9f8bb5 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt

@@ -286,8 +286,8 @@ CONFIGURATION FILE
 ------------------
 By default, the repository `config` file is shared across all working
 trees. If the config variables `core.bare` or `core.worktree` are
-already present in the config file, they will be applied to the main
-working trees only.
+present in the common config file and `extensions.worktreeConfig` is
+disabled, then they will be applied to the main working trees only.

"main working trees"?  Is that an accidental plural?

quoted hunk

 In order to have configuration specific to working trees, you can turn
 on the `worktreeConfig` extension, e.g.:

@@ -307,11 +307,16 @@ 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:

- - `core.worktree` and `core.bare` should never be shared
+ - `core.worktree` should never be shared.
+
+ - `core.bare` should not be shared unless the value is `core.bare=false`.

The double negative makes for harder parsing.  Perhaps
    - `core.bare` should not be shared if the value is `core.bare=true`
?

  - `core.sparseCheckout` is recommended per working tree, unless you
    are sure you always use sparse checkout for all working trees.

+See the documentation of `extensions.worktreeConfig` in
+linkgit:git-config[1] for more details.
+
 DETAILS
 -------
 Each linked working tree has a private sub-directory in the repository's
--
gitgitgadget

Thanks for documenting these details; I had some very minor comments
but this is well written and very helpful.

Re: [PATCH v4 2/5] worktree: create init_worktree_config()

From: Elijah Newren <hidden>
Date: 2022-01-27 07:02:14

On Tue, Jan 25, 2022 at 10:42 AM Derrick Stolee via GitGitGadget
[off-list ref] wrote:

From: Derrick Stolee <redacted>

Upgrading a repository to use extensions.worktreeConfig is non-trivial.
There are several steps involved, including moving some config settings
from the common config file to the main worktree's config.worktree file.
The previous change updated the documentation with all of these details.

Commands such as 'git sparse-checkout set' upgrade the repository to use
extensions.worktreeConfig without following these steps, causing some
user pain in some special cases.

Create a helper method, init_worktree_config(), that will be used in a
later change to fix this behavior within 'git sparse-checkout set'. The
method is carefully documented in worktree.h.

I was curious why you were only fixing `set` and not `init`, but I
looked ahead and it appears you are fixing both, since both use
set_config().  And I can see leaving out the mention of `init` since
it's deprecated.  Anyway, it's all good here, I'm basically just
thinking out loud...

quoted hunk

Note that we do _not_ upgrade the repository format version to 1 during
this process. The worktree config extension must be considered by Git
and third-party tools even if core.repositoryFormatVersion is 0 for
historical reasons documented in 11664196ac ("Revert
"check_repository_format_gently(): refuse extensions for old
repositories"", 2020-07-15). This is a special case for this extension,
and newer extensions (such as extensions.objectFormat) still need to
upgrade the repository format version.

Signed-off-by: Derrick Stolee <redacted>
---
 worktree.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 worktree.h | 19 +++++++++++++++
 2 files changed, 89 insertions(+)

diff --git a/worktree.c b/worktree.c
index 6f598dcfcdf..dc4ead4c8fb 100644
--- a/worktree.c
+++ b/worktree.c

@@ -5,6 +5,7 @@
 #include "worktree.h"
 #include "dir.h"
 #include "wt-status.h"
+#include "config.h"

 void free_worktrees(struct worktree **worktrees)
 {

@@ -826,3 +827,72 @@ int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath,
        *wtpath = path;
        return 0;
 }
+
+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 init_worktree_config(struct repository *r)
+{
+       int res = 0;
+       int bare = 0;
+       struct config_set cs = { { 0 } };
+       const char *core_worktree;
+       char *common_config_file = xstrfmt("%s/config", r->commondir);
+       char *main_worktree_file = xstrfmt("%s/config.worktree", r->commondir);
+
+       /*
+        * If the extension is already enabled, then we can skip the
+        * upgrade process.
+        */
+       if (repository_format_worktree_config)
+               return 0;
+       if ((res = git_config_set_gently("extensions.worktreeConfig", "true")))
+               return error(_("failed to set extensions.worktreeConfig setting"));
+
+       git_configset_init(&cs);
+       git_configset_add_file(&cs, common_config_file);
+
+       /*
+        * 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;
+       }
+
+       /*
+        * Ensure that we use worktree config for the remaining lifetime
+        * of the current process.
+        */
+       repository_format_worktree_config = 1;
+
+cleanup:
+       git_configset_clear(&cs);
+       free(common_config_file);
+       free(main_worktree_file);
+       return res;
+}

diff --git a/worktree.h b/worktree.h
index 9e06fcbdf3d..5ea5fcc3647 100644
--- a/worktree.h
+++ b/worktree.h

@@ -183,4 +183,23 @@ void strbuf_worktree_ref(const struct worktree *wt,
                         struct strbuf *sb,
                         const char *refname);

+/**
+ * Enable worktree config for the first time. This will make the following
+ * adjustments:
+ *
+ * 1. Add extensions.worktreeConfig=true in the common config file.
+ *
+ * 2. If the common config file has a core.worktree value or core.bare is
+ *    set to true, then those values are moved to the main worktree's
+ *    config.worktree file.

This is a bit ambiguous.  If core.worktree is set and core.bare is
false, are both moved or only one?  I'm afraid folks won't understand
that it's just one from this description.

+ *
+ * If extensions.worktreeConfig is already true, then this method
+ * terminates early without any of the above steps. The existing config
+ * arrangement is assumed to be intentional.
+ *
+ * Returns 0 on success. Reports an error message and returns non-zero
+ * if any of these steps fail.
+ */
+int init_worktree_config(struct repository *r);
+
 #endif
--

Other than the ambiguity in worktree.h, this patch looks solid.

Re: [PATCH v4 5/5] worktree: copy sparse-checkout patterns and config on add

From: Elijah Newren <hidden>
Date: 2022-01-27 07:09:53

On Tue, Jan 25, 2022 at 10:42 AM Derrick Stolee via GitGitGadget
[off-list ref] wrote:

quoted hunk

From: Derrick Stolee <redacted>

When adding a new worktree, it is reasonable to expect that we want to
use the current set of sparse-checkout settings for that new worktree.
This is particularly important for repositories where the worktree would
become too large to be useful. This is even more important when using
partial clone as well, since we want to avoid downloading the missing
blobs for files that should not be written to the new worktree.

The only way to create such a worktree without this intermediate step of
expanding the full worktree is to copy the sparse-checkout patterns and
config settings during 'git worktree add'. Each worktree has its own
sparse-checkout patterns, and the default behavior when the
sparse-checkout file is missing is to include all paths at HEAD. Thus,
we need to have patterns from somewhere, they might as well be the
current worktree's patterns. These are then modified independently in
the future.

In addition to the sparse-checkout file, copy the worktree config file
if worktree config is enabled and the file exists. This will copy over
any important settings to ensure the new worktree behaves the same as
the current one. The only exception we must continue to make is that
core.bare and core.worktree should become unset in the worktree's config
file.

Signed-off-by: Derrick Stolee <redacted>
---
 builtin/worktree.c                 | 60 ++++++++++++++++++++++++++++++
 t/t1091-sparse-checkout-builtin.sh | 31 +++++++++++----
 t/t2400-worktree-add.sh            | 46 ++++++++++++++++++++++-
 3 files changed, 127 insertions(+), 10 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 2838254f7f2..dc9cd6decc8 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c

@@ -335,6 +335,66 @@ static int add_worktree(const char *path, const char *refname,
        strbuf_addf(&sb, "%s/commondir", sb_repo.buf);
        write_file(sb.buf, "../..");

+       /*
+        * If the current worktree has sparse-checkout enabled, then copy
+        * the sparse-checkout patterns from the current worktree.
+        */
+       if (core_apply_sparse_checkout) {
+               char *from_file = git_pathdup("info/sparse-checkout");
+               char *to_file = xstrfmt("%s/worktrees/%s/info/sparse-checkout",
+                                       realpath.buf, name);
+
+               if (file_exists(from_file)) {
+                       if (safe_create_leading_directories(to_file) ||
+                           copy_file(to_file, from_file, 0666))
+                               error(_("failed to copy '%s' to '%s'; sparse-checkout may not work correctly"),
+                                     from_file, to_file);
+               }
+
+               free(from_file);
+               free(to_file);
+       }
+
+       /*
+        * If we are using worktree config, then copy all current config
+        * values from the current worktree into the new one, that way the
+        * new worktree behaves the same as this one.
+        */
+       if (repository_format_worktree_config) {
+               char *from_file = git_pathdup("config.worktree");
+               char *to_file = xstrfmt("%s/worktrees/%s/config.worktree",
+                                       realpath.buf, name);
+
+               if (file_exists(from_file)) {
+                       struct config_set cs = { { 0 }};
+                       const char *str_value;
+                       int bool_value;
+
+                       if (safe_create_leading_directories(to_file) ||
+                           copy_file(to_file, from_file, 0666))
+                               die(_("failed to copy worktree config from '%s' to '%s'"),
+                                   from_file, to_file);
+
+                       git_configset_init(&cs);
+                       git_configset_add_file(&cs, from_file);
+
+                       if (!git_configset_get_bool(&cs, "core.bare", &bool_value) &&
+                           bool_value &&
+                           git_config_set_multivar_in_file_gently(
+                                       to_file, "core.bare", NULL, "true", 0))
+                               error(_("failed to unset 'core.bare' in '%s'"), to_file);
+                       if (!git_configset_get_value(&cs, "core.worktree", &str_value) &&
+                           git_config_set_in_file_gently(to_file,
+                                                         "core.worktree", NULL))
+                               error(_("failed to unset 'core.worktree' in '%s'"), to_file);
+
+                       git_configset_clear(&cs);
+               }
+
+               free(from_file);
+               free(to_file);
+       }
+
        strvec_pushf(&child_env, "%s=%s", GIT_DIR_ENVIRONMENT, sb_git.buf);
        strvec_pushf(&child_env, "%s=%s", GIT_WORK_TREE_ENVIRONMENT, path);
        cp.git_cmd = 1;

diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index be6ea4ffe33..d929772be96 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh

@@ -146,9 +146,9 @@ test_expect_success 'interaction with clone --no-checkout (unborn index)' '
 '

 test_expect_success 'set enables config' '
-       git init empty-config &&
+       git init worktree-config &&
        (
-               cd empty-config &&
+               cd worktree-config &&
                test_commit test file &&
                test_path_is_missing .git/config.worktree &&
                git sparse-checkout set nothing &&

@@ -201,6 +201,21 @@ test_expect_success 'add to sparse-checkout' '
        check_files repo "a folder1 folder2"
 '

+test_expect_success 'worktree: add copies sparse-checkout patterns' '
+       cat repo/.git/info/sparse-checkout >old &&
+       test_when_finished cp old repo/.git/info/sparse-checkout &&
+       test_when_finished git -C repo worktree remove ../worktree &&
+       git -C repo sparse-checkout set "/*" &&

Could we add --no-cone to tests using that mode in anticipation of
switching the default?

quoted hunk

+       git -C repo worktree add --quiet ../worktree 2>err &&
+       test_must_be_empty err &&
+       new=repo/.git/worktrees/worktree/info/sparse-checkout &&
+       test_path_is_file $new &&
+       test_cmp repo/.git/info/sparse-checkout $new &&
+       git -C worktree sparse-checkout set --cone &&
+       test_cmp_config -C worktree true core.sparseCheckoutCone &&
+       test_must_fail git -C repo core.sparseCheckoutCone
+'
+
 test_expect_success 'cone mode: match patterns' '
        git -C repo config --worktree core.sparseCheckoutCone true &&
        rm -rf repo/a repo/folder1 repo/folder2 &&

@@ -520,13 +535,13 @@ test_expect_success 'interaction with submodules' '
 '

 test_expect_success 'different sparse-checkouts with worktrees' '
+       git -C repo sparse-checkout set --cone deep folder1 &&
        git -C repo worktree add --detach ../worktree &&
-       check_files worktree "a deep folder1 folder2" &&
-       git -C worktree sparse-checkout init --cone &&
-       git -C repo sparse-checkout set folder1 &&
-       git -C worktree sparse-checkout set deep/deeper1 &&
-       check_files repo a folder1 &&
-       check_files worktree a deep
+       check_files worktree "a deep folder1" &&
+       git -C repo sparse-checkout set --cone folder1 &&
+       git -C worktree sparse-checkout set --cone deep/deeper1 &&
+       check_files repo "a folder1" &&
+       check_files worktree "a deep"
 '

 test_expect_success 'set using filename keeps file on-disk' '

diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
index 37ad79470fb..3fb5b21b943 100755
--- a/t/t2400-worktree-add.sh
+++ b/t/t2400-worktree-add.sh

@@ -165,8 +165,50 @@ test_expect_success '"add" default branch of a bare repo' '
        (
                git clone --bare . bare2 &&
                cd bare2 &&
-               git worktree add ../there3 main
-       )
+               git worktree add ../there3 main &&
+               cd ../there3 &&
+               git status
+       ) &&
+       cat >expect <<-EOF &&
+       init.t
+       EOF
+       ls there3 >actual &&
+       test_cmp expect actual
+'
+
+test_expect_success '"add" to bare repo with worktree config' '
+       (
+               git clone --bare . bare3 &&
+               cd bare3 &&
+               git config extensions.worktreeconfig true &&
+               git config --worktree core.bare true &&
+               git config --worktree core.worktree "$(pwd)" &&
+               git config --worktree bogus.key value &&
+               git config --unset core.bare &&
+               git worktree add ../there4 main &&
+               cd ../there4 &&
+               git status &&
+               git worktree add --detach ../there5 &&
+               cd ../there5 &&
+               git status
+       ) &&
+
+       # the worktree has the arbitrary value copied.
+       test_cmp_config -C there4 value bogus.key &&
+       test_cmp_config -C there5 value bogus.key &&
+
+       # however, core.bare and core.worktree were removed.
+       test_must_fail git -C there4 config core.bare &&
+       test_must_fail git -C there4 config core.worktree &&
+
+       cat >expect <<-EOF &&
+       init.t
+       EOF
+
+       ls there4 >actual &&
+       test_cmp expect actual &&
+       ls there5 >actual &&
+       test_cmp expect actual
 '

 test_expect_success 'checkout with grafts' '
--

gitgitgadget

This patch is so awesome.  Totally looking forward to seeing it
included.  I was only able to spot one micro nit.

Re: [PATCH v4 4/5] sparse-checkout: set worktree-config correctly

From: Elijah Newren <hidden>
Date: 2022-01-27 07:15:37

On Tue, Jan 25, 2022 at 10:42 AM Derrick Stolee via GitGitGadget
[off-list ref] wrote:

quoted hunk

From: Derrick Stolee <redacted>

The previous change added repo_config_set_worktree_gently() to assist
writing config values into the config.worktree file, if enabled. An
earlier change added init_worktree_config() as a helper to initialize
extensions.worktreeConfig if not already enabled.

Let the sparse-checkout builtin use these helpers instead of attempting to
initialize the worktree config on its own. This changes behavior of 'git
sparse-checkout set' in a few important ways:

 1. Git will no longer upgrade the repository format, since this is not
    a requirement for understanding extensions.worktreeConfig.

 2. 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.

Reported-by: Sean Allred <redacted>
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Derrick Stolee <redacted>
---
 Documentation/git-sparse-checkout.txt | 24 ++++++++++++++++-------
 builtin/sparse-checkout.c             | 28 +++++++++++++--------------
 sparse-index.c                        | 10 +++-------
 t/t1091-sparse-checkout-builtin.sh    |  4 ++--
 4 files changed, 35 insertions(+), 31 deletions(-)

diff --git a/Documentation/git-sparse-checkout.txt b/Documentation/git-sparse-checkout.txt
index b81dbe06543..c6eae3ec7fd 100644
--- a/Documentation/git-sparse-checkout.txt
+++ b/Documentation/git-sparse-checkout.txt

@@ -31,12 +31,20 @@ COMMANDS
        Describe the patterns in the sparse-checkout file.

 'set'::
-       Enable the necessary config settings
-       (extensions.worktreeConfig, core.sparseCheckout,
-       core.sparseCheckoutCone) if they are not already enabled, and
-       write a set of patterns to the sparse-checkout file from the
-       list of arguments following the 'set' subcommand. Update the
-       working directory to match the new patterns.
+       Enable the necessary sparse-checkout config settings
+       (`core.sparseCheckout` and possibly `core.sparseCheckoutCone`) if

and possibly index.sparse as well, right?

quoted hunk

+       they are not already enabled, and write a set of patterns to the
+       sparse-checkout file from the list of arguments following the
+       'set' subcommand. Update the working directory to match the new
+       patterns.
++
+To ensure that adjusting the sparse-checkout settings within a worktree
+does not alter the sparse-checkout settings in other worktrees, the 'set'
+subcommand will upgrade your repository config to use worktree-specific
+config if not already present. The sparsity defined by the arguments to
+the 'set' subcommand are stored in the worktree-specific sparse-checkout
+file. See linkgit:git-worktree[1] and the documentation of
+`extensions.worktreeConfig` in linkgit:git-config[1] for more details.
 +
 When the `--stdin` option is provided, the patterns are read from
 standard in as a newline-delimited list instead of from the arguments.

@@ -73,7 +81,9 @@ interact with your repository until it is disabled.
        By default, these patterns are read from the command-line arguments,
        but they can be read from stdin using the `--stdin` option. When
        `core.sparseCheckoutCone` is enabled, the given patterns are interpreted
-       as directory names as in the 'set' subcommand.
+       as directory names as in the 'set' subcommand. The sparsity defined
+       by the arguments to the 'add' subcommand are added to the patterns
+       in the worktree-specific sparse-checkout file.

This sentence addition makes your series conflict with patch 4 of my
en/present-despite-skipped series.

The sentence also seems somewhat redundant with the first sentence of
the paragraph (not quoted here).  Perhaps consider just dropping it to
make it easier for Junio to integrate?

quoted hunk

 'reapply'::
        Reapply the sparsity pattern rules to paths in the working tree.

diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index 679c1070368..314c8d61f80 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c

@@ -15,6 +15,7 @@
 #include "wt-status.h"
 #include "quote.h"
 #include "sparse-index.h"
+#include "worktree.h"

 static const char *empty_base = "";

@@ -359,26 +360,23 @@ enum sparse_checkout_mode {

 static int set_config(enum sparse_checkout_mode mode)
 {
-       const char *config_path;
-
-       if (upgrade_repository_format(1) < 0)
-               die(_("unable to upgrade repository format to enable worktreeConfig"));

Wait, this got added in mid-2020, since v2.28.0 -- and I missed it but
it hasn't bit us yet??

I'm so sorry.  Earlier when I objected to setting this, I was worried
it was a new change and would introduce some new failures, but clearly
it was already introduced...and it turns out we've been fine.  So, I
was wrong to worry about this.

(Which is to say, if you want to add it back, I'm fine with it given
this info.  If you'd rather still take it out, I'm fine with that
too.)

quoted hunk

-       if (git_config_set_gently("extensions.worktreeConfig", "true")) {
-               error(_("failed to set extensions.worktreeConfig setting"));
+       /* Update to use worktree config, if not already. */
+       if (init_worktree_config(the_repository)) {
+               error(_("failed to initialize worktree config"));
                return 1;
        }

-       config_path = git_path("config.worktree");
-       git_config_set_in_file_gently(config_path,
-                                     "core.sparseCheckout",
-                                     mode ? "true" : NULL);
-
-       git_config_set_in_file_gently(config_path,
-                                     "core.sparseCheckoutCone",
-                                     mode == MODE_CONE_PATTERNS ? "true" : NULL);
+       if (repo_config_set_worktree_gently(the_repository,
+                                           "core.sparseCheckout",
+                                           mode ? "true" : "false") ||
+           repo_config_set_worktree_gently(the_repository,
+                                           "core.sparseCheckoutCone",
+                                           mode == MODE_CONE_PATTERNS ?
+                                               "true" : "false"))
+               return 1;

        if (mode == MODE_NO_PATTERNS)
-               set_sparse_index_config(the_repository, 0);
+               return set_sparse_index_config(the_repository, 0);

        return 0;
 }

diff --git a/sparse-index.c b/sparse-index.c
index a1d505d50e9..e93609999e0 100644
--- a/sparse-index.c
+++ b/sparse-index.c

@@ -99,13 +99,9 @@ static int convert_to_sparse_rec(struct index_state *istate,

 int set_sparse_index_config(struct repository *repo, int enable)
 {
-       int res;
-       char *config_path = repo_git_path(repo, "config.worktree");
-       res = git_config_set_in_file_gently(config_path,
-                                           "index.sparse",
-                                           enable ? "true" : NULL);
-       free(config_path);
-
+       int res = repo_config_set_worktree_gently(repo,
+                                                 "index.sparse",
+                                                 enable ? "true" : "false");
        prepare_repo_settings(repo);
        repo->settings.sparse_index = enable;
        return res;

diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index 42776984fe7..be6ea4ffe33 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh

@@ -117,7 +117,7 @@ test_expect_success 'switching to cone mode with non-cone mode patterns' '
                cd bad-patterns &&
                git sparse-checkout init &&
                git sparse-checkout add dir &&
-               git config core.sparseCheckoutCone true &&
+               git config --worktree core.sparseCheckoutCone true &&
                test_must_fail git sparse-checkout add dir 2>err &&
                grep "existing sparse-checkout patterns do not use cone mode" err
        )

@@ -256,7 +256,7 @@ test_expect_success 'sparse-index enabled and disabled' '
                test_cmp expect actual &&

                git -C repo config --list >config &&
-               ! grep index.sparse config
+               test_cmp_config -C repo false index.sparse
        )
 '

--

gitgitgadget

Patch looks good; I only had a few very minor comments.

Re: [PATCH v4 0/5] Sparse checkout: fix bug with worktree of bare repo

From: Elijah Newren <hidden>
Date: 2022-01-27 07:20:44

On Tue, Jan 25, 2022 at 10:42 AM Derrick Stolee via GitGitGadget
[off-list ref] wrote:

This series is now based on v2.35.0 since that contains all of the necessary
topics.

Heads up for the maintainer: has a minor textual conflict with
en/present-despite-skipped (this series modifies the end of a
paragraph that the other series modifies the beginning of).

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 or core.worktree are set in the
common config file. This series fixes this, but also puts in place some
helpers to prevent this from happening in the future.

ATTENTION: I have significantly redesigned the series since previous
versions, so most of this cover letter is new.

 * Patch 1 updates documentation around extensions.worktreeConfig in a few
   places to improve discoverability. Several cross links are added to make
   it easy to find the related areas. (The documentation for the changes to
   'git sparse-checkout' are delayed to patch 4.)

 * Patch 2 introduces the init_worktree_config() helper which follows the
   documented instructions to enable extensions.worktreeConfig as well as
   move the core.bare and core.worktree config values. This update does not
   modify core.repositoryFormatVersion, since this is not needed
   specifically for extensions.worktreeConfig.

 * Patch 3 adds a new repo_config_set_worktree_gently() helper method so we
   can internally adjust a config value within a worktree, at least if
   extensions.worktreeConfig is enabled. (It will write to the common config
   file if the extension is not enabled.)

 * Patch 4 modifies the sparse-checkout builtin to use
   init_worktree_config() and repo_config_set_worktree_gently() in ways that
   fix the reported bug. The behavior change here is that it will no longer
   upgrade the repository format version, since that is not needed for
   extensions.worktreeConfig.

 * Patch 5 updates 'git worktree add' to copy the worktree config from the
   current worktree to the new one (while unsetting core.bare=true and
   core.worktree=*) along with copying the sparse-checkout patterns file.

[1]
https://lore.kernel.org/git/CABceR4bZmtC4rCwgxZ1BBYZP69VOUca1f_moJoP989vTUZWu9Q@mail.gmail.com/
[2]
https://lore.kernel.org/git/CAPig+cQ6U_yFw-X2OWrizB1rbCvc4bNxuSzKFzmoLNnm0GH8Eg@mail.gmail.com/


Updates in v4
=============

 * Rebased to v2.35.0
 * Fixed memory leak (was leaking repo_git_path() result)
 * Added additional documentation updates so curious users can discover the
   intricacies of extensions.worktreeConfig from multiple entry points.
 * Significantly reduced the amount of changes to config.c.
 * 'git sparse-checkout' no longer upgrades the repository format.
 * Dropped the update to upgrade_repository_format(), since it is not
   needed.
 * Dropped the 'git worktree init-worktree-config' subcommand in favor of a
   helper method called by 'git sparse-checkout'
 * Many others because of the significant changes required by the above
   items.

This series has become pretty solid.  I had only very minor comments
on the patches.  Thanks for working on this.

Thanks, -Stolee

Derrick Stolee (5):
  Documentation: add extensions.worktreeConfig details
  worktree: create init_worktree_config()
  config: add repo_config_set_worktree_gently()
  sparse-checkout: set worktree-config correctly
  worktree: copy sparse-checkout patterns and config on add

 Documentation/config/extensions.txt   | 31 ++++++++++++
 Documentation/git-config.txt          |  8 ++-
 Documentation/git-sparse-checkout.txt | 24 ++++++---
 Documentation/git-worktree.txt        | 11 +++--
 builtin/sparse-checkout.c             | 28 +++++------
 builtin/worktree.c                    | 60 +++++++++++++++++++++++
 config.c                              | 35 ++++++++++++--
 config.h                              |  8 +++
 sparse-index.c                        | 10 ++--
 t/t1091-sparse-checkout-builtin.sh    | 35 ++++++++++----
 t/t2400-worktree-add.sh               | 46 +++++++++++++++++-
 worktree.c                            | 70 +++++++++++++++++++++++++++
 worktree.h                            | 19 ++++++++
 13 files changed, 336 insertions(+), 49 deletions(-)


base-commit: 89bece5c8c96f0b962cfc89e63f82d603fd60bed
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1101%2Fderrickstolee%2Fsparse-checkout%2Fbare-worktree-bug-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1101/derrickstolee/sparse-checkout/bare-worktree-bug-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/1101

Range-diff vs v3:

 1:  749ba67d21e < -:  ----------- setup: use a repository when upgrading format
 2:  61b96937016 < -:  ----------- config: make some helpers repo-aware
 -:  ----------- > 1:  459e09dedd7 Documentation: add extensions.worktreeConfig details
 3:  e2a0a458115 ! 2:  d262a76b448 worktree: add 'init-worktree-config' subcommand
     @@ Metadata
      Author: Derrick Stolee [off-list ref]

       ## Commit message ##
     -    worktree: add 'init-worktree-config' subcommand
     +    worktree: create init_worktree_config()

     -    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, 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.
     +    Upgrading a repository to use extensions.worktreeConfig is non-trivial.
     +    There are several steps involved, including moving some config settings
     +    from the common config file to the main worktree's config.worktree file.
     +    The previous change updated the documentation with all of these details.

     -    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.
     +    Commands such as 'git sparse-checkout set' upgrade the repository to use
     +    extensions.worktreeConfig without following these steps, causing some
     +    user pain in some special cases.

     -    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.
     +    Create a helper method, init_worktree_config(), that will be used in a
     +    later change to fix this behavior within 'git sparse-checkout set'. The
     +    method is carefully documented in worktree.h.

     -    To help resolve this transition, create the 'git worktree
     -    init-worktree-config' helper. This new subcommand does the following:
     +    Note that we do _not_ upgrade the repository format version to 1 during
     +    this process. The worktree config extension must be considered by Git
     +    and third-party tools even if core.repositoryFormatVersion is 0 for
     +    historical reasons documented in 11664196ac ("Revert
     +    "check_repository_format_gently(): refuse extensions for old
     +    repositories"", 2020-07-15). This is a special case for this extension,
     +    and newer extensions (such as extensions.objectFormat) still need to
     +    upgrade the repository format version.

     -     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
     -    was a submodule instead of a worktree, then this still applies
     -    correctly.
     -
     -    Helped-by: Eric Sunshine [off-list ref]
          Signed-off-by: Derrick Stolee [off-list ref]

     - ## 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 ##
     + ## worktree.c ##
      @@
     + #include "worktree.h"
     + #include "dir.h"
     + #include "wt-status.h"
     ++#include "config.h"

     - 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;
     + 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 int move_config_setting(const char *key, const char *value,
      +                        const char *from_file, const char *to_file)
      +{
     @@ builtin/worktree.c: static int repair(int ac, const char **av, const char *prefi
      + return 0;
      +}
      +
     -+static int init_worktree_config(int ac, const char **av, const char *prefix)
     ++int init_worktree_config(struct repository *r)
      +{
     -+ struct repository *r = the_repository;
     -+ struct option options[] = {
     -+         OPT_END()
     -+ };
      + int res = 0;
      + int bare = 0;
     -+ struct config_set cs = { 0 };
     ++ struct config_set cs = { { 0 } };
      + 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, common_config_file);
     -+
      + /*
     -+  * If the format and extension are already enabled, then we can
     -+  * skip the upgrade process.
     ++  * If the extension is already enabled, then we can skip the
     ++  * upgrade process.
      +  */
      + if (repository_format_worktree_config)
      +         return 0;
     ++ if ((res = git_config_set_gently("extensions.worktreeConfig", "true")))
     ++         return error(_("failed to set extensions.worktreeConfig setting"));
      +
     -+ if (upgrade_repository_format(r, 1) < 0) {
     -+         res = error(_("unable to upgrade repository format to enable worktreeConfig"));
     -+         goto cleanup;
     -+ }
     -+ if ((res = git_config_set_gently("extensions.worktreeConfig", "true"))) {
     -+         error(_("failed to set extensions.worktreeConfig setting"));
     -+         goto cleanup;
     -+ }
     ++ git_configset_init(&cs);
     ++ git_configset_add_file(&cs, common_config_file);
      +
      + /*
      +  * If core.bare is true in the common config file, then we need to
     @@ builtin/worktree.c: static int repair(int ac, const char **av, const char *prefi
      +                 goto cleanup;
      + }
      +
     ++ /*
     ++  * Ensure that we use worktree config for the remaining lifetime
     ++  * of the current process.
     ++  */
     ++ repository_format_worktree_config = 1;
     ++
      +cleanup:
      + git_configset_clear(&cs);
      + 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"))

     - ## 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
     + ## worktree.h ##
     +@@ worktree.h: void strbuf_worktree_ref(const struct worktree *wt,
     +                   struct strbuf *sb,
     +                   const char *refname);
     +
     ++/**
     ++ * Enable worktree config for the first time. This will make the following
     ++ * adjustments:
     ++ *
     ++ * 1. Add extensions.worktreeConfig=true in the common config file.
     ++ *
     ++ * 2. If the common config file has a core.worktree value or core.bare is
     ++ *    set to true, then those values are moved to the main worktree's
     ++ *    config.worktree file.
     ++ *
     ++ * If extensions.worktreeConfig is already true, then this method
     ++ * terminates early without any of the above steps. The existing config
     ++ * arrangement is assumed to be intentional.
     ++ *
     ++ * Returns 0 on success. Reports an error message and returns non-zero
     ++ * if any of these steps fail.
     ++ */
     ++int init_worktree_config(struct repository *r);
     ++
     + #endif
 4:  45316cd01c9 ! 3:  110d5e0546c config: add repo_config_set_worktree_gently()
     @@ config.c: int git_config_set_gently(const char *key, const char *value)
      +         free(file);
      +         return ret;
      + }
     -+ return repo_config_set_gently(r, key, value);
     ++ return repo_config_set_multivar_gently(r, key, value, NULL, 0);
      +}
      +
       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);
     +  git_config_set_multivar(key, value, NULL, 0);
     +@@ config.c: void git_config_set_multivar_in_file(const char *config_filename,
     + int git_config_set_multivar_gently(const char *key, const char *value,
     +                             const char *value_pattern, unsigned flags)
     + {
     +- return git_config_set_multivar_in_file_gently(NULL, key, value, value_pattern,
     +-                                               flags);
     ++ return repo_config_set_multivar_gently(the_repository, key, value,
     ++                                        value_pattern, flags);
      +}
      +
     ++int repo_config_set_multivar_gently(struct repository *r, const char *key,
     ++                             const char *value,
     ++                             const char *value_pattern, unsigned flags)
     ++{
     ++ char *file = repo_git_path(r, "config");
     ++ int res = git_config_set_multivar_in_file_gently(file,
     ++                                                  key, value,
     ++                                                  value_pattern,
     ++                                                  flags);
     ++ free(file);
     ++ return res;
     + }
     +
       void git_config_set_multivar(const char *key, const char *value,
                             const char *value_pattern, unsigned flags)
       {
     +- git_config_set_multivar_in_file(NULL, key, value, value_pattern,
     ++ git_config_set_multivar_in_file(git_path("config"),
     ++                                 key, value, value_pattern,
     +                                  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 *);
       /**
        * 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
     +@@ config.h: int git_config_parse_key(const char *, char **, size_t *);
     +
     + int git_config_set_multivar_gently(const char *, const char *, const char *, unsigned);
       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 repo_config_set_multivar_gently(struct repository *, const char *, const char *, const char *, unsigned);
       int git_config_set_multivar_in_file_gently(const char *, const char *, const char *, const char *, unsigned);

       /**
 5:  b200819c1bb ! 4:  fbfaa17797c sparse-checkout: use repo_config_set_worktree_gently()
     @@ Metadata
      Author: Derrick Stolee [off-list ref]

       ## Commit message ##
     -    sparse-checkout: use repo_config_set_worktree_gently()
     +    sparse-checkout: set worktree-config correctly

          The previous change added repo_config_set_worktree_gently() to assist
     -    writing config values into the worktree.config file, if enabled.
     +    writing config values into the config.worktree file, if enabled. An
     +    earlier change added init_worktree_config() as a helper to initialize
     +    extensions.worktreeConfig if not already enabled.

     -    Let the sparse-checkout builtin use this helper instead of attempting to
     +    Let the sparse-checkout builtin use these helpers instead of attempting to
          initialize the worktree config on its own. This changes behavior of 'git
          sparse-checkout set' in a few important ways:

     -     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.
     +     1. Git will no longer upgrade the repository format, since this is not
     +        a requirement for understanding extensions.worktreeConfig.

     -     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
     +     2. 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
     @@ Commit message
          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 [off-list ref]
          Helped-by: Eric Sunshine [off-list ref]
          Signed-off-by: Derrick Stolee [off-list ref]

     + ## Documentation/git-sparse-checkout.txt ##
     +@@ Documentation/git-sparse-checkout.txt: COMMANDS
     +  Describe the patterns in the sparse-checkout file.
     +
     + 'set'::
     +- Enable the necessary config settings
     +- (extensions.worktreeConfig, core.sparseCheckout,
     +- core.sparseCheckoutCone) if they are not already enabled, and
     +- write a set of patterns to the sparse-checkout file from the
     +- list of arguments following the 'set' subcommand. Update the
     +- working directory to match the new patterns.
     ++ Enable the necessary sparse-checkout config settings
     ++ (`core.sparseCheckout` and possibly `core.sparseCheckoutCone`) if
     ++ they are not already enabled, and write a set of patterns to the
     ++ sparse-checkout file from the list of arguments following the
     ++ 'set' subcommand. Update the working directory to match the new
     ++ patterns.
     +++
     ++To ensure that adjusting the sparse-checkout settings within a worktree
     ++does not alter the sparse-checkout settings in other worktrees, the 'set'
     ++subcommand will upgrade your repository config to use worktree-specific
     ++config if not already present. The sparsity defined by the arguments to
     ++the 'set' subcommand are stored in the worktree-specific sparse-checkout
     ++file. See linkgit:git-worktree[1] and the documentation of
     ++`extensions.worktreeConfig` in linkgit:git-config[1] for more details.
     + +
     + When the `--stdin` option is provided, the patterns are read from
     + standard in as a newline-delimited list instead of from the arguments.
     +@@ Documentation/git-sparse-checkout.txt: interact with your repository until it is disabled.
     +  By default, these patterns are read from the command-line arguments,
     +  but they can be read from stdin using the `--stdin` option. When
     +  `core.sparseCheckoutCone` is enabled, the given patterns are interpreted
     +- as directory names as in the 'set' subcommand.
     ++ as directory names as in the 'set' subcommand. The sparsity defined
     ++ by the arguments to the 'add' subcommand are added to the patterns
     ++ in the worktree-specific sparse-checkout file.
     +
     + 'reapply'::
     +  Reapply the sparsity pattern rules to paths in the working tree.
     +
       ## builtin/sparse-checkout.c ##
     +@@
     + #include "wt-status.h"
     + #include "quote.h"
     + #include "sparse-index.h"
     ++#include "worktree.h"
     +
     + static const char *empty_base = "";
     +
      @@ builtin/sparse-checkout.c: enum sparse_checkout_mode {

       static int set_config(enum sparse_checkout_mode mode)
       {
      - const char *config_path;
      -
     -- if (upgrade_repository_format(the_repository, 1) < 0)
     +- if (upgrade_repository_format(1) < 0)
      -         die(_("unable to upgrade repository format to enable worktreeConfig"));
      - if (git_config_set_gently("extensions.worktreeConfig", "true")) {
      -         error(_("failed to set extensions.worktreeConfig setting"));
     -+ if (repo_config_set_worktree_gently(the_repository,
     -+                                     "core.sparseCheckout",
     -+                                     mode ? "true" : "false") ||
     -+     repo_config_set_worktree_gently(the_repository,
     -+                                     "core.sparseCheckoutCone",
     -+                                     mode == MODE_CONE_PATTERNS ?
     -+                                         "true" : "false"))
     ++ /* Update to use worktree config, if not already. */
     ++ if (init_worktree_config(the_repository)) {
     ++         error(_("failed to initialize worktree config"));
                return 1;
     -- }
     --
     +  }
     +
      - config_path = git_path("config.worktree");
      - git_config_set_in_file_gently(config_path,
      -                               "core.sparseCheckout",
     @@ builtin/sparse-checkout.c: enum sparse_checkout_mode {
      - git_config_set_in_file_gently(config_path,
      -                               "core.sparseCheckoutCone",
      -                               mode == MODE_CONE_PATTERNS ? "true" : NULL);
     ++ if (repo_config_set_worktree_gently(the_repository,
     ++                                     "core.sparseCheckout",
     ++                                     mode ? "true" : "false") ||
     ++     repo_config_set_worktree_gently(the_repository,
     ++                                     "core.sparseCheckoutCone",
     ++                                     mode == MODE_CONE_PATTERNS ?
     ++                                         "true" : "false"))
     ++         return 1;

        if (mode == MODE_NO_PATTERNS)
      -         set_sparse_index_config(the_repository, 0);
     @@ 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 'interaction with clone --no-checkout (unborn index)' '
     - '
     -
     - 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
     -+ ) &&
     -+ 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 '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 'switching to cone mode with non-cone mode patterns' '
     +          cd bad-patterns &&
     +          git sparse-checkout init &&
     +          git sparse-checkout add dir &&
     +-         git config core.sparseCheckoutCone true &&
     ++         git config --worktree core.sparseCheckoutCone true &&
     +          test_must_fail git sparse-checkout add dir 2>err &&
     +          grep "existing sparse-checkout patterns do not use cone mode" 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 &&
     +          test_cmp expect actual &&
     +
                git -C repo config --list >config &&
      -         ! grep index.sparse config
      +         test_cmp_config -C repo false index.sparse
        )
       '

     -@@ 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 ! 5:  bb9e550ff3d worktree: copy sparse-checkout patterns and config on add
     @@ Commit message
          In addition to the sparse-checkout file, copy the worktree config file
          if worktree config is enabled and the file exists. This will copy over
          any important settings to ensure the new worktree behaves the same as
     -    the current one.
     +    the current one. The only exception we must continue to make is that
     +    core.bare and core.worktree should become unset in the worktree's config
     +    file.

          Signed-off-by: Derrick Stolee [off-list ref]

     @@ builtin/worktree.c: static int add_worktree(const char *path, const char *refnam
      + }
      +
      + /*
     -+  * If we are using worktree config, then copy all currenct config
     ++  * If we are using worktree config, then copy all current config
      +  * values from the current worktree into the new one, that way the
      +  * new worktree behaves the same as this one.
      +  */
     @@ builtin/worktree.c: static int add_worktree(const char *path, const char *refnam
      +                                 realpath.buf, name);
      +
      +         if (file_exists(from_file)) {
     ++                 struct config_set cs = { { 0 }};
     ++                 const char *str_value;
     ++                 int bool_value;
     ++
      +                 if (safe_create_leading_directories(to_file) ||
      +                     copy_file(to_file, from_file, 0666))
     -+                         error(_("failed to copy worktree config from '%s' to '%s'"),
     -+                               from_file, to_file);
     ++                         die(_("failed to copy worktree config from '%s' to '%s'"),
     ++                             from_file, to_file);
     ++
     ++                 git_configset_init(&cs);
     ++                 git_configset_add_file(&cs, from_file);
     ++
     ++                 if (!git_configset_get_bool(&cs, "core.bare", &bool_value) &&
     ++                     bool_value &&
     ++                     git_config_set_multivar_in_file_gently(
     ++                                 to_file, "core.bare", NULL, "true", 0))
     ++                         error(_("failed to unset 'core.bare' in '%s'"), to_file);
     ++                 if (!git_configset_get_value(&cs, "core.worktree", &str_value) &&
     ++                     git_config_set_in_file_gently(to_file,
     ++                                                   "core.worktree", NULL))
     ++                         error(_("failed to unset 'core.worktree' in '%s'"), to_file);
     ++
     ++                 git_configset_clear(&cs);
      +         }
      +
      +         free(from_file);
     @@ builtin/worktree.c: static int add_worktree(const char *path, const char *refnam
        cp.git_cmd = 1;

       ## t/t1091-sparse-checkout-builtin.sh ##
     -@@ t/t1091-sparse-checkout-builtin.sh: test_expect_success 'set enables config' '
     +@@ t/t1091-sparse-checkout-builtin.sh: test_expect_success 'interaction with clone --no-checkout (unborn index)' '
       '

     - test_expect_success 'set enables worktree config, if enabled' '
     -+ git init worktree-patterns &&
     -+ (
     -+         cd worktree-patterns &&
     -+         test_commit test file &&
     -+         mkdir dir dir2 &&
     -+         test_commit dir dir/file &&
     -+         test_commit dir2 dir2/file &&
     -+
     -+         # By initializing the worktree config here...
     -+         git worktree init-worktree-config &&
     -+
     -+         # This set command places config values in worktree-
     -+         # specific config...
     -+         git sparse-checkout set --cone dir &&
     -+
     -+         # Which must be copied, along with the sparse-checkout
     -+         # patterns, here.
     -+         git worktree add --detach ../worktree-patterns2
     -+ ) &&
     -+ test_cmp_config -C worktree-patterns true core.sparseCheckout &&
     -+ test_cmp_config -C worktree-patterns2 true core.sparseCheckout &&
     -+ test_cmp_config -C worktree-patterns true core.sparseCheckoutCone &&
     -+ test_cmp_config -C worktree-patterns2 true core.sparseCheckoutCone &&
     -+ test_cmp worktree-patterns/.git/info/sparse-checkout \
     -+          worktree-patterns/.git/worktrees/worktree-patterns2/info/sparse-checkout &&
     -+
     -+ ls worktree-patterns >expect &&
     -+ ls worktree-patterns2 >actual &&
     -+ test_cmp expect actual &&
     -+
     -+ # Double check that the copy works from a non-main worktree.
     -+ (
     -+         cd worktree-patterns2 &&
     -+         git sparse-checkout set dir2 &&
     -+         git worktree add --detach ../worktree-patterns3
     -+ ) &&
     -+ test_cmp_config -C worktree-patterns3 true core.sparseCheckout &&
     -+ test_cmp_config -C worktree-patterns3 true core.sparseCheckoutCone &&
     -+ test_cmp worktree-patterns/.git/worktrees/worktree-patterns2/info/sparse-checkout \
     -+          worktree-patterns/.git/worktrees/worktree-patterns3/info/sparse-checkout &&
     -+
     -+ ls worktree-patterns2 >expect &&
     -+ ls worktree-patterns3 >actual &&
     -+ test_cmp expect actual
     + test_expect_success 'set enables config' '
     +- git init empty-config &&
     ++ git init worktree-config &&
     +  (
     +-         cd empty-config &&
     ++         cd worktree-config &&
     +          test_commit test file &&
     +          test_path_is_missing .git/config.worktree &&
     +          git sparse-checkout set nothing &&
     +@@ t/t1091-sparse-checkout-builtin.sh: test_expect_success 'add to sparse-checkout' '
     +  check_files repo "a folder1 folder2"
     + '
     +
     ++test_expect_success 'worktree: add copies sparse-checkout patterns' '
     ++ cat repo/.git/info/sparse-checkout >old &&
     ++ test_when_finished cp old repo/.git/info/sparse-checkout &&
     ++ test_when_finished git -C repo worktree remove ../worktree &&
     ++ git -C repo sparse-checkout set "/*" &&
     ++ git -C repo worktree add --quiet ../worktree 2>err &&
     ++ test_must_be_empty err &&
     ++ new=repo/.git/worktrees/worktree/info/sparse-checkout &&
     ++ test_path_is_file $new &&
     ++ test_cmp repo/.git/info/sparse-checkout $new &&
     ++ git -C worktree sparse-checkout set --cone &&
     ++ test_cmp_config -C worktree true core.sparseCheckoutCone &&
     ++ test_must_fail git -C repo core.sparseCheckoutCone
      +'
      +
     -+test_expect_success 'worktree add copies sparse-checkout patterns' '
     -  git init worktree-config &&
     -  (
     -          cd worktree-config &&
     + test_expect_success 'cone mode: match patterns' '
     +  git -C repo config --worktree core.sparseCheckoutCone true &&
     +  rm -rf repo/a repo/folder1 repo/folder2 &&
      @@ t/t1091-sparse-checkout-builtin.sh: test_expect_success 'interaction with submodules' '
     + '

       test_expect_success 'different sparse-checkouts with worktrees' '
     ++ git -C repo sparse-checkout set --cone deep folder1 &&
        git -C repo worktree add --detach ../worktree &&
      - check_files worktree "a deep folder1 folder2" &&
     -+ check_files worktree "a folder1" &&
     -  git -C worktree sparse-checkout init --cone &&
     +- git -C worktree sparse-checkout init --cone &&
      - git -C repo sparse-checkout set folder1 &&
     -+ git -C repo sparse-checkout set folder1 folder2 &&
     -  git -C worktree sparse-checkout set deep/deeper1 &&
     +- git -C worktree sparse-checkout set deep/deeper1 &&
      - check_files repo a folder1 &&
     -+ check_files repo a folder1 folder2 &&
     -  check_files worktree a deep
     +- check_files worktree a deep
     ++ check_files worktree "a deep folder1" &&
     ++ git -C repo sparse-checkout set --cone folder1 &&
     ++ git -C worktree sparse-checkout set --cone deep/deeper1 &&
     ++ check_files repo "a folder1" &&
     ++ check_files worktree "a deep"
     + '
     +
     + test_expect_success 'set using filename keeps file on-disk' '
     +
     + ## t/t2400-worktree-add.sh ##
     +@@ t/t2400-worktree-add.sh: test_expect_success '"add" default branch of a bare repo' '
     +  (
     +          git clone --bare . bare2 &&
     +          cd bare2 &&
     +-         git worktree add ../there3 main
     +- )
     ++         git worktree add ../there3 main &&
     ++         cd ../there3 &&
     ++         git status
     ++ ) &&
     ++ cat >expect <<-EOF &&
     ++ init.t
     ++ EOF
     ++ ls there3 >actual &&
     ++ test_cmp expect actual
     ++'
     ++
     ++test_expect_success '"add" to bare repo with worktree config' '
     ++ (
     ++         git clone --bare . bare3 &&
     ++         cd bare3 &&
     ++         git config extensions.worktreeconfig true &&
     ++         git config --worktree core.bare true &&
     ++         git config --worktree core.worktree "$(pwd)" &&
     ++         git config --worktree bogus.key value &&
     ++         git config --unset core.bare &&
     ++         git worktree add ../there4 main &&
     ++         cd ../there4 &&
     ++         git status &&
     ++         git worktree add --detach ../there5 &&
     ++         cd ../there5 &&
     ++         git status
     ++ ) &&
     ++
     ++ # the worktree has the arbitrary value copied.
     ++ test_cmp_config -C there4 value bogus.key &&
     ++ test_cmp_config -C there5 value bogus.key &&
     ++
     ++ # however, core.bare and core.worktree were removed.
     ++ test_must_fail git -C there4 config core.bare &&
     ++ test_must_fail git -C there4 config core.worktree &&
     ++
     ++ cat >expect <<-EOF &&
     ++ init.t
     ++ EOF
     ++
     ++ ls there4 >actual &&
     ++ test_cmp expect actual &&
     ++ ls there5 >actual &&
     ++ test_cmp expect actual
       '

     + test_expect_success 'checkout with grafts' '

--
gitgitgadget

Re: [PATCH v4 1/5] Documentation: add extensions.worktreeConfig details

From: Derrick Stolee <hidden>
Date: 2022-01-27 14:15:51

On 1/26/2022 1:59 AM, Bagas Sanjaya wrote:

On 26/01/22 01.42, Derrick Stolee via GitGitGadget wrote:

quoted

As documented in 11664196ac ("Revert "check_repository_format_gently():
refuse extensions for old repositories"", 2020-07-15), this extension
must be considered regardless of the repository format version for
historical reasons.

...

quoted

+For historical reasons, `extensions.worktreeConfig` is respected
+regardless of the `core.repositoryFormatVersion` setting.

This implies `extensions.worktreeConfig` become integral part of every
repository format version, from the past until now and to the future,
right?

 
Right. What I'm saying is that this is already the case. I'm not
changing it. The commit message in 116664196ac discusses this in
depth. I'm pointing it out carefully because it is news to me
(hence why the sparse-checkout builtin updates the repository
format version before this series).

Thanks,
-Stolee

Re: [PATCH v4 1/5] Documentation: add extensions.worktreeConfig details

From: Derrick Stolee <hidden>
Date: 2022-01-27 14:17:33

On 1/27/2022 1:43 AM, Elijah Newren wrote:

On Tue, Jan 25, 2022 at 10:42 AM Derrick Stolee via GitGitGadget
[off-list ref] wrote:

quoted

 --worktree::
-       Similar to `--local` except that `.git/config.worktree` is
+       Similar to `--local` except that `$GIT_DIR/config.worktree` is
        read from or written to if `extensions.worktreeConfig` is
-       present. If not it's the same as `--local`.
+       enabled. If not it's the same as `--local`. Note that `$GIT_DIR`
+       is equal to `$GIT_COMMON_DIR` for the main worktree, but is of the
+       form `.git/worktrees/<worktree-name>/` for other worktrees. See

is of the form `$GIT_DIR/worktrees/<worktree-name>/`; .git isn't even
a directory in other worktrees.

Thanks. I tried to catch all of these, but I missed one.

quoted

+       linkgit:git-worktree[1] to learn how to enable
+       `extensions.worktreeConfig`.

 -f <config-file>::
 --file <config-file>::

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 9e862fbcf79..ea0ee9f8bb5 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt

@@ -286,8 +286,8 @@ CONFIGURATION FILE
 ------------------
 By default, the repository `config` file is shared across all working
 trees. If the config variables `core.bare` or `core.worktree` are
-already present in the config file, they will be applied to the main
-working trees only.
+present in the common config file and `extensions.worktreeConfig` is
+disabled, then they will be applied to the main working trees only.

"main working trees"?  Is that an accidental plural?

Yes. Thanks.

quoted

 In order to have configuration specific to working trees, you can turn
 on the `worktreeConfig` extension, e.g.:

@@ -307,11 +307,16 @@ 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:

- - `core.worktree` and `core.bare` should never be shared
+ - `core.worktree` should never be shared.
+
+ - `core.bare` should not be shared unless the value is `core.bare=false`.

The double negative makes for harder parsing.  Perhaps
    - `core.bare` should not be shared if the value is `core.bare=true`
?

That is cleaner. Excellent.

quoted

  - `core.sparseCheckout` is recommended per working tree, unless you
    are sure you always use sparse checkout for all working trees.

+See the documentation of `extensions.worktreeConfig` in
+linkgit:git-config[1] for more details.
+
 DETAILS
 -------
 Each linked working tree has a private sub-directory in the repository's
--
gitgitgadget

Thanks for documenting these details; I had some very minor comments
but this is well written and very helpful.

Thanks for your recommended improvements.
-Stolee

Re: [PATCH v4 4/5] sparse-checkout: set worktree-config correctly

From: Derrick Stolee <hidden>
Date: 2022-01-27 14:25:01

On 1/27/2022 2:15 AM, Elijah Newren wrote:

On Tue, Jan 25, 2022 at 10:42 AM Derrick Stolee via GitGitGadget
[off-list ref] wrote:

quoted

 'set'::
-       Enable the necessary config settings
-       (extensions.worktreeConfig, core.sparseCheckout,
-       core.sparseCheckoutCone) if they are not already enabled, and
-       write a set of patterns to the sparse-checkout file from the
-       list of arguments following the 'set' subcommand. Update the
-       working directory to match the new patterns.
+       Enable the necessary sparse-checkout config settings
+       (`core.sparseCheckout` and possibly `core.sparseCheckoutCone`) if

and possibly index.sparse as well, right?

Good catch.

quoted

+       they are not already enabled, and write a set of patterns to the
+       sparse-checkout file from the list of arguments following the
+       'set' subcommand. Update the working directory to match the new
+       patterns.
++
+To ensure that adjusting the sparse-checkout settings within a worktree
+does not alter the sparse-checkout settings in other worktrees, the 'set'
+subcommand will upgrade your repository config to use worktree-specific
+config if not already present. The sparsity defined by the arguments to
+the 'set' subcommand are stored in the worktree-specific sparse-checkout
+file. See linkgit:git-worktree[1] and the documentation of
+`extensions.worktreeConfig` in linkgit:git-config[1] for more details.
 +
 When the `--stdin` option is provided, the patterns are read from
 standard in as a newline-delimited list instead of from the arguments.

@@ -73,7 +81,9 @@ interact with your repository until it is disabled.
        By default, these patterns are read from the command-line arguments,
        but they can be read from stdin using the `--stdin` option. When
        `core.sparseCheckoutCone` is enabled, the given patterns are interpreted
-       as directory names as in the 'set' subcommand.
+       as directory names as in the 'set' subcommand. The sparsity defined
+       by the arguments to the 'add' subcommand are added to the patterns
+       in the worktree-specific sparse-checkout file.

This sentence addition makes your series conflict with patch 4 of my
en/present-despite-skipped series.

The sentence also seems somewhat redundant with the first sentence of
the paragraph (not quoted here).  Perhaps consider just dropping it to
make it easier for Junio to integrate?

I'll just drop it. Thanks.

quoted

 'reapply'::
        Reapply the sparsity pattern rules to paths in the working tree.

diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index 679c1070368..314c8d61f80 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c

@@ -15,6 +15,7 @@
 #include "wt-status.h"
 #include "quote.h"
 #include "sparse-index.h"
+#include "worktree.h"

 static const char *empty_base = "";

@@ -359,26 +360,23 @@ enum sparse_checkout_mode {

 static int set_config(enum sparse_checkout_mode mode)
 {
-       const char *config_path;
-
-       if (upgrade_repository_format(1) < 0)
-               die(_("unable to upgrade repository format to enable worktreeConfig"));

Wait, this got added in mid-2020, since v2.28.0 -- and I missed it but
it hasn't bit us yet??

I'm so sorry.  Earlier when I objected to setting this, I was worried
it was a new change and would introduce some new failures, but clearly
it was already introduced...and it turns out we've been fine.  So, I
was wrong to worry about this.

(Which is to say, if you want to add it back, I'm fine with it given
this info.  If you'd rather still take it out, I'm fine with that
too.)

Yes, this has been around for a while. However, based on our discussion on
this topic, this was never necessary for the worktreeConfig extension.

Removing it seems to increase our compatibility, but it would be interesting
if we catch new compatibility problems. I'm thinking specifically about
third-party tools that should complain about extensions.worktreeConfig and
specifically reading sparse-checkout settings from worktree config files.

Patch looks good; I only had a few very minor comments.

Thanks,
-Stolee

Re: [PATCH v4 0/5] Sparse checkout: fix bug with worktree of bare repo

From: Derrick Stolee <hidden>
Date: 2022-01-27 14:29:04

On 1/27/2022 2:20 AM, Elijah Newren wrote:

On Tue, Jan 25, 2022 at 10:42 AM Derrick Stolee via GitGitGadget
[off-list ref] wrote:

quoted

This series is now based on v2.35.0 since that contains all of the necessary
topics.

Heads up for the maintainer: has a minor textual conflict with
en/present-despite-skipped (this series modifies the end of a
paragraph that the other series modifies the beginning of).

Thanks for pointing this out. My next version will not have this conflict,
so feel free to resolve the conflict by dropping the changes on this side.

quoted

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 or core.worktree are set in the
common config file. This series fixes this, but also puts in place some
helpers to prevent this from happening in the future.

This series has become pretty solid.  I had only very minor comments
on the patches.  Thanks for working on this.

Thank you for your careful review. I'll let it simmer over the weekend
before sending a v5 that includes your recommended changes.

Thanks,
-Stolee

[PATCH v5 0/5] Sparse checkout: fix bug with worktree of bare repo

From: Derrick Stolee via GitGitGadget <hidden>
Date: 2022-01-31 15:01:05

This series is now based on v2.35.0 since that contains all of the necessary
topics.

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 or core.worktree are set in the
common config file. This series fixes this, but also puts in place some
helpers to prevent this from happening in the future.

ATTENTION: I have significantly redesigned the series since previous
versions, so most of this cover letter is new.

 * Patch 1 updates documentation around extensions.worktreeConfig in a few
   places to improve discoverability. Several cross links are added to make
   it easy to find the related areas. (The documentation for the changes to
   'git sparse-checkout' are delayed to patch 4.)

 * Patch 2 introduces the init_worktree_config() helper which follows the
   documented instructions to enable extensions.worktreeConfig as well as
   move the core.bare and core.worktree config values. This update does not
   modify core.repositoryFormatVersion, since this is not needed
   specifically for extensions.worktreeConfig.

 * Patch 3 adds a new repo_config_set_worktree_gently() helper method so we
   can internally adjust a config value within a worktree, at least if
   extensions.worktreeConfig is enabled. (It will write to the common config
   file if the extension is not enabled.)

 * Patch 4 modifies the sparse-checkout builtin to use
   init_worktree_config() and repo_config_set_worktree_gently() in ways that
   fix the reported bug. The behavior change here is that it will no longer
   upgrade the repository format version, since that is not needed for
   extensions.worktreeConfig.

 * Patch 5 updates 'git worktree add' to copy the worktree config from the
   current worktree to the new one (while unsetting core.bare=true and
   core.worktree=*) along with copying the sparse-checkout patterns file.

[1]
https://lore.kernel.org/git/CABceR4bZmtC4rCwgxZ1BBYZP69VOUca1f_moJoP989vTUZWu9Q@mail.gmail.com/
[2]
https://lore.kernel.org/git/CAPig+cQ6U_yFw-X2OWrizB1rbCvc4bNxuSzKFzmoLNnm0GH8Eg@mail.gmail.com/


Updates in v5
=============

 * Cleaned up documentation as per Elijah's suggestions.
 * Removed unnecessary conflicting change in git-sparse-checkout.txt
 * Fixed an ambiguous comment about moving config values.


Updates in v4
=============

 * Rebased to v2.35.0
 * Fixed memory leak (was leaking repo_git_path() result)
 * Added additional documentation updates so curious users can discover the
   intricacies of extensions.worktreeConfig from multiple entry points.
 * Significantly reduced the amount of changes to config.c.
 * 'git sparse-checkout' no longer upgrades the repository format.
 * Dropped the update to upgrade_repository_format(), since it is not
   needed.
 * Dropped the 'git worktree init-worktree-config' subcommand in favor of a
   helper method called by 'git sparse-checkout'
 * Many others because of the significant changes required by the above
   items.

Thanks, -Stolee

Derrick Stolee (5):
  Documentation: add extensions.worktreeConfig details
  worktree: create init_worktree_config()
  config: add repo_config_set_worktree_gently()
  sparse-checkout: set worktree-config correctly
  worktree: copy sparse-checkout patterns and config on add

 Documentation/config/extensions.txt   | 31 ++++++++++++
 Documentation/git-config.txt          |  8 ++-
 Documentation/git-sparse-checkout.txt | 16 ++++--
 Documentation/git-worktree.txt        | 11 +++--
 builtin/sparse-checkout.c             | 28 +++++------
 builtin/worktree.c                    | 60 +++++++++++++++++++++++
 config.c                              | 35 ++++++++++++--
 config.h                              |  8 +++
 sparse-index.c                        | 10 ++--
 t/t1091-sparse-checkout-builtin.sh    | 35 ++++++++++----
 t/t2400-worktree-add.sh               | 46 +++++++++++++++++-
 worktree.c                            | 70 +++++++++++++++++++++++++++
 worktree.h                            | 21 ++++++++
 13 files changed, 333 insertions(+), 46 deletions(-)


base-commit: 89bece5c8c96f0b962cfc89e63f82d603fd60bed
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1101%2Fderrickstolee%2Fsparse-checkout%2Fbare-worktree-bug-v5
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1101/derrickstolee/sparse-checkout/bare-worktree-bug-v5
Pull-Request: https://github.com/gitgitgadget/git/pull/1101

Range-diff vs v4:

 1:  459e09dedd7 ! 1:  1bd5f26271c Documentation: add extensions.worktreeConfig details
     @@ Commit message
          within git-sparse-checkout.txt, but a behavior change is needed before
          making those updates.
      
     +    Helped-by: Elijah Newren [off-list ref]
          Signed-off-by: Derrick Stolee [off-list ref]
      
       ## Documentation/config/extensions.txt ##
     @@ Documentation/git-config.txt: from all available files.
      -	present. If not it's the same as `--local`.
      +	enabled. If not it's the same as `--local`. Note that `$GIT_DIR`
      +	is equal to `$GIT_COMMON_DIR` for the main worktree, but is of the
     -+	form `.git/worktrees/<worktree-name>/` for other worktrees. See
     ++	form `$GIT_DIR/worktrees/<worktree-name>/` for other worktrees. See
      +	linkgit:git-worktree[1] to learn how to enable
      +	`extensions.worktreeConfig`.
       
     @@ Documentation/git-worktree.txt: CONFIGURATION FILE
      -already present in the config file, they will be applied to the main
      -working trees only.
      +present in the common config file and `extensions.worktreeConfig` is
     -+disabled, then they will be applied to the main working trees only.
     ++disabled, then they will be applied to the main working tree only.
       
       In order to have configuration specific to working trees, you can turn
       on the `worktreeConfig` extension, e.g.:
     @@ Documentation/git-worktree.txt: them to the `config.worktree` of the main workin
      - - `core.worktree` and `core.bare` should never be shared
      + - `core.worktree` should never be shared.
      +
     -+ - `core.bare` should not be shared unless the value is `core.bare=false`.
     ++ - `core.bare` should not be shared if the value is `core.bare=true`.
       
        - `core.sparseCheckout` is recommended per working tree, unless you
          are sure you always use sparse checkout for all working trees.
 2:  d262a76b448 ! 2:  2a2c350112e worktree: create init_worktree_config()
     @@ worktree.h: void strbuf_worktree_ref(const struct worktree *wt,
      + *
      + * 1. Add extensions.worktreeConfig=true in the common config file.
      + *
     -+ * 2. If the common config file has a core.worktree value or core.bare is
     -+ *    set to true, then those values are moved to the main worktree's
     -+ *    config.worktree file.
     ++ * 2. If the common config file has a core.worktree value, then that value
     ++ *    is moved to the main worktree's config.worktree file.
     ++ *
     ++ * 3. If the common config file has a core.bare enabled, then that value
     ++ *    is moved to the main worktree's config.worktree file.
      + *
      + * If extensions.worktreeConfig is already true, then this method
      + * terminates early without any of the above steps. The existing config
 3:  110d5e0546c = 3:  802b28a9510 config: add repo_config_set_worktree_gently()
 4:  fbfaa17797c ! 4:  08b89d17ccf sparse-checkout: set worktree-config correctly
     @@ Documentation/git-sparse-checkout.txt: COMMANDS
      -	(extensions.worktreeConfig, core.sparseCheckout,
      -	core.sparseCheckoutCone) if they are not already enabled, and
      -	write a set of patterns to the sparse-checkout file from the
     --	list of arguments following the 'set' subcommand. Update the
     --	working directory to match the new patterns.
      +	Enable the necessary sparse-checkout config settings
     -+	(`core.sparseCheckout` and possibly `core.sparseCheckoutCone`) if
     -+	they are not already enabled, and write a set of patterns to the
     -+	sparse-checkout file from the list of arguments following the
     -+	'set' subcommand. Update the working directory to match the new
     -+	patterns.
     -++
     ++	(`core.sparseCheckout`, `core.sparseCheckoutCone`, and
     ++	`index.sparse`) if they are not already set to the desired values,
     ++	and write a set of patterns to the sparse-checkout file from the
     + 	list of arguments following the 'set' subcommand. Update the
     + 	working directory to match the new patterns.
     + +
      +To ensure that adjusting the sparse-checkout settings within a worktree
      +does not alter the sparse-checkout settings in other worktrees, the 'set'
      +subcommand will upgrade your repository config to use worktree-specific
     @@ Documentation/git-sparse-checkout.txt: COMMANDS
      +the 'set' subcommand are stored in the worktree-specific sparse-checkout
      +file. See linkgit:git-worktree[1] and the documentation of
      +`extensions.worktreeConfig` in linkgit:git-config[1] for more details.
     - +
     +++
       When the `--stdin` option is provided, the patterns are read from
       standard in as a newline-delimited list instead of from the arguments.
     -@@ Documentation/git-sparse-checkout.txt: interact with your repository until it is disabled.
     - 	By default, these patterns are read from the command-line arguments,
     - 	but they can be read from stdin using the `--stdin` option. When
     - 	`core.sparseCheckoutCone` is enabled, the given patterns are interpreted
     --	as directory names as in the 'set' subcommand.
     -+	as directory names as in the 'set' subcommand. The sparsity defined
     -+	by the arguments to the 'add' subcommand are added to the patterns
     -+	in the worktree-specific sparse-checkout file.
     - 
     - 'reapply'::
     - 	Reapply the sparsity pattern rules to paths in the working tree.
     + +
      
       ## builtin/sparse-checkout.c ##
      @@
 5:  bb9e550ff3d ! 5:  85779dfaed3 worktree: copy sparse-checkout patterns and config on add
     @@ t/t1091-sparse-checkout-builtin.sh: test_expect_success 'add to sparse-checkout'
      +	cat repo/.git/info/sparse-checkout >old &&
      +	test_when_finished cp old repo/.git/info/sparse-checkout &&
      +	test_when_finished git -C repo worktree remove ../worktree &&
     -+	git -C repo sparse-checkout set "/*" &&
     ++	git -C repo sparse-checkout set --no-cone "/*" &&
      +	git -C repo worktree add --quiet ../worktree 2>err &&
      +	test_must_be_empty err &&
      +	new=repo/.git/worktrees/worktree/info/sparse-checkout &&

-- 
gitgitgadget

[PATCH v5 1/5] Documentation: add extensions.worktreeConfig details

From: Derrick Stolee via GitGitGadget <hidden>
Date: 2022-01-31 15:01:06

From: Derrick Stolee <redacted>

The extensions.worktreeConfig extension was added in 58b284a (worktree:
add per-worktree config files, 2018-10-21) and was somewhat documented
in Documentation/git-config.txt. However, the extensions.worktreeConfig
value was not specified further in the list of possible config keys. The
location of the config.worktree file is not specified, and there are
some precautions that should be mentioned clearly, but are only
mentioned in git-worktree.txt.

Expand the documentation to help users discover the complexities of
extensions.worktreeConfig by adding details and cross links in these
locations (relative to Documentation/):

- config/extensions.txt
- git-config.txt
- git-worktree.txt

The updates focus on items such as

* $GIT_DIR/config.worktree takes precedence over $GIT_COMMON_DIR/config.

* The core.worktree and core.bare=true settings are incorrect to have in
  the common config file when extensions.worktreeConfig is enabled.

* The sparse-checkout settings core.sparseCheckout[Cone] are recommended
  to be set in the worktree config.

As documented in 11664196ac ("Revert "check_repository_format_gently():
refuse extensions for old repositories"", 2020-07-15), this extension
must be considered regardless of the repository format version for
historical reasons.

A future change will update references to extensions.worktreeConfig
within git-sparse-checkout.txt, but a behavior change is needed before
making those updates.

Helped-by: Elijah Newren [off-list ref]
Signed-off-by: Derrick Stolee <redacted>
---
 Documentation/config/extensions.txt | 31 +++++++++++++++++++++++++++++
 Documentation/git-config.txt        |  8 ++++++--
 Documentation/git-worktree.txt      | 11 +++++++---
 3 files changed, 45 insertions(+), 5 deletions(-)

diff --git a/Documentation/config/extensions.txt b/Documentation/config/extensions.txt
index 4e23d73cdca..5999dcb2a1f 100644
--- a/Documentation/config/extensions.txt
+++ b/Documentation/config/extensions.txt

@@ -6,3 +6,34 @@ extensions.objectFormat::
 Note that this setting should only be set by linkgit:git-init[1] or
 linkgit:git-clone[1].  Trying to change it after initialization will not
 work and will produce hard-to-diagnose issues.
+
+extensions.worktreeConfig::
+	If enabled, then worktrees will load config settings from the
+	`$GIT_DIR/config.worktree` file in addition to the
+	`$GIT_COMMON_DIR/config` file. Note that `$GIT_COMMON_DIR` and
+	`$GIT_DIR` are the same for the main worktree, while other
+	worktrees have `$GIT_DIR` equal to
+	`$GIT_COMMON_DIR/worktrees/<worktree-name>/`. The settings in the
+	`config.worktree` file will override settings from any other
+	config files.
++
+When enabling `extensions.worktreeConfig`, you must be careful to move
+certain values from the common config file to the main worktree's
+`config.worktree` file, if present:
++
+* `core.worktree` must be moved from `$GIT_COMMON_DIR/config` to
+  `$GIT_COMMON_DIR/config.worktree`.
+* If `core.bare` is true, then it must be moved from `$GIT_COMMON_DIR/config`
+  to `$GIT_COMMON_DIR/config.worktree`.
++
+It may also be beneficial to adjust the locations of `core.sparseCheckout`
+and `core.sparseCheckoutCone` depending on your desire for customizable
+sparse-checkout settings for each worktree. By default, the `git
+sparse-checkout` builtin enables `extensions.worktreeConfig`, assigns
+these config values on a per-worktree basis, and uses the
+`$GIT_DIR/info/sparse-checkout` file to specify the sparsity for each
+worktree independently. See linkgit:git-sparse-checkout[1] for more
+details.
++
+For historical reasons, `extensions.worktreeConfig` is respected
+regardless of the `core.repositoryFormatVersion` setting.

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 2285effb363..a48f7529fbc 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt

@@ -141,9 +141,13 @@ from all available files.
 See also <<FILES>>.
 
 --worktree::
-	Similar to `--local` except that `.git/config.worktree` is
+	Similar to `--local` except that `$GIT_DIR/config.worktree` is
 	read from or written to if `extensions.worktreeConfig` is
-	present. If not it's the same as `--local`.
+	enabled. If not it's the same as `--local`. Note that `$GIT_DIR`
+	is equal to `$GIT_COMMON_DIR` for the main worktree, but is of the
+	form `$GIT_DIR/worktrees/<worktree-name>/` for other worktrees. See
+	linkgit:git-worktree[1] to learn how to enable
+	`extensions.worktreeConfig`.
 
 -f <config-file>::
 --file <config-file>::

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 9e862fbcf79..b8d53c48303 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt

@@ -286,8 +286,8 @@ CONFIGURATION FILE
 ------------------
 By default, the repository `config` file is shared across all working
 trees. If the config variables `core.bare` or `core.worktree` are
-already present in the config file, they will be applied to the main
-working trees only.
+present in the common config file and `extensions.worktreeConfig` is
+disabled, then they will be applied to the main working tree only.
 
 In order to have configuration specific to working trees, you can turn
 on the `worktreeConfig` extension, e.g.:

@@ -307,11 +307,16 @@ 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:
 
- - `core.worktree` and `core.bare` should never be shared
+ - `core.worktree` should never be shared.
+
+ - `core.bare` should not be shared if the value is `core.bare=true`.
 
  - `core.sparseCheckout` is recommended per working tree, unless you
    are sure you always use sparse checkout for all working trees.
 
+See the documentation of `extensions.worktreeConfig` in
+linkgit:git-config[1] for more details.
+
 DETAILS
 -------
 Each linked working tree has a private sub-directory in the repository's

-- 
gitgitgadget

[PATCH v5 2/5] worktree: create init_worktree_config()

From: Derrick Stolee via GitGitGadget <hidden>
Date: 2022-01-31 15:01:07

From: Derrick Stolee <redacted>

Upgrading a repository to use extensions.worktreeConfig is non-trivial.
There are several steps involved, including moving some config settings
from the common config file to the main worktree's config.worktree file.
The previous change updated the documentation with all of these details.

Commands such as 'git sparse-checkout set' upgrade the repository to use
extensions.worktreeConfig without following these steps, causing some
user pain in some special cases.

Create a helper method, init_worktree_config(), that will be used in a
later change to fix this behavior within 'git sparse-checkout set'. The
method is carefully documented in worktree.h.

Note that we do _not_ upgrade the repository format version to 1 during
this process. The worktree config extension must be considered by Git
and third-party tools even if core.repositoryFormatVersion is 0 for
historical reasons documented in 11664196ac ("Revert
"check_repository_format_gently(): refuse extensions for old
repositories"", 2020-07-15). This is a special case for this extension,
and newer extensions (such as extensions.objectFormat) still need to
upgrade the repository format version.

Signed-off-by: Derrick Stolee <redacted>
---
 worktree.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 worktree.h | 21 ++++++++++++++++
 2 files changed, 91 insertions(+)

diff --git a/worktree.c b/worktree.c
index 6f598dcfcdf..dc4ead4c8fb 100644
--- a/worktree.c
+++ b/worktree.c

@@ -5,6 +5,7 @@
 #include "worktree.h"
 #include "dir.h"
 #include "wt-status.h"
+#include "config.h"
 
 void free_worktrees(struct worktree **worktrees)
 {

@@ -826,3 +827,72 @@ int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath,
 	*wtpath = path;
 	return 0;
 }
+
+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 init_worktree_config(struct repository *r)
+{
+	int res = 0;
+	int bare = 0;
+	struct config_set cs = { { 0 } };
+	const char *core_worktree;
+	char *common_config_file = xstrfmt("%s/config", r->commondir);
+	char *main_worktree_file = xstrfmt("%s/config.worktree", r->commondir);
+
+	/*
+	 * If the extension is already enabled, then we can skip the
+	 * upgrade process.
+	 */
+	if (repository_format_worktree_config)
+		return 0;
+	if ((res = git_config_set_gently("extensions.worktreeConfig", "true")))
+		return error(_("failed to set extensions.worktreeConfig setting"));
+
+	git_configset_init(&cs);
+	git_configset_add_file(&cs, common_config_file);
+
+	/*
+	 * 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;
+	}
+
+	/*
+	 * Ensure that we use worktree config for the remaining lifetime
+	 * of the current process.
+	 */
+	repository_format_worktree_config = 1;
+
+cleanup:
+	git_configset_clear(&cs);
+	free(common_config_file);
+	free(main_worktree_file);
+	return res;
+}

diff --git a/worktree.h b/worktree.h
index 9e06fcbdf3d..e9e839926b0 100644
--- a/worktree.h
+++ b/worktree.h

@@ -183,4 +183,25 @@ void strbuf_worktree_ref(const struct worktree *wt,
 			 struct strbuf *sb,
 			 const char *refname);
 
+/**
+ * Enable worktree config for the first time. This will make the following
+ * adjustments:
+ *
+ * 1. Add extensions.worktreeConfig=true in the common config file.
+ *
+ * 2. If the common config file has a core.worktree value, then that value
+ *    is moved to the main worktree's config.worktree file.
+ *
+ * 3. If the common config file has a core.bare enabled, then that value
+ *    is moved to the main worktree's config.worktree file.
+ *
+ * If extensions.worktreeConfig is already true, then this method
+ * terminates early without any of the above steps. The existing config
+ * arrangement is assumed to be intentional.
+ *
+ * Returns 0 on success. Reports an error message and returns non-zero
+ * if any of these steps fail.
+ */
+int init_worktree_config(struct repository *r);
+
 #endif

-- 
gitgitgadget

[PATCH v5 3/5] config: add repo_config_set_worktree_gently()

From: Derrick Stolee via GitGitGadget <hidden>
Date: 2022-01-31 15:01:09

From: Derrick Stolee <redacted>

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().

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 <redacted>
---
 config.c | 35 ++++++++++++++++++++++++++++++++---
 config.h |  8 ++++++++
 2 files changed, 40 insertions(+), 3 deletions(-)

diff --git a/config.c b/config.c
index 2bffa8d4a01..1a03ced1a54 100644
--- a/config.c
+++ b/config.c

@@ -21,6 +21,7 @@
 #include "dir.h"
 #include "color.h"
 #include "refs.h"
+#include "worktree.h"
 
 struct config_source {
 	struct config_source *prev;

@@ -2884,6 +2885,20 @@ int git_config_set_gently(const char *key, const char *value)
 	return git_config_set_multivar_gently(key, value, NULL, 0);
 }
 
+int repo_config_set_worktree_gently(struct repository *r,
+				    const char *key, const char *value)
+{
+	/* 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_multivar_gently(r, key, value, NULL, 0);
+}
+
 void git_config_set(const char *key, const char *value)
 {
 	git_config_set_multivar(key, value, NULL, 0);

@@ -3181,14 +3196,28 @@ void git_config_set_multivar_in_file(const char *config_filename,
 int git_config_set_multivar_gently(const char *key, const char *value,
 				   const char *value_pattern, unsigned flags)
 {
-	return git_config_set_multivar_in_file_gently(NULL, key, value, value_pattern,
-						      flags);
+	return repo_config_set_multivar_gently(the_repository, key, value,
+					       value_pattern, flags);
+}
+
+int repo_config_set_multivar_gently(struct repository *r, const char *key,
+				    const char *value,
+				    const char *value_pattern, unsigned flags)
+{
+	char *file = repo_git_path(r, "config");
+	int res = git_config_set_multivar_in_file_gently(file,
+							 key, value,
+							 value_pattern,
+							 flags);
+	free(file);
+	return res;
 }
 
 void git_config_set_multivar(const char *key, const char *value,
 			     const char *value_pattern, unsigned flags)
 {
-	git_config_set_multivar_in_file(NULL, key, value, value_pattern,
+	git_config_set_multivar_in_file(git_path("config"),
+					key, value, value_pattern,
 					flags);
 }

diff --git a/config.h b/config.h
index f119de01309..1d98ad269bd 100644
--- a/config.h
+++ b/config.h

@@ -253,6 +253,13 @@ 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 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.
  */

@@ -281,6 +288,7 @@ int git_config_parse_key(const char *, char **, size_t *);
 
 int git_config_set_multivar_gently(const char *, const char *, const char *, unsigned);
 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);
 int git_config_set_multivar_in_file_gently(const char *, const char *, const char *, const char *, unsigned);
 
 /**

-- 
gitgitgadget

[PATCH v5 4/5] sparse-checkout: set worktree-config correctly

From: Derrick Stolee via GitGitGadget <hidden>
Date: 2022-01-31 15:01:12

From: Derrick Stolee <redacted>

The previous change added repo_config_set_worktree_gently() to assist
writing config values into the config.worktree file, if enabled. An
earlier change added init_worktree_config() as a helper to initialize
extensions.worktreeConfig if not already enabled.

Let the sparse-checkout builtin use these helpers instead of attempting to
initialize the worktree config on its own. This changes behavior of 'git
sparse-checkout set' in a few important ways:

 1. Git will no longer upgrade the repository format, since this is not
    a requirement for understanding extensions.worktreeConfig.

 2. 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.

Reported-by: Sean Allred <redacted>
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Derrick Stolee <redacted>
---
 Documentation/git-sparse-checkout.txt | 16 +++++++++++----
 builtin/sparse-checkout.c             | 28 +++++++++++++--------------
 sparse-index.c                        | 10 +++-------
 t/t1091-sparse-checkout-builtin.sh    |  4 ++--
 4 files changed, 30 insertions(+), 28 deletions(-)

diff --git a/Documentation/git-sparse-checkout.txt b/Documentation/git-sparse-checkout.txt
index b81dbe06543..94dad137b96 100644
--- a/Documentation/git-sparse-checkout.txt
+++ b/Documentation/git-sparse-checkout.txt

@@ -31,13 +31,21 @@ COMMANDS
 	Describe the patterns in the sparse-checkout file.
 
 'set'::
-	Enable the necessary config settings
-	(extensions.worktreeConfig, core.sparseCheckout,
-	core.sparseCheckoutCone) if they are not already enabled, and
-	write a set of patterns to the sparse-checkout file from the
+	Enable the necessary sparse-checkout config settings
+	(`core.sparseCheckout`, `core.sparseCheckoutCone`, and
+	`index.sparse`) if they are not already set to the desired values,
+	and write a set of patterns to the sparse-checkout file from the
 	list of arguments following the 'set' subcommand. Update the
 	working directory to match the new patterns.
 +
+To ensure that adjusting the sparse-checkout settings within a worktree
+does not alter the sparse-checkout settings in other worktrees, the 'set'
+subcommand will upgrade your repository config to use worktree-specific
+config if not already present. The sparsity defined by the arguments to
+the 'set' subcommand are stored in the worktree-specific sparse-checkout
+file. See linkgit:git-worktree[1] and the documentation of
+`extensions.worktreeConfig` in linkgit:git-config[1] for more details.
++
 When the `--stdin` option is provided, the patterns are read from
 standard in as a newline-delimited list instead of from the arguments.
 +

diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index 679c1070368..314c8d61f80 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c

@@ -15,6 +15,7 @@
 #include "wt-status.h"
 #include "quote.h"
 #include "sparse-index.h"
+#include "worktree.h"
 
 static const char *empty_base = "";

@@ -359,26 +360,23 @@ enum sparse_checkout_mode {
 
 static int set_config(enum sparse_checkout_mode mode)
 {
-	const char *config_path;
-
-	if (upgrade_repository_format(1) < 0)
-		die(_("unable to upgrade repository format to enable worktreeConfig"));
-	if (git_config_set_gently("extensions.worktreeConfig", "true")) {
-		error(_("failed to set extensions.worktreeConfig setting"));
+	/* Update to use worktree config, if not already. */
+	if (init_worktree_config(the_repository)) {
+		error(_("failed to initialize worktree config"));
 		return 1;
 	}
 
-	config_path = git_path("config.worktree");
-	git_config_set_in_file_gently(config_path,
-				      "core.sparseCheckout",
-				      mode ? "true" : NULL);
-
-	git_config_set_in_file_gently(config_path,
-				      "core.sparseCheckoutCone",
-				      mode == MODE_CONE_PATTERNS ? "true" : NULL);
+	if (repo_config_set_worktree_gently(the_repository,
+					    "core.sparseCheckout",
+					    mode ? "true" : "false") ||
+	    repo_config_set_worktree_gently(the_repository,
+					    "core.sparseCheckoutCone",
+					    mode == MODE_CONE_PATTERNS ?
+						"true" : "false"))
+		return 1;
 
 	if (mode == MODE_NO_PATTERNS)
-		set_sparse_index_config(the_repository, 0);
+		return set_sparse_index_config(the_repository, 0);
 
 	return 0;
 }

diff --git a/sparse-index.c b/sparse-index.c
index a1d505d50e9..e93609999e0 100644
--- a/sparse-index.c
+++ b/sparse-index.c

@@ -99,13 +99,9 @@ static int convert_to_sparse_rec(struct index_state *istate,
 
 int set_sparse_index_config(struct repository *repo, int enable)
 {
-	int res;
-	char *config_path = repo_git_path(repo, "config.worktree");
-	res = git_config_set_in_file_gently(config_path,
-					    "index.sparse",
-					    enable ? "true" : NULL);
-	free(config_path);
-
+	int res = repo_config_set_worktree_gently(repo,
+						  "index.sparse",
+						  enable ? "true" : "false");
 	prepare_repo_settings(repo);
 	repo->settings.sparse_index = enable;
 	return res;

diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index 42776984fe7..be6ea4ffe33 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh

@@ -117,7 +117,7 @@ test_expect_success 'switching to cone mode with non-cone mode patterns' '
 		cd bad-patterns &&
 		git sparse-checkout init &&
 		git sparse-checkout add dir &&
-		git config core.sparseCheckoutCone true &&
+		git config --worktree core.sparseCheckoutCone true &&
 		test_must_fail git sparse-checkout add dir 2>err &&
 		grep "existing sparse-checkout patterns do not use cone mode" err
 	)

@@ -256,7 +256,7 @@ test_expect_success 'sparse-index enabled and disabled' '
 		test_cmp expect actual &&
 
 		git -C repo config --list >config &&
-		! grep index.sparse config
+		test_cmp_config -C repo false index.sparse
 	)
 '

-- 
gitgitgadget

[PATCH v5 5/5] worktree: copy sparse-checkout patterns and config on add

From: Derrick Stolee via GitGitGadget <hidden>
Date: 2022-01-31 15:01:13

From: Derrick Stolee <redacted>

When adding a new worktree, it is reasonable to expect that we want to
use the current set of sparse-checkout settings for that new worktree.
This is particularly important for repositories where the worktree would
become too large to be useful. This is even more important when using
partial clone as well, since we want to avoid downloading the missing
blobs for files that should not be written to the new worktree.

The only way to create such a worktree without this intermediate step of
expanding the full worktree is to copy the sparse-checkout patterns and
config settings during 'git worktree add'. Each worktree has its own
sparse-checkout patterns, and the default behavior when the
sparse-checkout file is missing is to include all paths at HEAD. Thus,
we need to have patterns from somewhere, they might as well be the
current worktree's patterns. These are then modified independently in
the future.

In addition to the sparse-checkout file, copy the worktree config file
if worktree config is enabled and the file exists. This will copy over
any important settings to ensure the new worktree behaves the same as
the current one. The only exception we must continue to make is that
core.bare and core.worktree should become unset in the worktree's config
file.

Signed-off-by: Derrick Stolee <redacted>
---
 builtin/worktree.c                 | 60 ++++++++++++++++++++++++++++++
 t/t1091-sparse-checkout-builtin.sh | 31 +++++++++++----
 t/t2400-worktree-add.sh            | 46 ++++++++++++++++++++++-
 3 files changed, 127 insertions(+), 10 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 2838254f7f2..dc9cd6decc8 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c

@@ -335,6 +335,66 @@ static int add_worktree(const char *path, const char *refname,
 	strbuf_addf(&sb, "%s/commondir", sb_repo.buf);
 	write_file(sb.buf, "../..");
 
+	/*
+	 * If the current worktree has sparse-checkout enabled, then copy
+	 * the sparse-checkout patterns from the current worktree.
+	 */
+	if (core_apply_sparse_checkout) {
+		char *from_file = git_pathdup("info/sparse-checkout");
+		char *to_file = xstrfmt("%s/worktrees/%s/info/sparse-checkout",
+					realpath.buf, name);
+
+		if (file_exists(from_file)) {
+			if (safe_create_leading_directories(to_file) ||
+			    copy_file(to_file, from_file, 0666))
+				error(_("failed to copy '%s' to '%s'; sparse-checkout may not work correctly"),
+				      from_file, to_file);
+		}
+
+		free(from_file);
+		free(to_file);
+	}
+
+	/*
+	 * If we are using worktree config, then copy all current config
+	 * values from the current worktree into the new one, that way the
+	 * new worktree behaves the same as this one.
+	 */
+	if (repository_format_worktree_config) {
+		char *from_file = git_pathdup("config.worktree");
+		char *to_file = xstrfmt("%s/worktrees/%s/config.worktree",
+					realpath.buf, name);
+
+		if (file_exists(from_file)) {
+			struct config_set cs = { { 0 }};
+			const char *str_value;
+			int bool_value;
+
+			if (safe_create_leading_directories(to_file) ||
+			    copy_file(to_file, from_file, 0666))
+				die(_("failed to copy worktree config from '%s' to '%s'"),
+				    from_file, to_file);
+
+			git_configset_init(&cs);
+			git_configset_add_file(&cs, from_file);
+
+			if (!git_configset_get_bool(&cs, "core.bare", &bool_value) &&
+			    bool_value &&
+			    git_config_set_multivar_in_file_gently(
+					to_file, "core.bare", NULL, "true", 0))
+				error(_("failed to unset 'core.bare' in '%s'"), to_file);
+			if (!git_configset_get_value(&cs, "core.worktree", &str_value) &&
+			    git_config_set_in_file_gently(to_file,
+							  "core.worktree", NULL))
+				error(_("failed to unset 'core.worktree' in '%s'"), to_file);
+
+			git_configset_clear(&cs);
+		}
+
+		free(from_file);
+		free(to_file);
+	}
+
 	strvec_pushf(&child_env, "%s=%s", GIT_DIR_ENVIRONMENT, sb_git.buf);
 	strvec_pushf(&child_env, "%s=%s", GIT_WORK_TREE_ENVIRONMENT, path);
 	cp.git_cmd = 1;

diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index be6ea4ffe33..8b92e307318 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh

@@ -146,9 +146,9 @@ test_expect_success 'interaction with clone --no-checkout (unborn index)' '
 '
 
 test_expect_success 'set enables config' '
-	git init empty-config &&
+	git init worktree-config &&
 	(
-		cd empty-config &&
+		cd worktree-config &&
 		test_commit test file &&
 		test_path_is_missing .git/config.worktree &&
 		git sparse-checkout set nothing &&

@@ -201,6 +201,21 @@ test_expect_success 'add to sparse-checkout' '
 	check_files repo "a folder1 folder2"
 '
 
+test_expect_success 'worktree: add copies sparse-checkout patterns' '
+	cat repo/.git/info/sparse-checkout >old &&
+	test_when_finished cp old repo/.git/info/sparse-checkout &&
+	test_when_finished git -C repo worktree remove ../worktree &&
+	git -C repo sparse-checkout set --no-cone "/*" &&
+	git -C repo worktree add --quiet ../worktree 2>err &&
+	test_must_be_empty err &&
+	new=repo/.git/worktrees/worktree/info/sparse-checkout &&
+	test_path_is_file $new &&
+	test_cmp repo/.git/info/sparse-checkout $new &&
+	git -C worktree sparse-checkout set --cone &&
+	test_cmp_config -C worktree true core.sparseCheckoutCone &&
+	test_must_fail git -C repo core.sparseCheckoutCone
+'
+
 test_expect_success 'cone mode: match patterns' '
 	git -C repo config --worktree core.sparseCheckoutCone true &&
 	rm -rf repo/a repo/folder1 repo/folder2 &&

@@ -520,13 +535,13 @@ test_expect_success 'interaction with submodules' '
 '
 
 test_expect_success 'different sparse-checkouts with worktrees' '
+	git -C repo sparse-checkout set --cone deep folder1 &&
 	git -C repo worktree add --detach ../worktree &&
-	check_files worktree "a deep folder1 folder2" &&
-	git -C worktree sparse-checkout init --cone &&
-	git -C repo sparse-checkout set folder1 &&
-	git -C worktree sparse-checkout set deep/deeper1 &&
-	check_files repo a folder1 &&
-	check_files worktree a deep
+	check_files worktree "a deep folder1" &&
+	git -C repo sparse-checkout set --cone folder1 &&
+	git -C worktree sparse-checkout set --cone deep/deeper1 &&
+	check_files repo "a folder1" &&
+	check_files worktree "a deep"
 '
 
 test_expect_success 'set using filename keeps file on-disk' '

diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
index 37ad79470fb..3fb5b21b943 100755
--- a/t/t2400-worktree-add.sh
+++ b/t/t2400-worktree-add.sh

@@ -165,8 +165,50 @@ test_expect_success '"add" default branch of a bare repo' '
 	(
 		git clone --bare . bare2 &&
 		cd bare2 &&
-		git worktree add ../there3 main
-	)
+		git worktree add ../there3 main &&
+		cd ../there3 &&
+		git status
+	) &&
+	cat >expect <<-EOF &&
+	init.t
+	EOF
+	ls there3 >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '"add" to bare repo with worktree config' '
+	(
+		git clone --bare . bare3 &&
+		cd bare3 &&
+		git config extensions.worktreeconfig true &&
+		git config --worktree core.bare true &&
+		git config --worktree core.worktree "$(pwd)" &&
+		git config --worktree bogus.key value &&
+		git config --unset core.bare &&
+		git worktree add ../there4 main &&
+		cd ../there4 &&
+		git status &&
+		git worktree add --detach ../there5 &&
+		cd ../there5 &&
+		git status
+	) &&
+
+	# the worktree has the arbitrary value copied.
+	test_cmp_config -C there4 value bogus.key &&
+	test_cmp_config -C there5 value bogus.key &&
+
+	# however, core.bare and core.worktree were removed.
+	test_must_fail git -C there4 config core.bare &&
+	test_must_fail git -C there4 config core.worktree &&
+
+	cat >expect <<-EOF &&
+	init.t
+	EOF
+
+	ls there4 >actual &&
+	test_cmp expect actual &&
+	ls there5 >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success 'checkout with grafts' '

-- 
gitgitgadget

Re: [PATCH v5 0/5] Sparse checkout: fix bug with worktree of bare repo

From: Elijah Newren <hidden>
Date: 2022-01-31 16:17:56

On Mon, Jan 31, 2022 at 7:01 AM Derrick Stolee via GitGitGadget
[off-list ref] wrote:

This series is now based on v2.35.0 since that contains all of the necessary
topics.

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 or core.worktree are set in the
common config file. This series fixes this, but also puts in place some
helpers to prevent this from happening in the future.

ATTENTION: I have significantly redesigned the series since previous
versions, so most of this cover letter is new.

 * Patch 1 updates documentation around extensions.worktreeConfig in a few
   places to improve discoverability. Several cross links are added to make
   it easy to find the related areas. (The documentation for the changes to
   'git sparse-checkout' are delayed to patch 4.)

 * Patch 2 introduces the init_worktree_config() helper which follows the
   documented instructions to enable extensions.worktreeConfig as well as
   move the core.bare and core.worktree config values. This update does not
   modify core.repositoryFormatVersion, since this is not needed
   specifically for extensions.worktreeConfig.

 * Patch 3 adds a new repo_config_set_worktree_gently() helper method so we
   can internally adjust a config value within a worktree, at least if
   extensions.worktreeConfig is enabled. (It will write to the common config
   file if the extension is not enabled.)

 * Patch 4 modifies the sparse-checkout builtin to use
   init_worktree_config() and repo_config_set_worktree_gently() in ways that
   fix the reported bug. The behavior change here is that it will no longer
   upgrade the repository format version, since that is not needed for
   extensions.worktreeConfig.

 * Patch 5 updates 'git worktree add' to copy the worktree config from the
   current worktree to the new one (while unsetting core.bare=true and
   core.worktree=*) along with copying the sparse-checkout patterns file.

[1]
https://lore.kernel.org/git/CABceR4bZmtC4rCwgxZ1BBYZP69VOUca1f_moJoP989vTUZWu9Q@mail.gmail.com/
[2]
https://lore.kernel.org/git/CAPig+cQ6U_yFw-X2OWrizB1rbCvc4bNxuSzKFzmoLNnm0GH8Eg@mail.gmail.com/


Updates in v5
=============

 * Cleaned up documentation as per Elijah's suggestions.
 * Removed unnecessary conflicting change in git-sparse-checkout.txt
 * Fixed an ambiguous comment about moving config values.

Thanks for patiently addressing all my feedback.  These last two
rounds look really good, and this one you've addressed all my feedback
and I can't find any issues:

Reviewed-by: Elijah Newren <redacted>

Updates in v4
=============

 * Rebased to v2.35.0
 * Fixed memory leak (was leaking repo_git_path() result)
 * Added additional documentation updates so curious users can discover the
   intricacies of extensions.worktreeConfig from multiple entry points.
 * Significantly reduced the amount of changes to config.c.
 * 'git sparse-checkout' no longer upgrades the repository format.
 * Dropped the update to upgrade_repository_format(), since it is not
   needed.
 * Dropped the 'git worktree init-worktree-config' subcommand in favor of a
   helper method called by 'git sparse-checkout'
 * Many others because of the significant changes required by the above
   items.

Thanks, -Stolee

Derrick Stolee (5):
  Documentation: add extensions.worktreeConfig details
  worktree: create init_worktree_config()
  config: add repo_config_set_worktree_gently()
  sparse-checkout: set worktree-config correctly
  worktree: copy sparse-checkout patterns and config on add

 Documentation/config/extensions.txt   | 31 ++++++++++++
 Documentation/git-config.txt          |  8 ++-
 Documentation/git-sparse-checkout.txt | 16 ++++--
 Documentation/git-worktree.txt        | 11 +++--
 builtin/sparse-checkout.c             | 28 +++++------
 builtin/worktree.c                    | 60 +++++++++++++++++++++++
 config.c                              | 35 ++++++++++++--
 config.h                              |  8 +++
 sparse-index.c                        | 10 ++--
 t/t1091-sparse-checkout-builtin.sh    | 35 ++++++++++----
 t/t2400-worktree-add.sh               | 46 +++++++++++++++++-
 worktree.c                            | 70 +++++++++++++++++++++++++++
 worktree.h                            | 21 ++++++++
 13 files changed, 333 insertions(+), 46 deletions(-)


base-commit: 89bece5c8c96f0b962cfc89e63f82d603fd60bed
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1101%2Fderrickstolee%2Fsparse-checkout%2Fbare-worktree-bug-v5
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1101/derrickstolee/sparse-checkout/bare-worktree-bug-v5
Pull-Request: https://github.com/gitgitgadget/git/pull/1101

Range-diff vs v4:

 1:  459e09dedd7 ! 1:  1bd5f26271c Documentation: add extensions.worktreeConfig details
     @@ Commit message
          within git-sparse-checkout.txt, but a behavior change is needed before
          making those updates.

     +    Helped-by: Elijah Newren [off-list ref]
          Signed-off-by: Derrick Stolee [off-list ref]

       ## Documentation/config/extensions.txt ##
     @@ Documentation/git-config.txt: from all available files.
      - present. If not it's the same as `--local`.
      + enabled. If not it's the same as `--local`. Note that `$GIT_DIR`
      + is equal to `$GIT_COMMON_DIR` for the main worktree, but is of the
     -+ form `.git/worktrees/<worktree-name>/` for other worktrees. See
     ++ form `$GIT_DIR/worktrees/<worktree-name>/` for other worktrees. See
      + linkgit:git-worktree[1] to learn how to enable
      + `extensions.worktreeConfig`.

     @@ Documentation/git-worktree.txt: CONFIGURATION FILE
      -already present in the config file, they will be applied to the main
      -working trees only.
      +present in the common config file and `extensions.worktreeConfig` is
     -+disabled, then they will be applied to the main working trees only.
     ++disabled, then they will be applied to the main working tree only.

       In order to have configuration specific to working trees, you can turn
       on the `worktreeConfig` extension, e.g.:
     @@ Documentation/git-worktree.txt: them to the `config.worktree` of the main workin
      - - `core.worktree` and `core.bare` should never be shared
      + - `core.worktree` should never be shared.
      +
     -+ - `core.bare` should not be shared unless the value is `core.bare=false`.
     ++ - `core.bare` should not be shared if the value is `core.bare=true`.

        - `core.sparseCheckout` is recommended per working tree, unless you
          are sure you always use sparse checkout for all working trees.
 2:  d262a76b448 ! 2:  2a2c350112e worktree: create init_worktree_config()
     @@ worktree.h: void strbuf_worktree_ref(const struct worktree *wt,
      + *
      + * 1. Add extensions.worktreeConfig=true in the common config file.
      + *
     -+ * 2. If the common config file has a core.worktree value or core.bare is
     -+ *    set to true, then those values are moved to the main worktree's
     -+ *    config.worktree file.
     ++ * 2. If the common config file has a core.worktree value, then that value
     ++ *    is moved to the main worktree's config.worktree file.
     ++ *
     ++ * 3. If the common config file has a core.bare enabled, then that value
     ++ *    is moved to the main worktree's config.worktree file.
      + *
      + * If extensions.worktreeConfig is already true, then this method
      + * terminates early without any of the above steps. The existing config
 3:  110d5e0546c = 3:  802b28a9510 config: add repo_config_set_worktree_gently()
 4:  fbfaa17797c ! 4:  08b89d17ccf sparse-checkout: set worktree-config correctly
     @@ Documentation/git-sparse-checkout.txt: COMMANDS
      - (extensions.worktreeConfig, core.sparseCheckout,
      - core.sparseCheckoutCone) if they are not already enabled, and
      - write a set of patterns to the sparse-checkout file from the
     -- list of arguments following the 'set' subcommand. Update the
     -- working directory to match the new patterns.
      + Enable the necessary sparse-checkout config settings
     -+ (`core.sparseCheckout` and possibly `core.sparseCheckoutCone`) if
     -+ they are not already enabled, and write a set of patterns to the
     -+ sparse-checkout file from the list of arguments following the
     -+ 'set' subcommand. Update the working directory to match the new
     -+ patterns.
     -++
     ++ (`core.sparseCheckout`, `core.sparseCheckoutCone`, and
     ++ `index.sparse`) if they are not already set to the desired values,
     ++ and write a set of patterns to the sparse-checkout file from the
     +  list of arguments following the 'set' subcommand. Update the
     +  working directory to match the new patterns.
     + +
      +To ensure that adjusting the sparse-checkout settings within a worktree
      +does not alter the sparse-checkout settings in other worktrees, the 'set'
      +subcommand will upgrade your repository config to use worktree-specific
     @@ Documentation/git-sparse-checkout.txt: COMMANDS
      +the 'set' subcommand are stored in the worktree-specific sparse-checkout
      +file. See linkgit:git-worktree[1] and the documentation of
      +`extensions.worktreeConfig` in linkgit:git-config[1] for more details.
     - +
     +++
       When the `--stdin` option is provided, the patterns are read from
       standard in as a newline-delimited list instead of from the arguments.
     -@@ Documentation/git-sparse-checkout.txt: interact with your repository until it is disabled.
     -  By default, these patterns are read from the command-line arguments,
     -  but they can be read from stdin using the `--stdin` option. When
     -  `core.sparseCheckoutCone` is enabled, the given patterns are interpreted
     -- as directory names as in the 'set' subcommand.
     -+ as directory names as in the 'set' subcommand. The sparsity defined
     -+ by the arguments to the 'add' subcommand are added to the patterns
     -+ in the worktree-specific sparse-checkout file.
     -
     - 'reapply'::
     -  Reapply the sparsity pattern rules to paths in the working tree.
     + +

       ## builtin/sparse-checkout.c ##
      @@
 5:  bb9e550ff3d ! 5:  85779dfaed3 worktree: copy sparse-checkout patterns and config on add
     @@ t/t1091-sparse-checkout-builtin.sh: test_expect_success 'add to sparse-checkout'
      + cat repo/.git/info/sparse-checkout >old &&
      + test_when_finished cp old repo/.git/info/sparse-checkout &&
      + test_when_finished git -C repo worktree remove ../worktree &&
     -+ git -C repo sparse-checkout set "/*" &&
     ++ git -C repo sparse-checkout set --no-cone "/*" &&
      + git -C repo worktree add --quiet ../worktree 2>err &&
      + test_must_be_empty err &&
      + new=repo/.git/worktrees/worktree/info/sparse-checkout &&

--
gitgitgadget

Re: [PATCH v5 1/5] Documentation: add extensions.worktreeConfig details

From: Eric Sunshine <hidden>
Date: 2022-02-06 09:23:50

On Mon, Jan 31, 2022 at 10:01 AM Derrick Stolee via GitGitGadget
[off-list ref] wrote:

[...]
Expand the documentation to help users discover the complexities of
extensions.worktreeConfig by adding details and cross links in these
locations (relative to Documentation/):
[...]
Signed-off-by: Derrick Stolee <redacted>

A few minor comments, which can be addressed later or not at all, and
likely are not worth holding up the series...

quoted hunk

diff --git a/Documentation/config/extensions.txt b/Documentation/config/extensions.txt

@@ -6,3 +6,34 @@ extensions.objectFormat::
+extensions.worktreeConfig::
+       If enabled, then worktrees will load config settings from the
+       `$GIT_DIR/config.worktree` file in addition to the
+       `$GIT_COMMON_DIR/config` file. Note that `$GIT_COMMON_DIR` and
+       `$GIT_DIR` are the same for the main worktree, while other
+       worktrees have `$GIT_DIR` equal to
+       `$GIT_COMMON_DIR/worktrees/<worktree-name>/`. The settings in the
+       `config.worktree` file will override settings from any other
+       config files.

There have been some efforts[1][2] in the past to settle upon the term
"working tree" instead of "worktree" when talking about worktrees in
prose. (The term "worktree" is perfectly fine in paths, such as
`.git/worktrees/`, and in the command name `git worktree`, of course.)

Documentation/git-worktree.txt calls it "<id>" rather than
"<worktree-name>" since it is a unique identifier for the worktree
which may or may not match the worktree's basename.

Documentation/git-worktree.txt talks simply about the path
`.git/worktrees/` under the assumption that people will understand
that `.git/` is the repository's common directory (which may not even
be named `.git/` for a bare repository). Although saying
$GIT_COMMON_DIR is certainly technically accurate, the simpler `.git/`
doesn't seem to have caused any consternation. (Not a big deal. I
mention it only to highlight the inconsistency between the existing
and new documentation added here.)

The same comments apply to the rest of the patch.

[1]: bc483285b7 (Documentation/git-worktree: consistently use term
"linked working tree", 2015-07-20)
[2]: 4f375b2678 (git-worktree.txt: consistently use term "working
tree", 2020-08-03)

Re: [PATCH v5 2/5] worktree: create init_worktree_config()

From: Eric Sunshine <hidden>
Date: 2022-02-06 09:32:18

On Mon, Jan 31, 2022 at 10:01 AM Derrick Stolee via GitGitGadget
[off-list ref] wrote:

quoted hunk

[...]
Create a helper method, init_worktree_config(), that will be used in a
later change to fix this behavior within 'git sparse-checkout set'. The
method is carefully documented in worktree.h.
[...]
Signed-off-by: Derrick Stolee <redacted>
---

diff --git a/worktree.c b/worktree.c

@@ -826,3 +827,72 @@ int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath,
+int init_worktree_config(struct repository *r)
+{
+       int res = 0;
+       int bare = 0;
+       struct config_set cs = { { 0 } };
+       const char *core_worktree;
+       char *common_config_file = xstrfmt("%s/config", r->commondir);
+       char *main_worktree_file = xstrfmt("%s/config.worktree", r->commondir);
+
+       /*
+        * If the extension is already enabled, then we can skip the
+        * upgrade process.
+        */
+       if (repository_format_worktree_config)
+               return 0;
+       if ((res = git_config_set_gently("extensions.worktreeConfig", "true")))
+               return error(_("failed to set extensions.worktreeConfig setting"));

These two early returns are leaking `common_config_file` and
`main_worktree_file` which have already been allocated by xstrfmt().

+
+       git_configset_init(&cs);
+       git_configset_add_file(&cs, common_config_file);
+
+       /*
+        * 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.
+        */

The terminology "base worktree", which is not otherwise in the
project's lexicon, seems to be a leftover from earlier versions of
this series. Perhaps it could say instead "the repository-specific
configuration in $GIT_COMMON_DIR/worktree.config" or something?

+       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.
+        */

s/base worktree/main worktree/

+       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;
+       }
+
+       /*
+        * Ensure that we use worktree config for the remaining lifetime
+        * of the current process.
+        */
+       repository_format_worktree_config = 1;
+
+cleanup:
+       git_configset_clear(&cs);
+       free(common_config_file);
+       free(main_worktree_file);
+       return res;
+}

Re: [PATCH v5 4/5] sparse-checkout: set worktree-config correctly

From: Eric Sunshine <hidden>
Date: 2022-02-06 10:21:18

On Mon, Jan 31, 2022 at 10:01 AM Derrick Stolee via GitGitGadget
[off-list ref] wrote:

The previous change added repo_config_set_worktree_gently() to assist
writing config values into the config.worktree file, if enabled. An
earlier change added init_worktree_config() as a helper to initialize
extensions.worktreeConfig if not already enabled.

Let the sparse-checkout builtin use these helpers instead of attempting to
initialize the worktree config on its own. This changes behavior of 'git
sparse-checkout set' in a few important ways:

 1. Git will no longer upgrade the repository format, since this is not
    a requirement for understanding extensions.worktreeConfig.

 2. If the main worktree is bare, then this command will not put the
    worktree in a broken state.

Although the three of four of us involved in this discussion
understand what "broken state" means, such a description may be too
vague for future readers. To help them out, perhaps we can do a better
job of conveying the nature of the actual breakage by rewriting all of
the above like this:

    `git sparse-checkout set/init` enables worktree-specific
    configuration[*] by setting extensions.worktreeConfig=true, but
    neglects to perform the additional necessary bookkeeping of
    relocating `core.bare=true` and `core.worktree` from
    $GIT_COMMON_DIR/config to $GIT_COMMON_DIR/config.worktree, as
    documented in git-worktree.txt. As a result of this oversight,
    these settings, which are nonsensical for secondary worktrees, can
    cause Git commands to incorrectly consider a worktree bare (in the
    case of `core.bare`) or operate on the wrong worktree (in the case
    of `core.worktree`). Fix this problem by taking advantage of the
    recently-added init_worktree_config() which enables
    `extensions.worktreeConfig` and takes care of necessary
    bookkeeping.

    While at it, for backward-compatibility reasons, also stop
    upgrading the repository format to "1" since doing so is
    (unintentionally) not required to take advantage of
    `extensions.worktreeConfig`, as explained by 11664196ac ("Revert
    "check_repository_format_gently(): refuse extensions for old
    repositories"", 2020-07-15).

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.

Perhaps this paragraph can become the "[*]" footnote I referenced in
the above rewrite.

Reported-by: Sean Allred <redacted>
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Derrick Stolee <redacted>

Re: [PATCH v5 5/5] worktree: copy sparse-checkout patterns and config on add

From: Jean-Noël AVILA <hidden>
Date: 2022-02-06 10:37:01

On Monday, 31 January 2022 16:00:59 CET Derrick Stolee via GitGitGadget wrote:

quoted hunk

From: Derrick Stolee <redacted>

When adding a new worktree, it is reasonable to expect that we want to
use the current set of sparse-checkout settings for that new worktree.
This is particularly important for repositories where the worktree would
become too large to be useful. This is even more important when using
partial clone as well, since we want to avoid downloading the missing
blobs for files that should not be written to the new worktree.

The only way to create such a worktree without this intermediate step of
expanding the full worktree is to copy the sparse-checkout patterns and
config settings during 'git worktree add'. Each worktree has its own
sparse-checkout patterns, and the default behavior when the
sparse-checkout file is missing is to include all paths at HEAD. Thus,
we need to have patterns from somewhere, they might as well be the
current worktree's patterns. These are then modified independently in
the future.

In addition to the sparse-checkout file, copy the worktree config file
if worktree config is enabled and the file exists. This will copy over
any important settings to ensure the new worktree behaves the same as
the current one. The only exception we must continue to make is that
core.bare and core.worktree should become unset in the worktree's config
file.

Signed-off-by: Derrick Stolee <redacted>
---
 builtin/worktree.c                 | 60 ++++++++++++++++++++++++++++++
 t/t1091-sparse-checkout-builtin.sh | 31 +++++++++++----
 t/t2400-worktree-add.sh            | 46 ++++++++++++++++++++++-
 3 files changed, 127 insertions(+), 10 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 2838254f7f2..dc9cd6decc8 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c

@@ -335,6 +335,66 @@ static int add_worktree(const char *path, const char *refname,
 	strbuf_addf(&sb, "%s/commondir", sb_repo.buf);
 	write_file(sb.buf, "../..");
 
+	/*
+	 * If the current worktree has sparse-checkout enabled, then copy
+	 * the sparse-checkout patterns from the current worktree.
+	 */
+	if (core_apply_sparse_checkout) {
+		char *from_file = git_pathdup("info/sparse-checkout");
+		char *to_file = xstrfmt("%s/worktrees/%s/info/sparse-checkout",
+					realpath.buf, name);
+
+		if (file_exists(from_file)) {
+			if (safe_create_leading_directories(to_file) ||
+			    copy_file(to_file, from_file, 0666))
+				error(_("failed to copy '%s' to '%s'; sparse-checkout may not work correctly"),
+				      from_file, to_file);
+		}
+
+		free(from_file);
+		free(to_file);
+	}
+
+	/*
+	 * If we are using worktree config, then copy all current config
+	 * values from the current worktree into the new one, that way the
+	 * new worktree behaves the same as this one.
+	 */
+	if (repository_format_worktree_config) {
+		char *from_file = git_pathdup("config.worktree");
+		char *to_file = xstrfmt("%s/worktrees/%s/config.worktree",
+					realpath.buf, name);
+
+		if (file_exists(from_file)) {
+			struct config_set cs = { { 0 }};
+			const char *str_value;
+			int bool_value;
+
+			if (safe_create_leading_directories(to_file) ||
+			    copy_file(to_file, from_file, 0666))
+				die(_("failed to copy worktree config from '%s' to '%s'"),
+				    from_file, to_file);
+
+			git_configset_init(&cs);
+			git_configset_add_file(&cs, from_file);
+
+			if (!git_configset_get_bool(&cs, "core.bare", &bool_value) &&
+			    bool_value &&
+			    git_config_set_multivar_in_file_gently(
+					to_file, "core.bare", NULL, "true", 0))
+				error(_("failed to unset 'core.bare' in '%s'"), to_file);
+			if (!git_configset_get_value(&cs, "core.worktree", &str_value) &&
+			    git_config_set_in_file_gently(to_file,
+							  "core.worktree", NULL))
+				error(_("failed to unset 'core.worktree' in '%s'"), to_file);
+

In the first patch of this series, you use _("unable to set '%s' in'%s'). Does it make sense to reuse this string here?

Re: [PATCH v5 5/5] worktree: copy sparse-checkout patterns and config on add

From: Eric Sunshine <hidden>
Date: 2022-02-06 11:30:20

On Mon, Jan 31, 2022 at 10:01 AM Derrick Stolee via GitGitGadget
[off-list ref] wrote:

quoted hunk

When adding a new worktree, it is reasonable to expect that we want to
use the current set of sparse-checkout settings for that new worktree.
This is particularly important for repositories where the worktree would
become too large to be useful. This is even more important when using
partial clone as well, since we want to avoid downloading the missing
blobs for files that should not be written to the new worktree.

The only way to create such a worktree without this intermediate step of
expanding the full worktree is to copy the sparse-checkout patterns and
config settings during 'git worktree add'. Each worktree has its own
sparse-checkout patterns, and the default behavior when the
sparse-checkout file is missing is to include all paths at HEAD. Thus,
we need to have patterns from somewhere, they might as well be the
current worktree's patterns. These are then modified independently in
the future.

In addition to the sparse-checkout file, copy the worktree config file
if worktree config is enabled and the file exists. This will copy over
any important settings to ensure the new worktree behaves the same as
the current one. The only exception we must continue to make is that
core.bare and core.worktree should become unset in the worktree's config
file.

Signed-off-by: Derrick Stolee <redacted>
---

diff --git a/builtin/worktree.c b/builtin/worktree.c

@@ -335,6 +335,66 @@ static int add_worktree(const char *path, const char *refname,
+       /*
+        * If the current worktree has sparse-checkout enabled, then copy
+        * the sparse-checkout patterns from the current worktree.
+        */
+       if (core_apply_sparse_checkout) {
+               char *from_file = git_pathdup("info/sparse-checkout");
+               char *to_file = xstrfmt("%s/worktrees/%s/info/sparse-checkout",
+                                       realpath.buf, name);

I think this is too fragile and may easily be broken by an unrelated
change since `realpath` is a temporary container which gets reused,
thus it holds different paths at different times. For instance, it
first holds the realpath of $GIT_DIR but then later holds the realpath
of $GIT_COMMON_DIR. If someone later comes along and reuses it for
some other path, then the code added by this patch may end up
breaking. To make this robust, you should instead use `sb_repo` which
already has the value "$GIT_DIR/worktrees/<name>":

    char *to_file = xstrfmt(%s/info/sparse-checkout", sb_repo.buf);

+               if (file_exists(from_file)) {
+                       if (safe_create_leading_directories(to_file) ||
+                           copy_file(to_file, from_file, 0666))
+                               error(_("failed to copy '%s' to '%s'; sparse-checkout may not work correctly"),
+                                     from_file, to_file);
+               }
+
+               free(from_file);
+               free(to_file);
+       }
+
+       /*
+        * If we are using worktree config, then copy all current config
+        * values from the current worktree into the new one, that way the
+        * new worktree behaves the same as this one.
+        */

As an aid for future readers, it might be helpful to extend this
content to explain _why_ the new worktree should behave the same as
the current one. Reproducing the important bits from the commit
message here in the comment could make it more useful.

+       if (repository_format_worktree_config) {
+               char *from_file = git_pathdup("config.worktree");
+               char *to_file = xstrfmt("%s/worktrees/%s/config.worktree",
+                                       realpath.buf, name);

Same comment about fragility of using `realpath`. Instead:

    char *to_file = xstrfmt("%s/config.worktree", sb_repo.buf);

+               if (file_exists(from_file)) {
+                       struct config_set cs = { { 0 }};

s/}}/} }/

+                       const char *str_value;
+                       int bool_value;
+
+                       if (safe_create_leading_directories(to_file) ||
+                           copy_file(to_file, from_file, 0666))
+                               die(_("failed to copy worktree config from '%s' to '%s'"),
+                                   from_file, to_file);

All the other error conditions handled by this patch use error() but
this one uses die(). Does this one really warrant aborting the `git
worktree add` operation? Perhaps instead just add a label below this
new chunk of code and `goto` the label (or indent this code further
and avoid adding a label).

+                       git_configset_init(&cs);
+                       git_configset_add_file(&cs, from_file);
+
+                       if (!git_configset_get_bool(&cs, "core.bare", &bool_value) &&
+                           bool_value &&

Nit: This would be slightly easier to understand if you named this
variable `bare` (as you did in patch [2/5]) rather than `bool_value`.

+                           git_config_set_multivar_in_file_gently(
+                                       to_file, "core.bare", NULL, "true", 0))
+                               error(_("failed to unset 'core.bare' in '%s'"), to_file);
+                       if (!git_configset_get_value(&cs, "core.worktree", &str_value) &&

In patch [2/5] you used git_configset_get_string_tmp() to retrieve
this setting, but here you're using git_configset_get_value(). Is
there a reason for the inconsistency?

quoted hunk

+                           git_config_set_in_file_gently(to_file,
+                                                         "core.worktree", NULL))
+                               error(_("failed to unset 'core.worktree' in '%s'"), to_file);
+
+                       git_configset_clear(&cs);
+               }
+
+               free(from_file);
+               free(to_file);
+       }

diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh

@@ -201,6 +201,21 @@ test_expect_success 'add to sparse-checkout' '
+test_expect_success 'worktree: add copies sparse-checkout patterns' '
+       cat repo/.git/info/sparse-checkout >old &&
+       test_when_finished cp old repo/.git/info/sparse-checkout &&
+       test_when_finished git -C repo worktree remove ../worktree &&
+       git -C repo sparse-checkout set --no-cone "/*" &&
+       git -C repo worktree add --quiet ../worktree 2>err &&
+       test_must_be_empty err &&
+       new=repo/.git/worktrees/worktree/info/sparse-checkout &&

For robustness, this should be using:

    new=$(git rev-parse --git-path info/sparse-checkout)

to retrieve ".git/worktrees/<id>/info/sparse-checkout" rather than
hard-coding "worktree" for "<id>".

quoted hunk

+       test_path_is_file $new &&
+       test_cmp repo/.git/info/sparse-checkout $new &&
+       git -C worktree sparse-checkout set --cone &&
+       test_cmp_config -C worktree true core.sparseCheckoutCone &&
+       test_must_fail git -C repo core.sparseCheckoutCone
+'

diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
index 37ad79470fb..3fb5b21b943 100755
--- a/t/t2400-worktree-add.sh
+++ b/t/t2400-worktree-add.sh

@@ -165,8 +165,50 @@ test_expect_success '"add" default branch of a bare repo' '
        (
                git clone --bare . bare2 &&
                cd bare2 &&
-               git worktree add ../there3 main
-       )
+               git worktree add ../there3 main &&
+               cd ../there3 &&
+               git status
+       ) &&

Is this some debugging code you forgot to remove or was `git status`
failing due to the bug(s) fixed by this patch series? I'm guessing the
latter since you also use `git status` in more tests below. Anyhow,
it's not very clear what the `git-status` is meant to be testing. An
in-code comment _might_ help. Even better, perhaps, would be to add a
new single-purpose test or a well-named function which explicitly
checks the conditions you want to test (i.e. that git-config doesn't
report core.bare as true or core.worktree as having a value).

+       cat >expect <<-EOF &&
+       init.t
+       EOF
+       ls there3 >actual &&
+       test_cmp expect actual
+'
+
+test_expect_success '"add" to bare repo with worktree config' '
+       (
+               git clone --bare . bare3 &&
+               cd bare3 &&
+               git config extensions.worktreeconfig true &&
+               git config --worktree core.bare true &&
+               git config --worktree core.worktree "$(pwd)" &&

It's not clear to the casual reader why these nonsensical keys are
being added. An in-code comment explaining the reason (i.e. you expect
the `git worktree add` operation to drop them in the new worktree)
would be beneficial for future readers.

+               git config --worktree bogus.key value &&
+               git config --unset core.bare &&

Why is this being unset? (Genuine question. Am I missing something obvious?)

+               git worktree add ../there4 main &&
+               cd ../there4 &&
+               git status &&
+               git worktree add --detach ../there5 &&
+               cd ../there5 &&
+               git status
+       ) &&

Same comment about the purpose of git-status not being obvious. A
well-named function which checks the specific conditions you're
looking for would be more clear than abstruse invocations of
git-status.

+       # the worktree has the arbitrary value copied.
+       test_cmp_config -C there4 value bogus.key &&
+       test_cmp_config -C there5 value bogus.key &&
+
+       # however, core.bare and core.worktree were removed.
+       test_must_fail git -C there4 config core.bare &&
+       test_must_fail git -C there4 config core.worktree &&

This is the comment I was looking for above. It's certainly okay to
have it here, but it was confusing above to not understand the reason
those nonsensical keys were being added.

+       cat >expect <<-EOF &&
+       init.t
+       EOF
+
+       ls there4 >actual &&
+       test_cmp expect actual &&
+       ls there5 >actual &&
+       test_cmp expect actual
 '

Re: [PATCH v5 5/5] worktree: copy sparse-checkout patterns and config on add

From: Eric Sunshine <hidden>
Date: 2022-02-06 19:36:56

On Sun, Feb 6, 2022 at 6:30 AM Eric Sunshine [off-list ref] wrote:

On Mon, Jan 31, 2022 at 10:01 AM Derrick Stolee via GitGitGadget
[off-list ref] wrote:

quoted

+       new=repo/.git/worktrees/worktree/info/sparse-checkout &&

For robustness, this should be using:

    new=$(git rev-parse --git-path info/sparse-checkout)

to retrieve ".git/worktrees/<id>/info/sparse-checkout" rather than
hard-coding "worktree" for "<id>".

Of course, I mean to type:

    new=$(git -C worktree rev-parse --git-path info/sparse-checkout)

Re: [PATCH v5 5/5] worktree: copy sparse-checkout patterns and config on add

From: Derrick Stolee <hidden>
Date: 2022-02-07 14:29:58

On 2/6/2022 5:36 AM, Jean-Noël AVILA wrote:

On Monday, 31 January 2022 16:00:59 CET Derrick Stolee via GitGitGadget wrote:

Hi Jean-Noël. Thanks for your attention to the translatable messages
here:

quoted

error(_("failed to copy '%s' to '%s'; sparse-checkout may not work correctly"),
      from_file, to_file);

quoted

die(_("failed to copy worktree config from '%s' to '%s'"),
    from_file, to_file);

quoted

error(_("failed to unset 'core.bare' in '%s'"), to_file);

quoted

error(_("failed to unset 'core.worktree' in '%s'"), to_file);

In the first patch of this series, you use _("unable to set '%s' in'%s'). Does it make sense to reuse this string here?

I would argue that "unable to set" is not appropriate for any of these
messages. Perhaps the "failed to copy" messages might be able to use
"unable to set", but the information that the config setting is coming
from settings the user controlled is valuable.

The "failed to unset" means "we are trying to _remove_ this setting
from the config file", so "unable to set" does not seem to work here.

I'm open to revisiting this if you disagree.

Thanks,
-Stolee

Re: [PATCH v5 5/5] worktree: copy sparse-checkout patterns and config on add

From: Derrick Stolee <hidden>
Date: 2022-02-07 14:51:40

On 2/6/2022 6:30 AM, Eric Sunshine wrote:

On Mon, Jan 31, 2022 at 10:01 AM Derrick Stolee via GitGitGadget
[off-list ref] wrote:

quoted

+       /*
+        * If we are using worktree config, then copy all current config
+        * values from the current worktree into the new one, that way the
+        * new worktree behaves the same as this one.
+        */

As an aid for future readers, it might be helpful to extend this
content to explain _why_ the new worktree should behave the same as
the current one. Reproducing the important bits from the commit
message here in the comment could make it more useful.

Here, I think I disagree. The comment is intentionally short to say
"we need to be careful here" but the reason behind it can be found
in the commit message from history spelunking.

quoted

+                           git_config_set_multivar_in_file_gently(
+                                       to_file, "core.bare", NULL, "true", 0))
+                               error(_("failed to unset 'core.bare' in '%s'"), to_file);
+                       if (!git_configset_get_value(&cs, "core.worktree", &str_value) &&

In patch [2/5] you used git_configset_get_string_tmp() to retrieve
this setting, but here you're using git_configset_get_value(). Is
there a reason for the inconsistency?

I'm not sure why I chose to use one over the other, but looking at
the code, it seems that my use in patch 2 is the only use of
git_configset_get_string_tmp() that is not internal to config.c.

I should convert the one in patch 2 to use git_configset_get_value()
and then we can remove the declaration of git_configset_get_string_tmp()
in config.h.

quoted

+               git worktree add ../there3 main &&
+               cd ../there3 &&
+               git status
+       ) &&

Is this some debugging code you forgot to remove or was `git status`
failing due to the bug(s) fixed by this patch series? I'm guessing the
latter since you also use `git status` in more tests below. Anyhow,
it's not very clear what the `git-status` is meant to be testing. An
in-code comment _might_ help. Even better, perhaps, would be to add a
new single-purpose test or a well-named function which explicitly
checks the conditions you want to test (i.e. that git-config doesn't
report core.bare as true or core.worktree as having a value).

Basically, in the old code any Git command would immediately fail
because of the interpretation of core.bare or core.worktree. So
this check is just a check that a basic Git command doesn't fail

quoted

+               git config --worktree bogus.key value &&
+               git config --unset core.bare &&

Why is this being unset? (Genuine question. Am I missing something obvious?)

I'm moving it out of the common config file. Earlier commands
enabled it in the config.worktree file for this working tree.

Thanks,
-Stolee

[PATCH v6 0/6] Sparse checkout: fix bug with worktree of bare repo

From: Derrick Stolee via GitGitGadget <hidden>
Date: 2022-02-07 21:33:10

This series is now based on v2.35.0 since that contains all of the necessary
topics.

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 or core.worktree are set in the
common config file. This series fixes this, but also puts in place some
helpers to prevent this from happening in the future.

ATTENTION: I have significantly redesigned the series since previous
versions, so most of this cover letter is new.

 * Patch 1 updates documentation around extensions.worktreeConfig in a few
   places to improve discoverability. Several cross links are added to make
   it easy to find the related areas. (The documentation for the changes to
   'git sparse-checkout' are delayed to patch 4.)

 * Patch 2 introduces the init_worktree_config() helper which follows the
   documented instructions to enable extensions.worktreeConfig as well as
   move the core.bare and core.worktree config values. This update does not
   modify core.repositoryFormatVersion, since this is not needed
   specifically for extensions.worktreeConfig.

 * Patch 3 adds a new repo_config_set_worktree_gently() helper method so we
   can internally adjust a config value within a worktree, at least if
   extensions.worktreeConfig is enabled. (It will write to the common config
   file if the extension is not enabled.)

 * Patch 4 modifies the sparse-checkout builtin to use
   init_worktree_config() and repo_config_set_worktree_gently() in ways that
   fix the reported bug. The behavior change here is that it will no longer
   upgrade the repository format version, since that is not needed for
   extensions.worktreeConfig.

 * Patch 5 updates 'git worktree add' to copy the worktree config from the
   current worktree to the new one (while unsetting core.bare=true and
   core.worktree=*) along with copying the sparse-checkout patterns file.

[1]
https://lore.kernel.org/git/CABceR4bZmtC4rCwgxZ1BBYZP69VOUca1f_moJoP989vTUZWu9Q@mail.gmail.com/
[2]
https://lore.kernel.org/git/CAPig+cQ6U_yFw-X2OWrizB1rbCvc4bNxuSzKFzmoLNnm0GH8Eg@mail.gmail.com/


Updates in v6
=============

 * Updated documentation to use "working tree" over "worktree" and "" over
   ""
 * Delay some allocations to avoid leaking memory in error conditions.
 * Use "main worktree" over "base worktree" in comments.
 * 


Updates in v5
=============

Responded to most of Eric's suggestions. I pushed back on one about a
comment including information from the commit message, but everything else
should be as Eric suggested.

 * Cleaned up documentation as per Elijah's suggestions.
 * Removed unnecessary conflicting change in git-sparse-checkout.txt
 * Fixed an ambiguous comment about moving config values.
 * Removed use of git_configset_get_string_tmp() and added a patch that
   removes its public declaration.
 * Fragile variables are replaced with better ones.
 * Variable names and code style improved.
 * Several test cleanups in patch 5.


Updates in v4
=============

 * Rebased to v2.35.0
 * Fixed memory leak (was leaking repo_git_path() result)
 * Added additional documentation updates so curious users can discover the
   intricacies of extensions.worktreeConfig from multiple entry points.
 * Significantly reduced the amount of changes to config.c.
 * 'git sparse-checkout' no longer upgrades the repository format.
 * Dropped the update to upgrade_repository_format(), since it is not
   needed.
 * Dropped the 'git worktree init-worktree-config' subcommand in favor of a
   helper method called by 'git sparse-checkout'
 * Many others because of the significant changes required by the above
   items.

Thanks, -Stolee

Derrick Stolee (6):
  Documentation: add extensions.worktreeConfig details
  worktree: create init_worktree_config()
  config: add repo_config_set_worktree_gently()
  sparse-checkout: set worktree-config correctly
  worktree: copy sparse-checkout patterns and config on add
  config: make git_configset_get_string_tmp() private

 Documentation/config/extensions.txt   | 31 ++++++++++++
 Documentation/git-config.txt          |  8 ++-
 Documentation/git-sparse-checkout.txt | 16 ++++--
 Documentation/git-worktree.txt        | 11 ++--
 builtin/sparse-checkout.c             | 28 +++++-----
 builtin/worktree.c                    | 63 +++++++++++++++++++++++
 config.c                              | 39 ++++++++++++--
 config.h                              |  9 +++-
 sparse-index.c                        | 10 ++--
 t/t1091-sparse-checkout-builtin.sh    | 35 +++++++++----
 t/t2400-worktree-add.sh               | 58 ++++++++++++++++++++-
 worktree.c                            | 73 +++++++++++++++++++++++++++
 worktree.h                            | 21 ++++++++
 13 files changed, 353 insertions(+), 49 deletions(-)


base-commit: 89bece5c8c96f0b962cfc89e63f82d603fd60bed
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1101%2Fderrickstolee%2Fsparse-checkout%2Fbare-worktree-bug-v6
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1101/derrickstolee/sparse-checkout/bare-worktree-bug-v6
Pull-Request: https://github.com/gitgitgadget/git/pull/1101

Range-diff vs v5:

 1:  1bd5f26271c ! 1:  0260ff6cac0 Documentation: add extensions.worktreeConfig details
     @@ Documentation/config/extensions.txt: extensions.objectFormat::
      +	If enabled, then worktrees will load config settings from the
      +	`$GIT_DIR/config.worktree` file in addition to the
      +	`$GIT_COMMON_DIR/config` file. Note that `$GIT_COMMON_DIR` and
     -+	`$GIT_DIR` are the same for the main worktree, while other
     -+	worktrees have `$GIT_DIR` equal to
     -+	`$GIT_COMMON_DIR/worktrees/<worktree-name>/`. The settings in the
     ++	`$GIT_DIR` are the same for the main working tree, while other
     ++	working trees have `$GIT_DIR` equal to
     ++	`$GIT_COMMON_DIR/worktrees/<id>/`. The settings in the
      +	`config.worktree` file will override settings from any other
      +	config files.
      ++
      +When enabling `extensions.worktreeConfig`, you must be careful to move
     -+certain values from the common config file to the main worktree's
     ++certain values from the common config file to the main working tree's
      +`config.worktree` file, if present:
      ++
      +* `core.worktree` must be moved from `$GIT_COMMON_DIR/config` to
     @@ Documentation/git-config.txt: from all available files.
       	read from or written to if `extensions.worktreeConfig` is
      -	present. If not it's the same as `--local`.
      +	enabled. If not it's the same as `--local`. Note that `$GIT_DIR`
     -+	is equal to `$GIT_COMMON_DIR` for the main worktree, but is of the
     -+	form `$GIT_DIR/worktrees/<worktree-name>/` for other worktrees. See
     ++	is equal to `$GIT_COMMON_DIR` for the main working tree, but is of
     ++	the form `$GIT_DIR/worktrees/<id>/` for other working trees. See
      +	linkgit:git-worktree[1] to learn how to enable
      +	`extensions.worktreeConfig`.
       
 2:  2a2c350112e ! 2:  5d0cc242d92 worktree: create init_worktree_config()
     @@ worktree.c: int should_prune_worktree(const char *id, struct strbuf *reason, cha
      +	int bare = 0;
      +	struct config_set cs = { { 0 } };
      +	const char *core_worktree;
     -+	char *common_config_file = xstrfmt("%s/config", r->commondir);
     -+	char *main_worktree_file = xstrfmt("%s/config.worktree", r->commondir);
     ++	char *common_config_file;
     ++	char *main_worktree_file;
      +
      +	/*
      +	 * If the extension is already enabled, then we can skip the
     @@ worktree.c: int should_prune_worktree(const char *id, struct strbuf *reason, cha
      +	if ((res = git_config_set_gently("extensions.worktreeConfig", "true")))
      +		return error(_("failed to set extensions.worktreeConfig setting"));
      +
     ++	common_config_file = xstrfmt("%s/config", r->commondir);
     ++	main_worktree_file = xstrfmt("%s/config.worktree", r->commondir);
     ++
      +	git_configset_init(&cs);
      +	git_configset_add_file(&cs, common_config_file);
      +
      +	/*
      +	 * 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
     ++	 * move it to the main 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.
      +	 */
     @@ worktree.c: int should_prune_worktree(const char *id, struct strbuf *reason, cha
      +			goto cleanup;
      +	}
      +	/*
     -+	 * If core.worktree is set, then the base worktree is located
     ++	 * If core.worktree is set, then the main 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 (!git_configset_get_value(&cs, "core.worktree", &core_worktree)) {
      +		if ((res = move_config_setting("core.worktree", core_worktree,
      +					       common_config_file,
      +					       main_worktree_file)))
 3:  802b28a9510 = 3:  cf9e86fe3a4 config: add repo_config_set_worktree_gently()
 4:  08b89d17ccf ! 4:  5b5924eab49 sparse-checkout: set worktree-config correctly
     @@ Metadata
       ## Commit message ##
          sparse-checkout: set worktree-config correctly
      
     -    The previous change added repo_config_set_worktree_gently() to assist
     -    writing config values into the config.worktree file, if enabled. An
     -    earlier change added init_worktree_config() as a helper to initialize
     -    extensions.worktreeConfig if not already enabled.
     +    `git sparse-checkout set/init` enables worktree-specific
     +    configuration[*] by setting extensions.worktreeConfig=true, but neglects
     +    to perform the additional necessary bookkeeping of relocating
     +    `core.bare=true` and `core.worktree` from $GIT_COMMON_DIR/config to
     +    $GIT_COMMON_DIR/config.worktree, as documented in git-worktree.txt. As a
     +    result of this oversight, these settings, which are nonsensical for
     +    secondary worktrees, can cause Git commands to incorrectly consider a
     +    worktree bare (in the case of `core.bare`) or operate on the wrong
     +    worktree (in the case of `core.worktree`). Fix this problem by taking
     +    advantage of the recently-added init_worktree_config() which enables
     +    `extensions.worktreeConfig` and takes care of necessary bookkeeping.
      
     -    Let the sparse-checkout builtin use these helpers instead of attempting to
     -    initialize the worktree config on its own. This changes behavior of 'git
     -    sparse-checkout set' in a few important ways:
     +    While at it, for backward-compatibility reasons, also stop upgrading the
     +    repository format to "1" since doing so is (unintentionally) not
     +    required to take advantage of `extensions.worktreeConfig`, as explained
     +    by 11664196ac ("Revert "check_repository_format_gently(): refuse
     +    extensions for old repositories"", 2020-07-15).
      
     -     1. Git will no longer upgrade the repository format, since this is not
     -        a requirement for understanding extensions.worktreeConfig.
     -
     -     2. 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.
     +    [*] 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.
      
          Reported-by: Sean Allred [off-list ref]
          Helped-by: Eric Sunshine [off-list ref]
 5:  85779dfaed3 ! 5:  c51cb3714e7 worktree: copy sparse-checkout patterns and config on add
     @@ builtin/worktree.c: static int add_worktree(const char *path, const char *refnam
      +	 */
      +	if (core_apply_sparse_checkout) {
      +		char *from_file = git_pathdup("info/sparse-checkout");
     -+		char *to_file = xstrfmt("%s/worktrees/%s/info/sparse-checkout",
     -+					realpath.buf, name);
     ++		char *to_file = xstrfmt("%s/info/sparse-checkout",
     ++					sb_repo.buf);
      +
      +		if (file_exists(from_file)) {
      +			if (safe_create_leading_directories(to_file) ||
     @@ builtin/worktree.c: static int add_worktree(const char *path, const char *refnam
      +	 */
      +	if (repository_format_worktree_config) {
      +		char *from_file = git_pathdup("config.worktree");
     -+		char *to_file = xstrfmt("%s/worktrees/%s/config.worktree",
     -+					realpath.buf, name);
     ++		char *to_file = xstrfmt("%s/config.worktree",
     ++					sb_repo.buf);
      +
      +		if (file_exists(from_file)) {
     -+			struct config_set cs = { { 0 }};
     -+			const char *str_value;
     -+			int bool_value;
     ++			struct config_set cs = { { 0 } };
     ++			const char *core_worktree;
     ++			int bare;
      +
      +			if (safe_create_leading_directories(to_file) ||
     -+			    copy_file(to_file, from_file, 0666))
     -+				die(_("failed to copy worktree config from '%s' to '%s'"),
     -+				    from_file, to_file);
     ++			    copy_file(to_file, from_file, 0666)) {
     ++				error(_("failed to copy worktree config from '%s' to '%s'"),
     ++				      from_file, to_file);
     ++				goto worktree_copy_cleanup;
     ++			}
      +
      +			git_configset_init(&cs);
      +			git_configset_add_file(&cs, from_file);
      +
     -+			if (!git_configset_get_bool(&cs, "core.bare", &bool_value) &&
     -+			    bool_value &&
     ++			if (!git_configset_get_bool(&cs, "core.bare", &bare) &&
     ++			    bare &&
      +			    git_config_set_multivar_in_file_gently(
      +					to_file, "core.bare", NULL, "true", 0))
      +				error(_("failed to unset 'core.bare' in '%s'"), to_file);
     -+			if (!git_configset_get_value(&cs, "core.worktree", &str_value) &&
     ++			if (!git_configset_get_value(&cs, "core.worktree", &core_worktree) &&
      +			    git_config_set_in_file_gently(to_file,
      +							  "core.worktree", NULL))
      +				error(_("failed to unset 'core.worktree' in '%s'"), to_file);
     @@ builtin/worktree.c: static int add_worktree(const char *path, const char *refnam
      +			git_configset_clear(&cs);
      +		}
      +
     ++worktree_copy_cleanup:
      +		free(from_file);
      +		free(to_file);
      +	}
     @@ t/t1091-sparse-checkout-builtin.sh: test_expect_success 'add to sparse-checkout'
      +	git -C repo sparse-checkout set --no-cone "/*" &&
      +	git -C repo worktree add --quiet ../worktree 2>err &&
      +	test_must_be_empty err &&
     -+	new=repo/.git/worktrees/worktree/info/sparse-checkout &&
     -+	test_path_is_file $new &&
     -+	test_cmp repo/.git/info/sparse-checkout $new &&
     ++	new="$(git -C worktree rev-parse --git-path info/sparse-checkout)" &&
     ++	test_path_is_file "$new" &&
     ++	test_cmp repo/.git/info/sparse-checkout "$new" &&
      +	git -C worktree sparse-checkout set --cone &&
      +	test_cmp_config -C worktree true core.sparseCheckoutCone &&
      +	test_must_fail git -C repo core.sparseCheckoutCone
     @@ t/t2400-worktree-add.sh: test_expect_success '"add" default branch of a bare rep
      -	)
      +		git worktree add ../there3 main &&
      +		cd ../there3 &&
     ++		# Simple check that a Git command does not
     ++		# immediately fail with the current setup
      +		git status
      +	) &&
      +	cat >expect <<-EOF &&
     @@ t/t2400-worktree-add.sh: test_expect_success '"add" default branch of a bare rep
      +		git clone --bare . bare3 &&
      +		cd bare3 &&
      +		git config extensions.worktreeconfig true &&
     ++
     ++		# Add config values that are erroneous to have in
     ++		# a config.worktree file outside of the main
     ++		# working tree, to check that Git filters them out
     ++		# when copying config during "git worktree add".
      +		git config --worktree core.bare true &&
      +		git config --worktree core.worktree "$(pwd)" &&
     ++
     ++		# We want to check that bogus.key is copied
      +		git config --worktree bogus.key value &&
      +		git config --unset core.bare &&
      +		git worktree add ../there4 main &&
      +		cd ../there4 &&
     ++
     ++		# Simple check that a Git command does not
     ++		# immediately fail with the current setup
      +		git status &&
      +		git worktree add --detach ../there5 &&
      +		cd ../there5 &&
 -:  ----------- > 6:  f687a0bfa16 config: make git_configset_get_string_tmp() private

-- 
gitgitgadget

[PATCH v6 1/6] Documentation: add extensions.worktreeConfig details

From: Derrick Stolee via GitGitGadget <hidden>
Date: 2022-02-07 21:33:12

From: Derrick Stolee <redacted>

The extensions.worktreeConfig extension was added in 58b284a (worktree:
add per-worktree config files, 2018-10-21) and was somewhat documented
in Documentation/git-config.txt. However, the extensions.worktreeConfig
value was not specified further in the list of possible config keys. The
location of the config.worktree file is not specified, and there are
some precautions that should be mentioned clearly, but are only
mentioned in git-worktree.txt.

Expand the documentation to help users discover the complexities of
extensions.worktreeConfig by adding details and cross links in these
locations (relative to Documentation/):

- config/extensions.txt
- git-config.txt
- git-worktree.txt

The updates focus on items such as

* $GIT_DIR/config.worktree takes precedence over $GIT_COMMON_DIR/config.

* The core.worktree and core.bare=true settings are incorrect to have in
  the common config file when extensions.worktreeConfig is enabled.

* The sparse-checkout settings core.sparseCheckout[Cone] are recommended
  to be set in the worktree config.

As documented in 11664196ac ("Revert "check_repository_format_gently():
refuse extensions for old repositories"", 2020-07-15), this extension
must be considered regardless of the repository format version for
historical reasons.

A future change will update references to extensions.worktreeConfig
within git-sparse-checkout.txt, but a behavior change is needed before
making those updates.

Helped-by: Elijah Newren [off-list ref]
Signed-off-by: Derrick Stolee <redacted>
---
 Documentation/config/extensions.txt | 31 +++++++++++++++++++++++++++++
 Documentation/git-config.txt        |  8 ++++++--
 Documentation/git-worktree.txt      | 11 +++++++---
 3 files changed, 45 insertions(+), 5 deletions(-)

diff --git a/Documentation/config/extensions.txt b/Documentation/config/extensions.txt
index 4e23d73cdca..bccaec7a963 100644
--- a/Documentation/config/extensions.txt
+++ b/Documentation/config/extensions.txt

@@ -6,3 +6,34 @@ extensions.objectFormat::
 Note that this setting should only be set by linkgit:git-init[1] or
 linkgit:git-clone[1].  Trying to change it after initialization will not
 work and will produce hard-to-diagnose issues.
+
+extensions.worktreeConfig::
+	If enabled, then worktrees will load config settings from the
+	`$GIT_DIR/config.worktree` file in addition to the
+	`$GIT_COMMON_DIR/config` file. Note that `$GIT_COMMON_DIR` and
+	`$GIT_DIR` are the same for the main working tree, while other
+	working trees have `$GIT_DIR` equal to
+	`$GIT_COMMON_DIR/worktrees/<id>/`. The settings in the
+	`config.worktree` file will override settings from any other
+	config files.
++
+When enabling `extensions.worktreeConfig`, you must be careful to move
+certain values from the common config file to the main working tree's
+`config.worktree` file, if present:
++
+* `core.worktree` must be moved from `$GIT_COMMON_DIR/config` to
+  `$GIT_COMMON_DIR/config.worktree`.
+* If `core.bare` is true, then it must be moved from `$GIT_COMMON_DIR/config`
+  to `$GIT_COMMON_DIR/config.worktree`.
++
+It may also be beneficial to adjust the locations of `core.sparseCheckout`
+and `core.sparseCheckoutCone` depending on your desire for customizable
+sparse-checkout settings for each worktree. By default, the `git
+sparse-checkout` builtin enables `extensions.worktreeConfig`, assigns
+these config values on a per-worktree basis, and uses the
+`$GIT_DIR/info/sparse-checkout` file to specify the sparsity for each
+worktree independently. See linkgit:git-sparse-checkout[1] for more
+details.
++
+For historical reasons, `extensions.worktreeConfig` is respected
+regardless of the `core.repositoryFormatVersion` setting.

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 2285effb363..bdcfd94b642 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt

@@ -141,9 +141,13 @@ from all available files.
 See also <<FILES>>.
 
 --worktree::
-	Similar to `--local` except that `.git/config.worktree` is
+	Similar to `--local` except that `$GIT_DIR/config.worktree` is
 	read from or written to if `extensions.worktreeConfig` is
-	present. If not it's the same as `--local`.
+	enabled. If not it's the same as `--local`. Note that `$GIT_DIR`
+	is equal to `$GIT_COMMON_DIR` for the main working tree, but is of
+	the form `$GIT_DIR/worktrees/<id>/` for other working trees. See
+	linkgit:git-worktree[1] to learn how to enable
+	`extensions.worktreeConfig`.
 
 -f <config-file>::
 --file <config-file>::

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 9e862fbcf79..b8d53c48303 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt

@@ -286,8 +286,8 @@ CONFIGURATION FILE
 ------------------
 By default, the repository `config` file is shared across all working
 trees. If the config variables `core.bare` or `core.worktree` are
-already present in the config file, they will be applied to the main
-working trees only.
+present in the common config file and `extensions.worktreeConfig` is
+disabled, then they will be applied to the main working tree only.
 
 In order to have configuration specific to working trees, you can turn
 on the `worktreeConfig` extension, e.g.:

@@ -307,11 +307,16 @@ 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:
 
- - `core.worktree` and `core.bare` should never be shared
+ - `core.worktree` should never be shared.
+
+ - `core.bare` should not be shared if the value is `core.bare=true`.
 
  - `core.sparseCheckout` is recommended per working tree, unless you
    are sure you always use sparse checkout for all working trees.
 
+See the documentation of `extensions.worktreeConfig` in
+linkgit:git-config[1] for more details.
+
 DETAILS
 -------
 Each linked working tree has a private sub-directory in the repository's

-- 
gitgitgadget

[PATCH v6 2/6] worktree: create init_worktree_config()

From: Derrick Stolee via GitGitGadget <hidden>
Date: 2022-02-07 21:33:14

From: Derrick Stolee <redacted>

Upgrading a repository to use extensions.worktreeConfig is non-trivial.
There are several steps involved, including moving some config settings
from the common config file to the main worktree's config.worktree file.
The previous change updated the documentation with all of these details.

Commands such as 'git sparse-checkout set' upgrade the repository to use
extensions.worktreeConfig without following these steps, causing some
user pain in some special cases.

Create a helper method, init_worktree_config(), that will be used in a
later change to fix this behavior within 'git sparse-checkout set'. The
method is carefully documented in worktree.h.

Note that we do _not_ upgrade the repository format version to 1 during
this process. The worktree config extension must be considered by Git
and third-party tools even if core.repositoryFormatVersion is 0 for
historical reasons documented in 11664196ac ("Revert
"check_repository_format_gently(): refuse extensions for old
repositories"", 2020-07-15). This is a special case for this extension,
and newer extensions (such as extensions.objectFormat) still need to
upgrade the repository format version.

Signed-off-by: Derrick Stolee <redacted>
---
 worktree.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 worktree.h | 21 ++++++++++++++++
 2 files changed, 94 insertions(+)

diff --git a/worktree.c b/worktree.c
index 6f598dcfcdf..5292c94b3d9 100644
--- a/worktree.c
+++ b/worktree.c

@@ -5,6 +5,7 @@
 #include "worktree.h"
 #include "dir.h"
 #include "wt-status.h"
+#include "config.h"
 
 void free_worktrees(struct worktree **worktrees)
 {

@@ -826,3 +827,75 @@ int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath,
 	*wtpath = path;
 	return 0;
 }
+
+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 init_worktree_config(struct repository *r)
+{
+	int res = 0;
+	int bare = 0;
+	struct config_set cs = { { 0 } };
+	const char *core_worktree;
+	char *common_config_file;
+	char *main_worktree_file;
+
+	/*
+	 * If the extension is already enabled, then we can skip the
+	 * upgrade process.
+	 */
+	if (repository_format_worktree_config)
+		return 0;
+	if ((res = git_config_set_gently("extensions.worktreeConfig", "true")))
+		return error(_("failed to set extensions.worktreeConfig setting"));
+
+	common_config_file = xstrfmt("%s/config", r->commondir);
+	main_worktree_file = xstrfmt("%s/config.worktree", r->commondir);
+
+	git_configset_init(&cs);
+	git_configset_add_file(&cs, common_config_file);
+
+	/*
+	 * If core.bare is true in the common config file, then we need to
+	 * move it to the main 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 main 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_value(&cs, "core.worktree", &core_worktree)) {
+		if ((res = move_config_setting("core.worktree", core_worktree,
+					       common_config_file,
+					       main_worktree_file)))
+			goto cleanup;
+	}
+
+	/*
+	 * Ensure that we use worktree config for the remaining lifetime
+	 * of the current process.
+	 */
+	repository_format_worktree_config = 1;
+
+cleanup:
+	git_configset_clear(&cs);
+	free(common_config_file);
+	free(main_worktree_file);
+	return res;
+}

diff --git a/worktree.h b/worktree.h
index 9e06fcbdf3d..e9e839926b0 100644
--- a/worktree.h
+++ b/worktree.h

@@ -183,4 +183,25 @@ void strbuf_worktree_ref(const struct worktree *wt,
 			 struct strbuf *sb,
 			 const char *refname);
 
+/**
+ * Enable worktree config for the first time. This will make the following
+ * adjustments:
+ *
+ * 1. Add extensions.worktreeConfig=true in the common config file.
+ *
+ * 2. If the common config file has a core.worktree value, then that value
+ *    is moved to the main worktree's config.worktree file.
+ *
+ * 3. If the common config file has a core.bare enabled, then that value
+ *    is moved to the main worktree's config.worktree file.
+ *
+ * If extensions.worktreeConfig is already true, then this method
+ * terminates early without any of the above steps. The existing config
+ * arrangement is assumed to be intentional.
+ *
+ * Returns 0 on success. Reports an error message and returns non-zero
+ * if any of these steps fail.
+ */
+int init_worktree_config(struct repository *r);
+
 #endif

-- 
gitgitgadget

[PATCH v6 3/6] config: add repo_config_set_worktree_gently()

From: Derrick Stolee via GitGitGadget <hidden>
Date: 2022-02-07 21:33:18

From: Derrick Stolee <redacted>

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().

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 <redacted>
---
 config.c | 35 ++++++++++++++++++++++++++++++++---
 config.h |  8 ++++++++
 2 files changed, 40 insertions(+), 3 deletions(-)

diff --git a/config.c b/config.c
index 2bffa8d4a01..1a03ced1a54 100644
--- a/config.c
+++ b/config.c

@@ -21,6 +21,7 @@
 #include "dir.h"
 #include "color.h"
 #include "refs.h"
+#include "worktree.h"
 
 struct config_source {
 	struct config_source *prev;

@@ -2884,6 +2885,20 @@ int git_config_set_gently(const char *key, const char *value)
 	return git_config_set_multivar_gently(key, value, NULL, 0);
 }
 
+int repo_config_set_worktree_gently(struct repository *r,
+				    const char *key, const char *value)
+{
+	/* 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_multivar_gently(r, key, value, NULL, 0);
+}
+
 void git_config_set(const char *key, const char *value)
 {
 	git_config_set_multivar(key, value, NULL, 0);

@@ -3181,14 +3196,28 @@ void git_config_set_multivar_in_file(const char *config_filename,
 int git_config_set_multivar_gently(const char *key, const char *value,
 				   const char *value_pattern, unsigned flags)
 {
-	return git_config_set_multivar_in_file_gently(NULL, key, value, value_pattern,
-						      flags);
+	return repo_config_set_multivar_gently(the_repository, key, value,
+					       value_pattern, flags);
+}
+
+int repo_config_set_multivar_gently(struct repository *r, const char *key,
+				    const char *value,
+				    const char *value_pattern, unsigned flags)
+{
+	char *file = repo_git_path(r, "config");
+	int res = git_config_set_multivar_in_file_gently(file,
+							 key, value,
+							 value_pattern,
+							 flags);
+	free(file);
+	return res;
 }
 
 void git_config_set_multivar(const char *key, const char *value,
 			     const char *value_pattern, unsigned flags)
 {
-	git_config_set_multivar_in_file(NULL, key, value, value_pattern,
+	git_config_set_multivar_in_file(git_path("config"),
+					key, value, value_pattern,
 					flags);
 }

diff --git a/config.h b/config.h
index f119de01309..1d98ad269bd 100644
--- a/config.h
+++ b/config.h

@@ -253,6 +253,13 @@ 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 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.
  */

@@ -281,6 +288,7 @@ int git_config_parse_key(const char *, char **, size_t *);
 
 int git_config_set_multivar_gently(const char *, const char *, const char *, unsigned);
 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);
 int git_config_set_multivar_in_file_gently(const char *, const char *, const char *, const char *, unsigned);
 
 /**

-- 
gitgitgadget

[PATCH v6 4/6] sparse-checkout: set worktree-config correctly

From: Derrick Stolee via GitGitGadget <hidden>
Date: 2022-02-07 21:33:23

From: Derrick Stolee <redacted>

`git sparse-checkout set/init` enables worktree-specific
configuration[*] by setting extensions.worktreeConfig=true, but neglects
to perform the additional necessary bookkeeping of relocating
`core.bare=true` and `core.worktree` from $GIT_COMMON_DIR/config to
$GIT_COMMON_DIR/config.worktree, as documented in git-worktree.txt. As a
result of this oversight, these settings, which are nonsensical for
secondary worktrees, can cause Git commands to incorrectly consider a
worktree bare (in the case of `core.bare`) or operate on the wrong
worktree (in the case of `core.worktree`). Fix this problem by taking
advantage of the recently-added init_worktree_config() which enables
`extensions.worktreeConfig` and takes care of necessary bookkeeping.

While at it, for backward-compatibility reasons, also stop upgrading the
repository format to "1" since doing so is (unintentionally) not
required to take advantage of `extensions.worktreeConfig`, as explained
by 11664196ac ("Revert "check_repository_format_gently(): refuse
extensions for old repositories"", 2020-07-15).

[*] 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.

Reported-by: Sean Allred <redacted>
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Derrick Stolee <redacted>
---
 Documentation/git-sparse-checkout.txt | 16 +++++++++++----
 builtin/sparse-checkout.c             | 28 +++++++++++++--------------
 sparse-index.c                        | 10 +++-------
 t/t1091-sparse-checkout-builtin.sh    |  4 ++--
 4 files changed, 30 insertions(+), 28 deletions(-)

diff --git a/Documentation/git-sparse-checkout.txt b/Documentation/git-sparse-checkout.txt
index b81dbe06543..94dad137b96 100644
--- a/Documentation/git-sparse-checkout.txt
+++ b/Documentation/git-sparse-checkout.txt

@@ -31,13 +31,21 @@ COMMANDS
 	Describe the patterns in the sparse-checkout file.
 
 'set'::
-	Enable the necessary config settings
-	(extensions.worktreeConfig, core.sparseCheckout,
-	core.sparseCheckoutCone) if they are not already enabled, and
-	write a set of patterns to the sparse-checkout file from the
+	Enable the necessary sparse-checkout config settings
+	(`core.sparseCheckout`, `core.sparseCheckoutCone`, and
+	`index.sparse`) if they are not already set to the desired values,
+	and write a set of patterns to the sparse-checkout file from the
 	list of arguments following the 'set' subcommand. Update the
 	working directory to match the new patterns.
 +
+To ensure that adjusting the sparse-checkout settings within a worktree
+does not alter the sparse-checkout settings in other worktrees, the 'set'
+subcommand will upgrade your repository config to use worktree-specific
+config if not already present. The sparsity defined by the arguments to
+the 'set' subcommand are stored in the worktree-specific sparse-checkout
+file. See linkgit:git-worktree[1] and the documentation of
+`extensions.worktreeConfig` in linkgit:git-config[1] for more details.
++
 When the `--stdin` option is provided, the patterns are read from
 standard in as a newline-delimited list instead of from the arguments.
 +

diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index 679c1070368..314c8d61f80 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c

@@ -15,6 +15,7 @@
 #include "wt-status.h"
 #include "quote.h"
 #include "sparse-index.h"
+#include "worktree.h"
 
 static const char *empty_base = "";

@@ -359,26 +360,23 @@ enum sparse_checkout_mode {
 
 static int set_config(enum sparse_checkout_mode mode)
 {
-	const char *config_path;
-
-	if (upgrade_repository_format(1) < 0)
-		die(_("unable to upgrade repository format to enable worktreeConfig"));
-	if (git_config_set_gently("extensions.worktreeConfig", "true")) {
-		error(_("failed to set extensions.worktreeConfig setting"));
+	/* Update to use worktree config, if not already. */
+	if (init_worktree_config(the_repository)) {
+		error(_("failed to initialize worktree config"));
 		return 1;
 	}
 
-	config_path = git_path("config.worktree");
-	git_config_set_in_file_gently(config_path,
-				      "core.sparseCheckout",
-				      mode ? "true" : NULL);
-
-	git_config_set_in_file_gently(config_path,
-				      "core.sparseCheckoutCone",
-				      mode == MODE_CONE_PATTERNS ? "true" : NULL);
+	if (repo_config_set_worktree_gently(the_repository,
+					    "core.sparseCheckout",
+					    mode ? "true" : "false") ||
+	    repo_config_set_worktree_gently(the_repository,
+					    "core.sparseCheckoutCone",
+					    mode == MODE_CONE_PATTERNS ?
+						"true" : "false"))
+		return 1;
 
 	if (mode == MODE_NO_PATTERNS)
-		set_sparse_index_config(the_repository, 0);
+		return set_sparse_index_config(the_repository, 0);
 
 	return 0;
 }

diff --git a/sparse-index.c b/sparse-index.c
index a1d505d50e9..e93609999e0 100644
--- a/sparse-index.c
+++ b/sparse-index.c

@@ -99,13 +99,9 @@ static int convert_to_sparse_rec(struct index_state *istate,
 
 int set_sparse_index_config(struct repository *repo, int enable)
 {
-	int res;
-	char *config_path = repo_git_path(repo, "config.worktree");
-	res = git_config_set_in_file_gently(config_path,
-					    "index.sparse",
-					    enable ? "true" : NULL);
-	free(config_path);
-
+	int res = repo_config_set_worktree_gently(repo,
+						  "index.sparse",
+						  enable ? "true" : "false");
 	prepare_repo_settings(repo);
 	repo->settings.sparse_index = enable;
 	return res;

diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index 42776984fe7..be6ea4ffe33 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh

@@ -117,7 +117,7 @@ test_expect_success 'switching to cone mode with non-cone mode patterns' '
 		cd bad-patterns &&
 		git sparse-checkout init &&
 		git sparse-checkout add dir &&
-		git config core.sparseCheckoutCone true &&
+		git config --worktree core.sparseCheckoutCone true &&
 		test_must_fail git sparse-checkout add dir 2>err &&
 		grep "existing sparse-checkout patterns do not use cone mode" err
 	)

@@ -256,7 +256,7 @@ test_expect_success 'sparse-index enabled and disabled' '
 		test_cmp expect actual &&
 
 		git -C repo config --list >config &&
-		! grep index.sparse config
+		test_cmp_config -C repo false index.sparse
 	)
 '

-- 
gitgitgadget

[PATCH v6 5/6] worktree: copy sparse-checkout patterns and config on add

From: Derrick Stolee via GitGitGadget <hidden>
Date: 2022-02-07 21:33:24

From: Derrick Stolee <redacted>

When adding a new worktree, it is reasonable to expect that we want to
use the current set of sparse-checkout settings for that new worktree.
This is particularly important for repositories where the worktree would
become too large to be useful. This is even more important when using
partial clone as well, since we want to avoid downloading the missing
blobs for files that should not be written to the new worktree.

The only way to create such a worktree without this intermediate step of
expanding the full worktree is to copy the sparse-checkout patterns and
config settings during 'git worktree add'. Each worktree has its own
sparse-checkout patterns, and the default behavior when the
sparse-checkout file is missing is to include all paths at HEAD. Thus,
we need to have patterns from somewhere, they might as well be the
current worktree's patterns. These are then modified independently in
the future.

In addition to the sparse-checkout file, copy the worktree config file
if worktree config is enabled and the file exists. This will copy over
any important settings to ensure the new worktree behaves the same as
the current one. The only exception we must continue to make is that
core.bare and core.worktree should become unset in the worktree's config
file.

Signed-off-by: Derrick Stolee <redacted>
---
 builtin/worktree.c                 | 63 ++++++++++++++++++++++++++++++
 t/t1091-sparse-checkout-builtin.sh | 31 +++++++++++----
 t/t2400-worktree-add.sh            | 58 ++++++++++++++++++++++++++-
 3 files changed, 142 insertions(+), 10 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 2838254f7f2..c6eb636329a 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c

@@ -335,6 +335,69 @@ static int add_worktree(const char *path, const char *refname,
 	strbuf_addf(&sb, "%s/commondir", sb_repo.buf);
 	write_file(sb.buf, "../..");
 
+	/*
+	 * If the current worktree has sparse-checkout enabled, then copy
+	 * the sparse-checkout patterns from the current worktree.
+	 */
+	if (core_apply_sparse_checkout) {
+		char *from_file = git_pathdup("info/sparse-checkout");
+		char *to_file = xstrfmt("%s/info/sparse-checkout",
+					sb_repo.buf);
+
+		if (file_exists(from_file)) {
+			if (safe_create_leading_directories(to_file) ||
+			    copy_file(to_file, from_file, 0666))
+				error(_("failed to copy '%s' to '%s'; sparse-checkout may not work correctly"),
+				      from_file, to_file);
+		}
+
+		free(from_file);
+		free(to_file);
+	}
+
+	/*
+	 * If we are using worktree config, then copy all current config
+	 * values from the current worktree into the new one, that way the
+	 * new worktree behaves the same as this one.
+	 */
+	if (repository_format_worktree_config) {
+		char *from_file = git_pathdup("config.worktree");
+		char *to_file = xstrfmt("%s/config.worktree",
+					sb_repo.buf);
+
+		if (file_exists(from_file)) {
+			struct config_set cs = { { 0 } };
+			const char *core_worktree;
+			int bare;
+
+			if (safe_create_leading_directories(to_file) ||
+			    copy_file(to_file, from_file, 0666)) {
+				error(_("failed to copy worktree config from '%s' to '%s'"),
+				      from_file, to_file);
+				goto worktree_copy_cleanup;
+			}
+
+			git_configset_init(&cs);
+			git_configset_add_file(&cs, from_file);
+
+			if (!git_configset_get_bool(&cs, "core.bare", &bare) &&
+			    bare &&
+			    git_config_set_multivar_in_file_gently(
+					to_file, "core.bare", NULL, "true", 0))
+				error(_("failed to unset 'core.bare' in '%s'"), to_file);
+			if (!git_configset_get_value(&cs, "core.worktree", &core_worktree) &&
+			    git_config_set_in_file_gently(to_file,
+							  "core.worktree", NULL))
+				error(_("failed to unset 'core.worktree' in '%s'"), to_file);
+
+			git_configset_clear(&cs);
+		}
+
+worktree_copy_cleanup:
+		free(from_file);
+		free(to_file);
+	}
+
 	strvec_pushf(&child_env, "%s=%s", GIT_DIR_ENVIRONMENT, sb_git.buf);
 	strvec_pushf(&child_env, "%s=%s", GIT_WORK_TREE_ENVIRONMENT, path);
 	cp.git_cmd = 1;

diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index be6ea4ffe33..8a757b43e6c 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh

@@ -146,9 +146,9 @@ test_expect_success 'interaction with clone --no-checkout (unborn index)' '
 '
 
 test_expect_success 'set enables config' '
-	git init empty-config &&
+	git init worktree-config &&
 	(
-		cd empty-config &&
+		cd worktree-config &&
 		test_commit test file &&
 		test_path_is_missing .git/config.worktree &&
 		git sparse-checkout set nothing &&

@@ -201,6 +201,21 @@ test_expect_success 'add to sparse-checkout' '
 	check_files repo "a folder1 folder2"
 '
 
+test_expect_success 'worktree: add copies sparse-checkout patterns' '
+	cat repo/.git/info/sparse-checkout >old &&
+	test_when_finished cp old repo/.git/info/sparse-checkout &&
+	test_when_finished git -C repo worktree remove ../worktree &&
+	git -C repo sparse-checkout set --no-cone "/*" &&
+	git -C repo worktree add --quiet ../worktree 2>err &&
+	test_must_be_empty err &&
+	new="$(git -C worktree rev-parse --git-path info/sparse-checkout)" &&
+	test_path_is_file "$new" &&
+	test_cmp repo/.git/info/sparse-checkout "$new" &&
+	git -C worktree sparse-checkout set --cone &&
+	test_cmp_config -C worktree true core.sparseCheckoutCone &&
+	test_must_fail git -C repo core.sparseCheckoutCone
+'
+
 test_expect_success 'cone mode: match patterns' '
 	git -C repo config --worktree core.sparseCheckoutCone true &&
 	rm -rf repo/a repo/folder1 repo/folder2 &&

@@ -520,13 +535,13 @@ test_expect_success 'interaction with submodules' '
 '
 
 test_expect_success 'different sparse-checkouts with worktrees' '
+	git -C repo sparse-checkout set --cone deep folder1 &&
 	git -C repo worktree add --detach ../worktree &&
-	check_files worktree "a deep folder1 folder2" &&
-	git -C worktree sparse-checkout init --cone &&
-	git -C repo sparse-checkout set folder1 &&
-	git -C worktree sparse-checkout set deep/deeper1 &&
-	check_files repo a folder1 &&
-	check_files worktree a deep
+	check_files worktree "a deep folder1" &&
+	git -C repo sparse-checkout set --cone folder1 &&
+	git -C worktree sparse-checkout set --cone deep/deeper1 &&
+	check_files repo "a folder1" &&
+	check_files worktree "a deep"
 '
 
 test_expect_success 'set using filename keeps file on-disk' '

diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
index 37ad79470fb..43139af08fc 100755
--- a/t/t2400-worktree-add.sh
+++ b/t/t2400-worktree-add.sh

@@ -165,8 +165,62 @@ test_expect_success '"add" default branch of a bare repo' '
 	(
 		git clone --bare . bare2 &&
 		cd bare2 &&
-		git worktree add ../there3 main
-	)
+		git worktree add ../there3 main &&
+		cd ../there3 &&
+		# Simple check that a Git command does not
+		# immediately fail with the current setup
+		git status
+	) &&
+	cat >expect <<-EOF &&
+	init.t
+	EOF
+	ls there3 >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '"add" to bare repo with worktree config' '
+	(
+		git clone --bare . bare3 &&
+		cd bare3 &&
+		git config extensions.worktreeconfig true &&
+
+		# Add config values that are erroneous to have in
+		# a config.worktree file outside of the main
+		# working tree, to check that Git filters them out
+		# when copying config during "git worktree add".
+		git config --worktree core.bare true &&
+		git config --worktree core.worktree "$(pwd)" &&
+
+		# We want to check that bogus.key is copied
+		git config --worktree bogus.key value &&
+		git config --unset core.bare &&
+		git worktree add ../there4 main &&
+		cd ../there4 &&
+
+		# Simple check that a Git command does not
+		# immediately fail with the current setup
+		git status &&
+		git worktree add --detach ../there5 &&
+		cd ../there5 &&
+		git status
+	) &&
+
+	# the worktree has the arbitrary value copied.
+	test_cmp_config -C there4 value bogus.key &&
+	test_cmp_config -C there5 value bogus.key &&
+
+	# however, core.bare and core.worktree were removed.
+	test_must_fail git -C there4 config core.bare &&
+	test_must_fail git -C there4 config core.worktree &&
+
+	cat >expect <<-EOF &&
+	init.t
+	EOF
+
+	ls there4 >actual &&
+	test_cmp expect actual &&
+	ls there5 >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success 'checkout with grafts' '

-- 
gitgitgadget

[PATCH v6 6/6] config: make git_configset_get_string_tmp() private

From: Derrick Stolee via GitGitGadget <hidden>
Date: 2022-02-07 21:33:25

From: Derrick Stolee <redacted>

This method was created in f1de981e8 (config: fix leaks from
git_config_get_string_const(), 2020-08-14) but its only use was in the
repo_config_get_string_tmp() method, also declared in config.h and
implemented in config.c. Since this is otherwise unused and is a very
similar implementation to git_configset_get_value(), let's remove this
declaration.

Signed-off-by: Derrick Stolee <redacted>
---
 config.c | 4 ++--
 config.h | 1 -
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/config.c b/config.c
index 1a03ced1a54..870b22dd2ff 100644
--- a/config.c
+++ b/config.c

@@ -2179,8 +2179,8 @@ int git_configset_get_string(struct config_set *cs, const char *key, char **dest
 		return 1;
 }
 
-int git_configset_get_string_tmp(struct config_set *cs, const char *key,
-				 const char **dest)
+static int git_configset_get_string_tmp(struct config_set *cs, const char *key,
+					const char **dest)
 {
 	const char *value;
 	if (!git_configset_get_value(cs, key, &value)) {

diff --git a/config.h b/config.h
index 1d98ad269bd..184aef1eca4 100644
--- a/config.h
+++ b/config.h

@@ -494,7 +494,6 @@ void git_configset_clear(struct config_set *cs);
 int git_configset_get_value(struct config_set *cs, const char *key, const char **dest);
 
 int git_configset_get_string(struct config_set *cs, const char *key, char **dest);
-int git_configset_get_string_tmp(struct config_set *cs, const char *key, const char **dest);
 int git_configset_get_int(struct config_set *cs, const char *key, int *dest);
 int git_configset_get_ulong(struct config_set *cs, const char *key, unsigned long *dest);
 int git_configset_get_bool(struct config_set *cs, const char *key, int *dest);

-- 
gitgitgadget

Re: [PATCH v6 0/6] Sparse checkout: fix bug with worktree of bare repo

From: Elijah Newren <hidden>
Date: 2022-02-08 04:14:55

On Mon, Feb 7, 2022 at 1:33 PM Derrick Stolee via GitGitGadget
[off-list ref] wrote:

This series is now based on v2.35.0 since that contains all of the necessary
topics.

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 or core.worktree are set in the
common config file. This series fixes this, but also puts in place some
helpers to prevent this from happening in the future.

ATTENTION: I have significantly redesigned the series since previous
versions, so most of this cover letter is new.

 * Patch 1 updates documentation around extensions.worktreeConfig in a few
   places to improve discoverability. Several cross links are added to make
   it easy to find the related areas. (The documentation for the changes to
   'git sparse-checkout' are delayed to patch 4.)

 * Patch 2 introduces the init_worktree_config() helper which follows the
   documented instructions to enable extensions.worktreeConfig as well as
   move the core.bare and core.worktree config values. This update does not
   modify core.repositoryFormatVersion, since this is not needed
   specifically for extensions.worktreeConfig.

 * Patch 3 adds a new repo_config_set_worktree_gently() helper method so we
   can internally adjust a config value within a worktree, at least if
   extensions.worktreeConfig is enabled. (It will write to the common config
   file if the extension is not enabled.)

 * Patch 4 modifies the sparse-checkout builtin to use
   init_worktree_config() and repo_config_set_worktree_gently() in ways that
   fix the reported bug. The behavior change here is that it will no longer
   upgrade the repository format version, since that is not needed for
   extensions.worktreeConfig.

 * Patch 5 updates 'git worktree add' to copy the worktree config from the
   current worktree to the new one (while unsetting core.bare=true and
   core.worktree=*) along with copying the sparse-checkout patterns file.

[1]
https://lore.kernel.org/git/CABceR4bZmtC4rCwgxZ1BBYZP69VOUca1f_moJoP989vTUZWu9Q@mail.gmail.com/
[2]
https://lore.kernel.org/git/CAPig+cQ6U_yFw-X2OWrizB1rbCvc4bNxuSzKFzmoLNnm0GH8Eg@mail.gmail.com/


Updates in v6
=============

 * Updated documentation to use "working tree" over "worktree" and "" over
   ""

Not sure what "" over "" means.

 * Delay some allocations to avoid leaking memory in error conditions.
 * Use "main worktree" over "base worktree" in comments.
 *

Was the empty bullet point meant to cover the new patch 6?  Anyway,
comments on the cover letter aside, the patches themselves are:

Reviewed-by: Elijah Newren <redacted>

Re: [PATCH v6 0/6] Sparse checkout: fix bug with worktree of bare repo

From: Eric Sunshine <hidden>
Date: 2022-02-08 05:32:53

On Mon, Feb 7, 2022 at 11:14 PM Elijah Newren [off-list ref] wrote:

On Mon, Feb 7, 2022 at 1:33 PM Derrick Stolee via GitGitGadget
[off-list ref] wrote:

quoted

Updates in v6
 * Updated documentation to use "working tree" over "worktree" and "" over
   ""

Not sure what "" over "" means.

Probably related to my review comment[1] about spelling it
"$GIT_DIR/worktrees/<id>/" rather than
"$GIT_DIR/worktrees/<worktree-name>/".

quoted

 * Delay some allocations to avoid leaking memory in error conditions.
 * Use "main worktree" over "base worktree" in comments.
 *

Was the empty bullet point meant to cover the new patch 6?

The "Updates in v6" section was botched a bit. If you look closely,
the remaining bullet points actually ended up in the "Updates in v5"
section. The complete "Updates in v6" section should have been
(approximately):

 * Updated documentation to use "working tree" over "worktree" and
   "<id>" over "<worktree-name>"
 * Delay some allocations to avoid leaking memory in error conditions.
 * Use "main worktree" over "base worktree" in comments.
 * Removed use of git_configset_get_string_tmp() and added a patch that
   removes its public declaration.
 * Fragile variables are replaced with better ones.
 * Variable names and code style improved.
 * Several test cleanups in patch 5.

[1]: https://lore.kernel.org/git/pull.1101.v4.git.1643136134.gitgitgadget@gmail.com/T/#m4926177771017bbea82c33e9e03e6a9a004ebf24

Re: [PATCH v6 0/6] Sparse checkout: fix bug with worktree of bare repo

From: Elijah Newren <hidden>
Date: 2022-02-08 05:33:14

On Mon, Feb 7, 2022 at 9:03 PM Eric Sunshine [off-list ref] wrote:

On Mon, Feb 7, 2022 at 11:14 PM Elijah Newren [off-list ref] wrote:

quoted

On Mon, Feb 7, 2022 at 1:33 PM Derrick Stolee via GitGitGadget
[off-list ref] wrote:

quoted

Updates in v6
 * Updated documentation to use "working tree" over "worktree" and "" over
   ""

Not sure what "" over "" means.

Probably related to my review comment[1] about spelling it
"$GIT_DIR/worktrees/<id>/" rather than
"$GIT_DIR/worktrees/<worktree-name>/".

quoted

 * Delay some allocations to avoid leaking memory in error conditions.
 * Use "main worktree" over "base worktree" in comments.
 *

Was the empty bullet point meant to cover the new patch 6?

The "Updates in v6" section was botched a bit. If you look closely,
the remaining bullet points actually ended up in the "Updates in v5"
section. The complete "Updates in v6" section should have been
(approximately):

 * Updated documentation to use "working tree" over "worktree" and
   "<id>" over "<worktree-name>"
 * Delay some allocations to avoid leaking memory in error conditions.
 * Use "main worktree" over "base worktree" in comments.
 * Removed use of git_configset_get_string_tmp() and added a patch that
   removes its public declaration.
 * Fragile variables are replaced with better ones.
 * Variable names and code style improved.
 * Several test cleanups in patch 5.

[1]: https://lore.kernel.org/git/pull.1101.v4.git.1643136134.gitgitgadget@gmail.com/T/#m4926177771017bbea82c33e9e03e6a9a004ebf24

So, you clearly also read the patches in this round.  Do they also
look good to you?   :-)

Re: [PATCH v6 0/6] Sparse checkout: fix bug with worktree of bare repo

From: Eric Sunshine <hidden>
Date: 2022-02-08 05:43:06

On Tue, Feb 8, 2022 at 12:18 AM Elijah Newren [off-list ref] wrote:

On Mon, Feb 7, 2022 at 9:03 PM Eric Sunshine [off-list ref] wrote:

quoted

On Mon, Feb 7, 2022 at 11:14 PM Elijah Newren [off-list ref] wrote:

quoted

Was the empty bullet point meant to cover the new patch 6?

The "Updates in v6" section was botched a bit. If you look closely,
the remaining bullet points actually ended up in the "Updates in v5"
section. The complete "Updates in v6" section should have been
(approximately):

 * Updated documentation to use "working tree" over "worktree" and
   "<id>" over "<worktree-name>"
 * Delay some allocations to avoid leaking memory in error conditions.
 * Use "main worktree" over "base worktree" in comments.
 * Removed use of git_configset_get_string_tmp() and added a patch that
   removes its public declaration.
 * Fragile variables are replaced with better ones.
 * Variable names and code style improved.
 * Several test cleanups in patch 5.

So, you clearly also read the patches in this round.  Do they also
look good to you?   :-)

I have not yet looked at either the patches or the range-diff, and I
only scanned my eye quickly over the cover letter, but the empty
bullet point made me stop and look a bit more carefully at that part
(and only that part) of the cover letter. Not sure yet when I'll have
time to carefully read this round.

Re: [PATCH v6 0/6] Sparse checkout: fix bug with worktree of bare repo

From: Derrick Stolee <hidden>
Date: 2022-02-08 14:33:15

On 2/8/2022 12:02 AM, Eric Sunshine wrote:

On Mon, Feb 7, 2022 at 11:14 PM Elijah Newren [off-list ref] wrote:

quoted

On Mon, Feb 7, 2022 at 1:33 PM Derrick Stolee via GitGitGadget
[off-list ref] wrote:

quoted

Updates in v6
 * Updated documentation to use "working tree" over "worktree" and "" over
   ""

Not sure what "" over "" means.

Probably related to my review comment[1] about spelling it
"$GIT_DIR/worktrees/<id>/" rather than
"$GIT_DIR/worktrees/<worktree-name>/".

quoted

 * Delay some allocations to avoid leaking memory in error conditions.
 * Use "main worktree" over "base worktree" in comments.
 *

Was the empty bullet point meant to cover the new patch 6?

The "Updates in v6" section was botched a bit. If you look closely,
the remaining bullet points actually ended up in the "Updates in v5"
section. The complete "Updates in v6" section should have been
(approximately):

Whoops on mixing them up. Sorry about that.

 * Updated documentation to use "working tree" over "worktree" and
   "<id>" over "<worktree-name>"

this is the issue for the empty quotes. GGG thought these were HTML
tags, so made them disappear. I should have used `<id>` and `<worktree-name>`.

 * Delay some allocations to avoid leaking memory in error conditions.
 * Use "main worktree" over "base worktree" in comments.
 * Removed use of git_configset_get_string_tmp() and added a patch that
   removes its public declaration.
 * Fragile variables are replaced with better ones.
 * Variable names and code style improved.
 * Several test cleanups in patch 5.

[1]: https://lore.kernel.org/git/pull.1101.v4.git.1643136134.gitgitgadget@gmail.com/T/#m4926177771017bbea82c33e9e03e6a9a004ebf24

Thanks for cleaning up my cover letter!
-Stolee

Re: [PATCH v6 1/6] Documentation: add extensions.worktreeConfig details

From: Junio C Hamano <hidden>
Date: 2022-02-08 22:24:57

"Derrick Stolee via GitGitGadget" [off-list ref] writes:

quoted hunk

diff --git a/Documentation/config/extensions.txt b/Documentation/config/extensions.txt
index 4e23d73cdca..bccaec7a963 100644
--- a/Documentation/config/extensions.txt
+++ b/Documentation/config/extensions.txt

@@ -6,3 +6,34 @@ extensions.objectFormat::
 Note that this setting should only be set by linkgit:git-init[1] or
 linkgit:git-clone[1].  Trying to change it after initialization will not
 work and will produce hard-to-diagnose issues.
+
+extensions.worktreeConfig::
+	If enabled, then worktrees will load config settings from the
+	`$GIT_DIR/config.worktree` file in addition to the
+	`$GIT_COMMON_DIR/config` file. Note that `$GIT_COMMON_DIR` and
+	`$GIT_DIR` are the same for the main working tree, while other
+	working trees have `$GIT_DIR` equal to
+	`$GIT_COMMON_DIR/worktrees/<id>/`. The settings in the

The mixed use of "worktree" and "working tree" in this paragraph
might confuse readers into thinking that the paragraph is being
careful to make distinction between the two.  All references to
"working tree" in the above paragraph should actually be "worktree",
I would think.

	Side note: "working tree" is in the glossary-content.txt,
	but "worktree", which is one "working tree" + repository
	metadata (i.e. ".git/") that may be partially shared with
	other "worktree"s of a single repository, is not defined.

This is a tangent, but I wonder why we chose to use a different
filename (i.e. not "config" but "config.worktree") for this.  If we
were redoing multi-worktree support from scratch, we would not reuse
the $GIT_DIR used by the primary worktree as $GIT_COMMON_DIR, so
that all worktrees would share a single $GIT_COMMON_DIR and
$GIT_COMMON_DIR/config that has stuff that is shared among all the
worktrees, while per worktree stuff is in $GIT_DIR/config even for
the primary worktree.  But that is all water under the bridge now.

Other than the terminology gotcha, looked sensible.  Migrating
automatically and/or noticing a suspicious setting may be needed to
help end users, but that would not be within the scope of this step.

Attached is a "how about this?" glossary update suggestion.  Most of
the existing mention of "working tree" are fine as-is because they
only care about what is in the "working tree", but some should be
changed to "worktree" to stress the fact that they care not just the
"working tree" part but also the repository metadata part that is
associated with that single "working tree".  The first hunk says
worktree but it is clear that it is interested only in the "working
tree" files.

 Documentation/glossary-content.txt | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git c/Documentation/glossary-content.txt w/Documentation/glossary-content.txt
index c077971335..d816512c6a 100644
--- c/Documentation/glossary-content.txt
+++ w/Documentation/glossary-content.txt

@@ -312,7 +312,7 @@ Pathspecs are used on the command line of "git ls-files", "git
 ls-tree", "git add", "git grep", "git diff", "git checkout",
 and many other commands to
 limit the scope of operations to some subset of the tree or
-worktree.  See the documentation of each command for whether
+working tree.  See the documentation of each command for whether
 paths are relative to the current directory or toplevel.  The
 pathspec syntax is as follows:
 +

@@ -446,7 +446,7 @@ exclude;;
 	interface than the <<def_plumbing,plumbing>>.
 
 [[def_per_worktree_ref]]per-worktree ref::
-	Refs that are per-<<def_working_tree,worktree>>, rather than
+	Refs that are per-<<def_worktree,worktree>>, rather than
 	global.  This is presently only <<def_HEAD,HEAD>> and any refs
 	that start with `refs/bisect/`, but might later include other
 	unusual refs.

@@ -669,3 +669,12 @@ The most notable example is `HEAD`.
 	The tree of actual checked out files.  The working tree normally
 	contains the contents of the <<def_HEAD,HEAD>> commit's tree,
 	plus any local changes that you have made but not yet committed.
+
+[[def_work_tree]]worktree::
+	A repository can have zero (i.e. bare repository) or one or
+	more worktrees attached to it. One "worktree" consists of a
+	"working tree" and repository metadata, most of which are
+	shared among other worktrees of a single repository, and
+	some of which are maintained separately per worktree
+	(e.g. the index, HEAD, per-worktree refs and per-worktree
+	configuration file)

Re: [PATCH v6 1/6] Documentation: add extensions.worktreeConfig details

From: Derrick Stolee <hidden>
Date: 2022-02-09 02:41:38

On 2/8/2022 5:20 PM, Junio C Hamano wrote:

"Derrick Stolee via GitGitGadget" [off-list ref] writes:

quoted

diff --git a/Documentation/config/extensions.txt b/Documentation/config/extensions.txt
index 4e23d73cdca..bccaec7a963 100644
--- a/Documentation/config/extensions.txt
+++ b/Documentation/config/extensions.txt

@@ -6,3 +6,34 @@ extensions.objectFormat::
 Note that this setting should only be set by linkgit:git-init[1] or
 linkgit:git-clone[1].  Trying to change it after initialization will not
 work and will produce hard-to-diagnose issues.
+
+extensions.worktreeConfig::
+	If enabled, then worktrees will load config settings from the
+	`$GIT_DIR/config.worktree` file in addition to the
+	`$GIT_COMMON_DIR/config` file. Note that `$GIT_COMMON_DIR` and
+	`$GIT_DIR` are the same for the main working tree, while other
+	working trees have `$GIT_DIR` equal to
+	`$GIT_COMMON_DIR/worktrees/<id>/`. The settings in the

The mixed use of "worktree" and "working tree" in this paragraph
might confuse readers into thinking that the paragraph is being
careful to make distinction between the two.  All references to
"working tree" in the above paragraph should actually be "worktree",
I would think.

I generally agree. This was changed in the most-recent re-roll
based on a request by Eric [1]. I'm happy to take whichever
version the two of you settle on.

[1] https://lore.kernel.org/git/CAPig+cS-3CxxyPGcy_vkeN_WYTRo1b-ZhJNdPy8ARZSNKkF1Xg@mail.gmail.com/

	Side note: "working tree" is in the glossary-content.txt,
	but "worktree", which is one "working tree" + repository
	metadata (i.e. ".git/") that may be partially shared with
	other "worktree"s of a single repository, is not defined.

This is a tangent, but I wonder why we chose to use a different
filename (i.e. not "config" but "config.worktree") for this.  If we
were redoing multi-worktree support from scratch, we would not reuse
the $GIT_DIR used by the primary worktree as $GIT_COMMON_DIR, so
that all worktrees would share a single $GIT_COMMON_DIR and
$GIT_COMMON_DIR/config that has stuff that is shared among all the
worktrees, while per worktree stuff is in $GIT_DIR/config even for
the primary worktree.  But that is all water under the bridge now.

Right. I think that since the primary worktree uses $GIT_COMMON_DIR
as its location for base information (like HEAD) it also means that
its worktree-specific config file cannot be called "config".

Perhaps there could have been a way to split the worktrees so the
primary worktree got its own directory within ".git/worktrees/", but
my guess is that the design optimized for backwards compatibility:
Git clients that don't understand worktrees could still interact with
the primary worktree.

quoted hunk

Other than the terminology gotcha, looked sensible.  Migrating
automatically and/or noticing a suspicious setting may be needed to
help end users, but that would not be within the scope of this step.

Attached is a "how about this?" glossary update suggestion.  Most of
the existing mention of "working tree" are fine as-is because they
only care about what is in the "working tree", but some should be
changed to "worktree" to stress the fact that they care not just the
"working tree" part but also the repository metadata part that is
associated with that single "working tree".  The first hunk says
worktree but it is clear that it is interested only in the "working
tree" files.

 Documentation/glossary-content.txt | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git c/Documentation/glossary-content.txt w/Documentation/glossary-content.txt
index c077971335..d816512c6a 100644
--- c/Documentation/glossary-content.txt
+++ w/Documentation/glossary-content.txt

@@ -312,7 +312,7 @@ Pathspecs are used on the command line of "git ls-files", "git
 ls-tree", "git add", "git grep", "git diff", "git checkout",
 and many other commands to
 limit the scope of operations to some subset of the tree or
-worktree.  See the documentation of each command for whether
+working tree.  See the documentation of each command for whether
 paths are relative to the current directory or toplevel.  The
 pathspec syntax is as follows:
 +

@@ -446,7 +446,7 @@ exclude;;
 	interface than the <<def_plumbing,plumbing>>.
 
 [[def_per_worktree_ref]]per-worktree ref::
-	Refs that are per-<<def_working_tree,worktree>>, rather than
+	Refs that are per-<<def_worktree,worktree>>, rather than
 	global.  This is presently only <<def_HEAD,HEAD>> and any refs
 	that start with `refs/bisect/`, but might later include other
 	unusual refs.

@@ -669,3 +669,12 @@ The most notable example is `HEAD`.
 	The tree of actual checked out files.  The working tree normally
 	contains the contents of the <<def_HEAD,HEAD>> commit's tree,
 	plus any local changes that you have made but not yet committed.
+
+[[def_work_tree]]worktree::
+	A repository can have zero (i.e. bare repository) or one or
+	more worktrees attached to it. One "worktree" consists of a
+	"working tree" and repository metadata, most of which are
+	shared among other worktrees of a single repository, and
+	some of which are maintained separately per worktree
+	(e.g. the index, HEAD, per-worktree refs and per-worktree
+	configuration file)

I like this addition, except that I don't understand the "per-worktree
refs" (other than HEAD). Are there other thins used by features such
as merge and rebase that would appear as worktree-specific? Of course,
some state for these operations is stored per-worktree, I just didn't
know if any were actually "refs".

Other than that technicality, which could be completely correct, this
is a good idea to include.

Thanks,
-Stolee

Re: [PATCH v5 5/5] worktree: copy sparse-checkout patterns and config on add

From: Jean-Noël Avila <hidden>
Date: 2022-02-09 07:53:29

Le 07/02/2022 à 15:10, Derrick Stolee a écrit :

On 2/6/2022 5:36 AM, Jean-Noël AVILA wrote:

quoted

On Monday, 31 January 2022 16:00:59 CET Derrick Stolee via GitGitGadget wrote:

Hi Jean-Noël. Thanks for your attention to the translatable messages
here:

quoted

error(_("failed to copy '%s' to '%s'; sparse-checkout may not work correctly"),
       from_file, to_file);

quoted

die(_("failed to copy worktree config from '%s' to '%s'"),
     from_file, to_file);

quoted

error(_("failed to unset 'core.bare' in '%s'"), to_file);

quoted

error(_("failed to unset 'core.worktree' in '%s'"), to_file);

quoted

In the first patch of this series, you use _("unable to set '%s' in'%s'). Does it make sense to reuse this string here?

I would argue that "unable to set" is not appropriate for any of these
messages. Perhaps the "failed to copy" messages might be able to use
"unable to set", but the information that the config setting is coming
from settings the user controlled is valuable.

The "failed to unset" means "we are trying to _remove_ this setting
from the config file", so "unable to set" does not seem to work here.

I'm open to revisiting this if you disagree.

Thanks,
-Stolee

Hi Derrick,

Sorry for not being more precise. The first two errors were not the 
subject of this remark.

For the last two, this is quite surprising that the same function 
failing (git_config_set_in_file_gently) can lead to different error 
messages.

In any case, I would argue at least for  shifting to :

     error(_("failed to unset '%s' in '%s'"), 'core.bare", to_file);
and
     error(_("failed to unset '%s' in '%s'"), "core.worktree", to_file);

in order to factorize the message and get the option name out of the way.

Thanks,

Jean-Noël

Re: [PATCH v5 5/5] worktree: copy sparse-checkout patterns and config on add

From: Derrick Stolee <hidden>
Date: 2022-02-09 14:45:50

On 2/9/2022 2:53 AM, Jean-Noël Avila wrote:

Le 07/02/2022 à 15:10, Derrick Stolee a écrit :

quoted

On 2/6/2022 5:36 AM, Jean-Noël AVILA wrote:

quoted

On Monday, 31 January 2022 16:00:59 CET Derrick Stolee via GitGitGadget wrote:

Hi Jean-Noël. Thanks for your attention to the translatable messages
here:

quoted

error(_("failed to copy '%s' to '%s'; sparse-checkout may not work correctly"),
       from_file, to_file);

quoted

die(_("failed to copy worktree config from '%s' to '%s'"),
     from_file, to_file);

quoted

error(_("failed to unset 'core.bare' in '%s'"), to_file);

quoted

error(_("failed to unset 'core.worktree' in '%s'"), to_file);

quoted

In the first patch of this series, you use _("unable to set '%s' in'%s'). Does it make sense to reuse this string here?

I would argue that "unable to set" is not appropriate for any of these
messages. Perhaps the "failed to copy" messages might be able to use
"unable to set", but the information that the config setting is coming
from settings the user controlled is valuable.

The "failed to unset" means "we are trying to _remove_ this setting
from the config file", so "unable to set" does not seem to work here.

I'm open to revisiting this if you disagree.

Thanks,
-Stolee

Hi Derrick,

Sorry for not being more precise. The first two errors were not the subject of this remark.

For the last two, this is quite surprising that the same function failing (git_config_set_in_file_gently) can lead to different error messages.

In any case, I would argue at least for  shifting to :

    error(_("failed to unset '%s' in '%s'"), 'core.bare", to_file);
and
    error(_("failed to unset '%s' in '%s'"), "core.worktree", to_file);

in order to factorize the message and get the option name out of the way.

Thank you for the clarification! This makes sense as a way to
reduce load on translators. Sorry for misunderstanding.

Thanks,
-Stolee

Re: [PATCH v6 1/6] Documentation: add extensions.worktreeConfig details

From: Elijah Newren <hidden>
Date: 2022-02-09 18:04:29

On Tue, Feb 8, 2022 at 2:20 PM Junio C Hamano [off-list ref] wrote:

quoted hunk

"Derrick Stolee via GitGitGadget" [off-list ref] writes:

quoted

diff --git a/Documentation/config/extensions.txt b/Documentation/config/extensions.txt
index 4e23d73cdca..bccaec7a963 100644
--- a/Documentation/config/extensions.txt
+++ b/Documentation/config/extensions.txt

@@ -6,3 +6,34 @@ extensions.objectFormat::
 Note that this setting should only be set by linkgit:git-init[1] or
 linkgit:git-clone[1].  Trying to change it after initialization will not
 work and will produce hard-to-diagnose issues.
+
+extensions.worktreeConfig::
+     If enabled, then worktrees will load config settings from the
+     `$GIT_DIR/config.worktree` file in addition to the
+     `$GIT_COMMON_DIR/config` file. Note that `$GIT_COMMON_DIR` and
+     `$GIT_DIR` are the same for the main working tree, while other
+     working trees have `$GIT_DIR` equal to
+     `$GIT_COMMON_DIR/worktrees/<id>/`. The settings in the

The mixed use of "worktree" and "working tree" in this paragraph
might confuse readers into thinking that the paragraph is being
careful to make distinction between the two.  All references to
"working tree" in the above paragraph should actually be "worktree",
I would think.

        Side note: "working tree" is in the glossary-content.txt,
        but "worktree", which is one "working tree" + repository
        metadata (i.e. ".git/") that may be partially shared with
        other "worktree"s of a single repository, is not defined.

This is a tangent, but I wonder why we chose to use a different
filename (i.e. not "config" but "config.worktree") for this.  If we
were redoing multi-worktree support from scratch, we would not reuse
the $GIT_DIR used by the primary worktree as $GIT_COMMON_DIR, so
that all worktrees would share a single $GIT_COMMON_DIR and
$GIT_COMMON_DIR/config that has stuff that is shared among all the
worktrees, while per worktree stuff is in $GIT_DIR/config even for
the primary worktree.  But that is all water under the bridge now.

Other than the terminology gotcha, looked sensible.  Migrating
automatically and/or noticing a suspicious setting may be needed to
help end users, but that would not be within the scope of this step.

Attached is a "how about this?" glossary update suggestion.  Most of
the existing mention of "working tree" are fine as-is because they
only care about what is in the "working tree", but some should be
changed to "worktree" to stress the fact that they care not just the
"working tree" part but also the repository metadata part that is
associated with that single "working tree".  The first hunk says
worktree but it is clear that it is interested only in the "working
tree" files.

 Documentation/glossary-content.txt | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git c/Documentation/glossary-content.txt w/Documentation/glossary-content.txt
index c077971335..d816512c6a 100644
--- c/Documentation/glossary-content.txt
+++ w/Documentation/glossary-content.txt

@@ -312,7 +312,7 @@ Pathspecs are used on the command line of "git ls-files", "git
 ls-tree", "git add", "git grep", "git diff", "git checkout",
 and many other commands to
 limit the scope of operations to some subset of the tree or
-worktree.  See the documentation of each command for whether
+working tree.  See the documentation of each command for whether
 paths are relative to the current directory or toplevel.  The
 pathspec syntax is as follows:
 +

@@ -446,7 +446,7 @@ exclude;;
        interface than the <<def_plumbing,plumbing>>.

 [[def_per_worktree_ref]]per-worktree ref::
-       Refs that are per-<<def_working_tree,worktree>>, rather than
+       Refs that are per-<<def_worktree,worktree>>, rather than
        global.  This is presently only <<def_HEAD,HEAD>> and any refs
        that start with `refs/bisect/`, but might later include other
        unusual refs.

@@ -669,3 +669,12 @@ The most notable example is `HEAD`.
        The tree of actual checked out files.  The working tree normally
        contains the contents of the <<def_HEAD,HEAD>> commit's tree,
        plus any local changes that you have made but not yet committed.
+
+[[def_work_tree]]worktree::
+       A repository can have zero (i.e. bare repository) or one or
+       more worktrees attached to it. One "worktree" consists of a
+       "working tree" and repository metadata, most of which are
+       shared among other worktrees of a single repository, and
+       some of which are maintained separately per worktree
+       (e.g. the index, HEAD, per-worktree refs and per-worktree
+       configuration file)

We could also add pseudorefs to the list of things maintained
separately in the final parenthetical comment, but otherwise looks
good.

Re: [PATCH v6 1/6] Documentation: add extensions.worktreeConfig details

From: Eric Sunshine <hidden>
Date: 2022-02-15 20:38:14

On Tue, Feb 8, 2022 at 9:34 PM Derrick Stolee [off-list ref] wrote:

On 2/8/2022 5:20 PM, Junio C Hamano wrote:

quoted

"Derrick Stolee via GitGitGadget" [off-list ref] writes:

quoted

+extensions.worktreeConfig::
+    If enabled, then worktrees will load config settings from the
+    `$GIT_DIR/config.worktree` file in addition to the
+    `$GIT_COMMON_DIR/config` file. Note that `$GIT_COMMON_DIR` and
+    `$GIT_DIR` are the same for the main working tree, while other
+    working trees have `$GIT_DIR` equal to
+    `$GIT_COMMON_DIR/worktrees/<id>/`. The settings in the

The mixed use of "worktree" and "working tree" in this paragraph
might confuse readers into thinking that the paragraph is being
careful to make distinction between the two.  All references to
"working tree" in the above paragraph should actually be "worktree",
I would think.

I generally agree. This was changed in the most-recent re-roll
based on a request by Eric [1]. I'm happy to take whichever
version the two of you settle on.

[1] https://lore.kernel.org/git/CAPig+cS-3CxxyPGcy_vkeN_WYTRo1b-ZhJNdPy8ARZSNKkF1Xg@mail.gmail.com/

"request" is perhaps too strong a word considering that I led in with:

    A few minor comments, which can be addressed later or not
    at all, and likely are not worth holding up the series...

I mentioned "worktree vs. working tree" only to point out the
terminology inconsistency being introduced by the new patch; the same
sort of inconsistency which had bothered Michael Haggerty enough to do
something about it in bc483285b7 (Documentation/git-worktree:
consistently use term "linked working tree", 2015-07-20).

I, personally, prefer the term "worktree" for both convenience and
because it better encapsulates the overall "thing" which is
manipulated by the git-worktree command unlike the term "working tree"
which, as Junio points out, has (perhaps) a more narrow meaning. As
such, I would not be opposed to a patch series which changes "working
tree" to "worktree" in documentation where appropriate, but that's
outside the scope of this series.

Re: [PATCH v5 5/5] worktree: copy sparse-checkout patterns and config on add

From: Eric Sunshine <hidden>
Date: 2022-02-15 22:01:46

On Mon, Feb 7, 2022 at 9:30 AM Derrick Stolee [off-list ref] wrote:

On 2/6/2022 6:30 AM, Eric Sunshine wrote:

quoted

On Mon, Jan 31, 2022 at 10:01 AM Derrick Stolee via GitGitGadget
[off-list ref] wrote:

quoted

+               git config --worktree bogus.key value &&
+               git config --unset core.bare &&

Why is this being unset? (Genuine question. Am I missing something obvious?)

I'm moving it out of the common config file. Earlier commands
enabled it in the config.worktree file for this working tree.

But won't the `git worktree add` commands which immediately follow
this bit automatically drop `core.bare=true` from the common config
file? Or am I misthinking on this? Or are you just trying to be
explicit here with the manual removal?

Re: [PATCH v6 5/6] worktree: copy sparse-checkout patterns and config on add

From: Eric Sunshine <hidden>
Date: 2022-02-15 22:24:12

On Mon, Feb 7, 2022 at 4:33 PM Derrick Stolee via GitGitGadget
[off-list ref] wrote:

quoted hunk

When adding a new worktree, it is reasonable to expect that we want to
use the current set of sparse-checkout settings for that new worktree.
This is particularly important for repositories where the worktree would
become too large to be useful. This is even more important when using
partial clone as well, since we want to avoid downloading the missing
blobs for files that should not be written to the new worktree.
[...]
Signed-off-by: Derrick Stolee <redacted>
---

diff --git a/builtin/worktree.c b/builtin/worktree.c

@@ -335,6 +335,69 @@ static int add_worktree(const char *path, const char *refname,
+       /*
+        * If the current worktree has sparse-checkout enabled, then copy
+        * the sparse-checkout patterns from the current worktree.
+        */
+       if (core_apply_sparse_checkout) {
+               char *from_file = git_pathdup("info/sparse-checkout");
+               char *to_file = xstrfmt("%s/info/sparse-checkout",
+                                       sb_repo.buf);
+
+               if (file_exists(from_file)) {
+                       if (safe_create_leading_directories(to_file) ||
+                           copy_file(to_file, from_file, 0666))
+                               error(_("failed to copy '%s' to '%s'; sparse-checkout may not work correctly"),
+                                     from_file, to_file);
+               }
+
+               free(from_file);
+               free(to_file);
+       }
+
+       /*
+        * If we are using worktree config, then copy all current config
+        * values from the current worktree into the new one, that way the
+        * new worktree behaves the same as this one.
+        */
+       if (repository_format_worktree_config) {
+               char *from_file = git_pathdup("config.worktree");
+               char *to_file = xstrfmt("%s/config.worktree",
+                                       sb_repo.buf);
+
+               if (file_exists(from_file)) {
+                       struct config_set cs = { { 0 } };
+                       const char *core_worktree;
+                       int bare;
+
+                       if (safe_create_leading_directories(to_file) ||
+                           copy_file(to_file, from_file, 0666)) {
+                               error(_("failed to copy worktree config from '%s' to '%s'"),
+                                     from_file, to_file);
+                               goto worktree_copy_cleanup;
+                       }
+
+                       git_configset_init(&cs);
+                       git_configset_add_file(&cs, from_file);
+
+                       if (!git_configset_get_bool(&cs, "core.bare", &bare) &&
+                           bare &&
+                           git_config_set_multivar_in_file_gently(
+                                       to_file, "core.bare", NULL, "true", 0))
+                               error(_("failed to unset 'core.bare' in '%s'"), to_file);
+                       if (!git_configset_get_value(&cs, "core.worktree", &core_worktree) &&
+                           git_config_set_in_file_gently(to_file,
+                                                         "core.worktree", NULL))
+                               error(_("failed to unset 'core.worktree' in '%s'"), to_file);
+
+                       git_configset_clear(&cs);
+               }
+
+worktree_copy_cleanup:
+               free(from_file);
+               free(to_file);
+       }

Given that add_worktree() is already overly long, a good future
cleanup would be to move these two new hunks of functionality into
separate functions -- copy_sparsity() and copy_config(), perhaps -- in
builtin/worktree.c. This should be simple enough since (I think) the
only state that needs to be passed to them is `sb_repo`. But such
cleanup needn't hold up this series.

Re: [PATCH v6 0/6] Sparse checkout: fix bug with worktree of bare repo

From: Eric Sunshine <hidden>
Date: 2022-02-16 00:57:13

On Tue, Feb 8, 2022 at 12:42 AM Eric Sunshine [off-list ref] wrote:

On Tue, Feb 8, 2022 at 12:18 AM Elijah Newren [off-list ref] wrote:

quoted

So, you clearly also read the patches in this round.  Do they also
look good to you?   :-)

I have not yet looked at either the patches or the range-diff, and I
only scanned my eye quickly over the cover letter, but the empty
bullet point made me stop and look a bit more carefully at that part
(and only that part) of the cover letter. Not sure yet when I'll have
time to carefully read this round.

I've now read through the series and, I think, left only one or two
minor not-actionable comments.

Re: [PATCH v5 5/5] worktree: copy sparse-checkout patterns and config on add

From: Derrick Stolee <hidden>
Date: 2022-02-16 13:59:04

On 2/15/2022 5:01 PM, Eric Sunshine wrote:

On Mon, Feb 7, 2022 at 9:30 AM Derrick Stolee [off-list ref] wrote:

quoted

On 2/6/2022 6:30 AM, Eric Sunshine wrote:

quoted

On Mon, Jan 31, 2022 at 10:01 AM Derrick Stolee via GitGitGadget
[off-list ref] wrote:

quoted

+               git config --worktree bogus.key value &&
+               git config --unset core.bare &&

Why is this being unset? (Genuine question. Am I missing something obvious?)

I'm moving it out of the common config file. Earlier commands
enabled it in the config.worktree file for this working tree.

But won't the `git worktree add` commands which immediately follow
this bit automatically drop `core.bare=true` from the common config
file? Or am I misthinking on this? Or are you just trying to be
explicit here with the manual removal?

Ah. Here we are testing that bogus.key gets copied from the
config.worktree file, but core.bare and core.worktree do _not_.

This is kind of like the case where we run two 'git worktree add'
commands in a row. The first one moves core.bare and core.worktree
into the config.worktree file. The second one attempts to copy the
config.worktree file into the new worktree (but must filter out
these config keys).

Thanks,
-Stolee

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help