Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sparse checkout: fix bug with worktree of bare repo #1101

Closed
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
31 changes: 31 additions & 0 deletions Documentation/config/extensions.txt
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,34 @@ extensions.objectFormat::
Note that this setting should only be set by linkgit:git-init[1] or
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Bagas Sanjaya wrote (reply to this):

On 26/01/22 01.42, Derrick Stolee via GitGitGadget wrote:
> 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.
> 
...
> +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?

-- 
An old man doll... just what I always wanted! - Clara

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 1/26/2022 1:59 AM, Bagas Sanjaya wrote:
> On 26/01/22 01.42, Derrick Stolee via GitGitGadget wrote:
>> 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.
>>
> ...
>> +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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Elijah Newren wrote (reply to this):

On Tue, Jan 25, 2022 at 10:42 AM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Derrick Stolee <dstolee@microsoft.com>
>
> 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 <dstolee@microsoft.com>
> ---
>  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.

> +       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?

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 1/27/2022 1:43 AM, Elijah Newren wrote:
> On Tue, Jan 25, 2022 at 10:42 AM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:

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

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

>>   - `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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Eric Sunshine wrote (reply to this):

On Mon, Jan 31, 2022 at 10:01 AM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> 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 <dstolee@microsoft.com>

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

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 2/8/2022 5:20 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> 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.

> 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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

Derrick Stolee <stolee@gmail.com> writes:

>> +[[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".

"per-worktree ref" is an entry in the glossary.

    [[def_per_worktree_ref]]per-worktree ref::
            Refs that are per-<<def_working_tree,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.

And those other things are also listed as "pseudoref".

    [[def_pseudoref]]pseudoref::
            Pseudorefs are a class of files under `$GIT_DIR` which behave
            like refs for the purposes of rev-parse, but which are treated
            specially by git...

I think the motivation of special casing refs/bisect/ is to allow
use of a separate worktree for bisecting without affecting other
development or another bisection.  The HEAD is singled out in the
description, but MERGE_HEAD and others (pseudoref) that are declared
here to be files under '$GIT_DIR', when we migrate fully to other
backend that may not want to have files under '$GIT_DIR' to
represent them, ought to become per-worktree, for the same reason as
HEAD should be per-worktree, i.e. it allows worktrees to be
independent from each other and have their checkout at different
commits, growing history of different branches in parallel.


Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 2/9/2022 12:19 PM, Junio C Hamano wrote:
> Derrick Stolee <stolee@gmail.com> writes:
> 
>>> +[[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".
> 
> "per-worktree ref" is an entry in the glossary.
> 
>     [[def_per_worktree_ref]]per-worktree ref::
>             Refs that are per-<<def_working_tree,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.
> 
> And those other things are also listed as "pseudoref".
> 
>     [[def_pseudoref]]pseudoref::
>             Pseudorefs are a class of files under `$GIT_DIR` which behave
>             like refs for the purposes of rev-parse, but which are treated
>             specially by git...
> 
> I think the motivation of special casing refs/bisect/ is to allow
> use of a separate worktree for bisecting without affecting other
> development or another bisection.  The HEAD is singled out in the
> description, but MERGE_HEAD and others (pseudoref) that are declared
> here to be files under '$GIT_DIR', when we migrate fully to other
> backend that may not want to have files under '$GIT_DIR' to
> represent them, ought to become per-worktree, for the same reason as
> HEAD should be per-worktree, i.e. it allows worktrees to be
> independent from each other and have their checkout at different
> commits, growing history of different branches in parallel.

Thanks for this additional context! It means that I need to look
around more carefully, not that your patch needs any changes.

-Stolee

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Elijah Newren wrote (reply to this):

On Wed, Feb 9, 2022 at 9:19 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Derrick Stolee <stolee@gmail.com> writes:
>
> >> +[[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".
>
> "per-worktree ref" is an entry in the glossary.
>
>     [[def_per_worktree_ref]]per-worktree ref::
>             Refs that are per-<<def_working_tree,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.
>
> And those other things are also listed as "pseudoref".
>
>     [[def_pseudoref]]pseudoref::
>             Pseudorefs are a class of files under `$GIT_DIR` which behave
>             like refs for the purposes of rev-parse, but which are treated
>             specially by git...
>
> I think the motivation of special casing refs/bisect/ is to allow
> use of a separate worktree for bisecting without affecting other
> development or another bisection.  The HEAD is singled out in the
> description, but MERGE_HEAD and others (pseudoref) that are declared
> here to be files under '$GIT_DIR', when we migrate fully to other
> backend that may not want to have files under '$GIT_DIR' to
> represent them, ought to become per-worktree, for the same reason as
> HEAD should be per-worktree, i.e. it allows worktrees to be
> independent from each other and have their checkout at different
> commits, growing history of different branches in parallel.

You had me worried for a second; things would be really broken if
these pseudorefs weren't per-worktree.

But testing just now, I think the pseudorefs are already per-worktree.
I just did a merge in a secondary worktree, and then observed from the
primary worktree that a .git/worktrees/<id>/MERGE_HEAD was created,
not a .git/MERGE_HEAD.  (Maybe the glossary could just spell out that
these are under $GIT_DIR and _not_ $GIT_COMMON_DIR to avoid potential
confusion?)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Elijah Newren wrote (reply to this):

On Tue, Feb 8, 2022 at 2:20 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > 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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

Elijah Newren <newren@gmail.com> writes:

> But testing just now, I think the pseudorefs are already per-worktree.
> I just did a merge in a secondary worktree, and then observed from the
> primary worktree that a .git/worktrees/<id>/MERGE_HEAD was created,
> not a .git/MERGE_HEAD.  (Maybe the glossary could just spell out that
> these are under $GIT_DIR and _not_ $GIT_COMMON_DIR to avoid potential
> confusion?)

I actually think the longer-term direction is to describe that these
are always per-worktree, without referring to $GIT_DIR or giving any
hints that these may be represented as a file in the filesystem.
That would leave the door open for the reftable backend to take them
over as well as the normal refs.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

Elijah Newren <newren@gmail.com> writes:

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

I think what needs updating is the per_worktree_ref section.  Before
we say "later include other unusual refs", not so unusual pseudorefs
can be mentioned there.

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.
8 changes: 6 additions & 2 deletions Documentation/git-config.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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>::
Expand Down
16 changes: 12 additions & 4 deletions Documentation/git-sparse-checkout.txt
Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,21 @@ COMMANDS
Describe the patterns in the sparse-checkout file.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Elijah Newren wrote (reply to this):

On Tue, Jan 25, 2022 at 10:42 AM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Derrick Stolee <dstolee@microsoft.com>
>
> 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 <allred.sean@gmail.com>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  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?

> +       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?

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


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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 1/27/2022 2:15 AM, Elijah Newren wrote:
> On Tue, Jan 25, 2022 at 10:42 AM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>  '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.

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

>>  '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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Eric Sunshine wrote (reply to this):

On Mon, Jan 31, 2022 at 10:01 AM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> 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 <allred.sean@gmail.com>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>


'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.
+
Expand Down
11 changes: 8 additions & 3 deletions Documentation/git-worktree.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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.:
Expand All @@ -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
Expand Down
28 changes: 13 additions & 15 deletions builtin/sparse-checkout.c
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#include "wt-status.h"
#include "quote.h"
#include "sparse-index.h"
#include "worktree.h"

static const char *empty_base = "";

Expand Down Expand Up @@ -359,26 +360,23 @@ enum sparse_checkout_mode {

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Eric Sunshine wrote (reply to this):

On Tue, Dec 21, 2021 at 2:14 PM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> The previous change added repo_config_set_worktree_gently() to assist
> writing config values into the worktree.config file, especially when
> that may not have been initialized.
>
> When the base repo is bare, running 'git sparse-checkout init' in a
> worktree will create the config.worktree file for the worktree, but that
> will start causing the worktree to parse the bare repo's core.bare=true
> value and start treating the worktree as bare. This causes more problems
> as other commands are run in that worktree.
>
> The fix is to have this assignment into config.worktree be handled by
> the repo_config_set_worktree_gently() helper.
>
> Reported-by: Sean Allred <allred.sean@gmail.com>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
> diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
> @@ -71,6 +71,20 @@ test_expect_success 'git sparse-checkout init' '
> +test_expect_success 'init in a worktree of a bare repo' '

Nit: Although `init` doing an incomplete job of enabling per-worktree
config was indeed the source of the problem, it actually manifested
when invoking other commands, such as `set`. Consequently, it may be
slightly misleading to talk about `init` in the test title. A title
such as "worktree of a bare repo" might be good enough. Anyhow, just a
nit.

> +       test_when_finished rm -rf bare worktree &&
> +       git clone --bare repo bare &&
> +       git -C bare worktree add ../worktree &&
> +       (
> +               cd worktree &&
> +               git sparse-checkout init &&
> +               test_must_fail git config core.bare &&

Nit: I'm rather "meh" on explicitly checking `core.bare` here since
it's not particularly relevant to the test: just testing `init + set`
alone is enough to trigger the bug which begat this patch series.
Future readers of this test might even be confused by the presence of
this `core.bare` check.

> +               git sparse-checkout set /*

The `/*` is expanding to all entries in the root of the filesystem,
which probably isn't what you intended. I suspect you want literal
"/*", in which case you need to quote it:

    git sparse-checkout set "/*"

> +       ) &&
> +       git -C bare config --list --show-origin >actual &&
> +       grep "file:config.worktree      core.bare=true" actual

As mentioned above, I'm fairly meh on this part (and perhaps leaning
toward the negative) since it places too much emphasis on a low-level
detail. I _could_ see this as a test of the new function which
upgrades the repo to per-worktree config, but that's not what _this_
test is about.

> +'

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Eric Sunshine wrote (reply to this):

On Tue, Dec 28, 2021 at 4:32 PM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> The previous change added repo_config_set_worktree_gently() to assist
> writing config values into the worktree.config file, if enabled.

s/worktree.config/config.worktree/

(I made the same mistake several times earlier in this discussion.)

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

"worktree config" is a bit ambiguous here. It might be clearer to say
"enable per-worktree configuration" or "enable worktree-specific
configuration".

>  1. Git will no longer upgrade the repository format and add the
>     worktree config extension. The user should run 'git worktree
>     init-worktree-config' to enable this feature.
>
>  2. If worktree config is disabled, then this command will set the
>     core.sparseCheckout (and possibly core.sparseCheckoutCone and
>     index.sparse) values in the common config file.
>
>  3. If the main worktree is bare, then this command will not put the
>     worktree in a broken state.

There are a few other cases of ambiguity in this enumerated list, as
well, and the need for `s/main worktree is bare/repository is bare/`,
however, if you end up restructuring this series the to follow the
recipe Elijah set forth at the bottom of [1], then most of this
patch's commit message will be rewritten anyhow, so my mention of
these minor issues will be moot.

[1]: https://lore.kernel.org/git/CABPp-BHuO3B366uJuODMQo-y449p8cAMVn0g2MTcO5di3Xa7Zg@mail.gmail.com/

> The main reason to use worktree-specific config for the sparse-checkout
> builtin was to avoid enabling sparse-checkout patterns in one and
> causing a loss of files in another. If a worktree does not have a
> sparse-checkout patterns file, then the sparse-checkout logic will not
> kick in on that worktree.
>
> This new logic introduces a new user pattern that could lead to some
> confusion. Suppose a user has not upgraded to worktree config and
> follows these steps in order:
>
>  1. Enable sparse-checkout in a worktree.
>
>  2. Disable sparse-checkout in that worktree without deleting that
>     worktree's sparse-checkout file.
>
>  3. Enable sparse-checkout in another worktree.
>
> After these steps, the first worktree will have sparse-checkout enabled
> with whatever patterns exist. The worktree does not immediately have
> those patterns applied, but a variety of Git commands would apply the
> sparse-checkout patterns and update the worktree state to reflect those
> patterns. This situation is likely very rare and the workaround is to
> upgrade to worktree specific config on purpose. Users already in this
> state used the sparse-checkout builtin with a version that upgraded to
> worktree config, anyway.
>
> Reported-by: Sean Allred <allred.sean@gmail.com>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>

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;
}
Expand Down
63 changes: 63 additions & 0 deletions builtin/worktree.c
Original file line number Diff line number Diff line change
Expand Up @@ -335,6 +335,69 @@ static int add_worktree(const char *path, const char *refname,
strbuf_addf(&sb, "%s/commondir", sb_repo.buf);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Elijah Newren wrote (reply to this):

On Tue, Jan 25, 2022 at 10:42 AM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Derrick Stolee <dstolee@microsoft.com>
>
> 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 <dstolee@microsoft.com>
> ---
>  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?

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Jean-Noël AVILA wrote (reply to this):

On Monday, 31 January 2022 16:00:59 CET Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <dstolee@microsoft.com>
> 
> 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 <dstolee@microsoft.com>
> ---
>  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?




Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Derrick Stolee wrote (reply to this):

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:

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

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

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

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Jean-Noël Avila wrote (reply to this):

Le 07/02/2022 à 15:10, Derrick Stolee a écrit :
> 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:
> 
>>> error(_("failed to copy '%s' to '%s'; sparse-checkout may not work correctly"),
>>>        from_file, to_file);
> 
>>> die(_("failed to copy worktree config from '%s' to '%s'"),
>>>      from_file, to_file);
> 
>>> error(_("failed to unset 'core.bare' in '%s'"), to_file);
> 
>>> 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
> 

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 2/9/2022 2:53 AM, Jean-Noël Avila wrote:
> Le 07/02/2022 à 15:10, Derrick Stolee a écrit :
>> 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:
>>
>>>> error(_("failed to copy '%s' to '%s'; sparse-checkout may not work correctly"),
>>>>        from_file, to_file);
>>
>>>> die(_("failed to copy worktree config from '%s' to '%s'"),
>>>>      from_file, to_file);
>>
>>>> error(_("failed to unset 'core.bare' in '%s'"), to_file);
>>
>>>> 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
>>
> 
> 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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Eric Sunshine wrote (reply to this):

On Mon, Jan 31, 2022 at 10:01 AM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> 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 <dstolee@microsoft.com>
> ---
> 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?

> +                           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>".

> +       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
>  '

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Eric Sunshine wrote (reply to this):

On Sun, Feb 6, 2022 at 6:30 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Mon, Jan 31, 2022 at 10:01 AM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> > +       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)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 2/6/2022 6:30 AM, Eric Sunshine wrote:
> On Mon, Jan 31, 2022 at 10:01 AM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>> +       /*
>> +        * 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.

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

>> +               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

>> +               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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Eric Sunshine wrote (reply to this):

On Mon, Feb 7, 2022 at 4:33 PM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> 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 <dstolee@microsoft.com>
> ---
> 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.

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;
Expand Down
35 changes: 32 additions & 3 deletions config.c
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
#include "dir.h"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Eric Sunshine wrote (reply to this):

On Tue, Dec 21, 2021 at 2:14 PM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> The previous change added upgrade_to_worktree_config() to assist
> creating a worktree-specific config for the first time. However, this
> requires every config writer to care about that upgrade before writing
> to the worktree-specific config. In addition, callers need to know how
> to generate the name of the config.worktree file and pass it to the
> config API.
>
> To assist, create a new repo_config_set_worktree_gently() method in the
> config API that handles the upgrade_to_worktree_config() method in
> addition to assigning the value in the worktree-specific config. This
> will be consumed by an upcoming change.

I still feel very uncomfortable about this function conflating two
very different concerns (upgrading the repository to per-worktree
config, and the simple setting of a config variable). Since I've
already explained my discomfort and suggested alternatives several
times during this discussion (most recently at [1]), I don't have all
that much more to say. However, I do have a few comments...

First, I would have no problem if this function "did the right thing"
where "the right thing" means correctly choosing between .git/config
and .git/config.worktree depending upon whether or not
`extensions.worktreeConfig` is set, and only that. It should not
perform any sort of repository upgrade on its own. Doing it this way
should satisfy your major concern of relieving callers from having to
figure out the correct configuration file name.

Second, if you absolutely must have this function, as implemented, as
part of the public API (despite my protests), please give it a name
which is sufficiently different from the other "config setter"
functions so that people won't be confused into thinking it's just
another "setter" without realizing that calling it has repository-wide
consequences.

Third, I don't have an objection if you want to make this function
private (static) to builtin/sparse-checkout.c, thus omitting it from
the public API. This way you can have its convenience where you want
it, and we can delay finishing this discussion until such time that it
becomes apparent that other modules want its convenience, as well, if
that ever comes about.

[1]: https://lore.kernel.org/git/CAPig+cRombN-8g0t7Hs9qQypJoY41gK3+kvypH4D0G6EB4JgbQ@mail.gmail.com/

> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
> diff --git a/config.c b/config.c
> index 9c9eef16018..81f3a689c11 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;
> @@ -2880,6 +2881,15 @@ 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)
> +{
> +       return upgrade_to_worktree_config(r) ||
> +              git_config_set_multivar_in_file_gently(
> +                        repo_git_path(r, "config.worktree"),
> +                        key, value, NULL, 0);
> +}
> +
>  void git_config_set(const char *key, const char *value)
>  {
>         repo_config_set(the_repository, key, value);
> diff --git a/config.h b/config.h
> index 5531fc018e3..b05c51b3528 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 into the config.worktree file for the current
> + * worktree. This will initialize extensions.worktreeConfig if necessary,
> + * which might trigger some changes to the root repository's config file.
> + */
> +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.
>   */
> --

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Derrick Stolee <dstolee@microsoft.com>
>
> 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.

Makes sense.

> +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);
> +}

OK.

> @@ -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);
> +}

Is this an unrelated "morally no-op" change?

> +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;
>  }

OK.

>  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);
>  }

Is this an unrelated "morally no-op" change?

It might have value to make caller more explicit by reducing the use
of "I give NULL, you use 'config' for me", but that doesn't sound
related to the addition of set_per_worktree_config_gently() helper.

> 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);
>  
>  /**

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 2/8/2022 5:18 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
...
>> @@ -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);
>> +}
> 
> Is this an unrelated "morally no-op" change?

This one is to match the pattern of how "git_*" methods should
depend on their "repo_*" counterparts (with "the_repository" inserted
properly). So, it's part of the standard process for creating these
"repo_*" variants.

>>  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);
>>  }
> 
> Is this an unrelated "morally no-op" change?
> 
> It might have value to make caller more explicit by reducing the use
> of "I give NULL, you use 'config' for me", but that doesn't sound
> related to the addition of set_per_worktree_config_gently() helper.

Here, you're right. This one should have followed the same pattern
of having the "git_*" equivalent call the "repo_*" method, but
instead I incorrectly inlined some of the code.

The proper body should be

void git_config_set_multivar(const char *key, const char *value,
			     const char *value_pattern, unsigned flags)
{
	repo_config_set_multivar_gently(the_repository, key, value,
					value_pattern, flags);
}

to follow convention.

Thanks,
-Stolee

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

Derrick Stolee <stolee@gmail.com> writes:

> On 2/8/2022 5:18 PM, Junio C Hamano wrote:
>> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> ...
>>> @@ -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);
>>> +}
>> 
>> Is this an unrelated "morally no-op" change?
>
> This one is to match the pattern of how "git_*" methods should
> depend on their "repo_*" counterparts (with "the_repository" inserted
> properly). So, it's part of the standard process for creating these
> "repo_*" variants.

If only one of repo_config_set_multivar_gently() and
git_config_set_multivar_gently() existed and we were completing the
pair, then I would understand the explanation, but the title says
that it is adding repo_config_set_worktree_gently(), which is not,
and that is where the "unrelated" comes from.

It needs to be a separate preparatory step to add
repo_config_set_multivar_gently() before we add
repo_config_set_worktree_gently(), perhaps?

A bit higher level question is if the public part of "config-set"
API functions should gain an "easy" (in the sense of curl_easy_* set
of API functions) API to allow the callers to say "I do not care to
find out if per-worktree configuration is in use, or this particular
variable is meant to be per-worktree, just set it to this value".

On this question, I am of two minds.  As certain variables (like
core.sparseCheckout) should always be per-worktree just like certain
refs (like HEAD) should always be per-worktree, I can understand the
viewpoint that the callers _ought_ to know and explicitly say that
they want to get/set in the per-worktree configuration file, but at
the same time, I would think the callers should not have to care.
So, I dunno.

Thanks.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 2/9/2022 12:49 PM, Junio C Hamano wrote:
> Derrick Stolee <stolee@gmail.com> writes:
> 
>> On 2/8/2022 5:18 PM, Junio C Hamano wrote:
>>> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
>> ...
>>>> @@ -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);
>>>> +}
>>>
>>> Is this an unrelated "morally no-op" change?
>>
>> This one is to match the pattern of how "git_*" methods should
>> depend on their "repo_*" counterparts (with "the_repository" inserted
>> properly). So, it's part of the standard process for creating these
>> "repo_*" variants.
> 
> If only one of repo_config_set_multivar_gently() and
> git_config_set_multivar_gently() existed and we were completing the
> pair, then I would understand the explanation, but the title says
> that it is adding repo_config_set_worktree_gently(), which is not,
> and that is where the "unrelated" comes from.
> 
> It needs to be a separate preparatory step to add
> repo_config_set_multivar_gently() before we add
> repo_config_set_worktree_gently(), perhaps?

True, they could be split. The reason to create the _multivar_
version is for the case that worktree config is not specified,
so that is the only caller at the moment.

> A bit higher level question is if the public part of "config-set"
> API functions should gain an "easy" (in the sense of curl_easy_* set
> of API functions) API to allow the callers to say "I do not care to
> find out if per-worktree configuration is in use, or this particular
> variable is meant to be per-worktree, just set it to this value".
> 
> On this question, I am of two minds.  As certain variables (like
> core.sparseCheckout) should always be per-worktree just like certain
> refs (like HEAD) should always be per-worktree, I can understand the
> viewpoint that the callers _ought_ to know and explicitly say that
> they want to get/set in the per-worktree configuration file, but at
> the same time, I would think the callers should not have to care.
> So, I dunno.

This is an interesting idea. This would require creating a list
of "should be per-worktree" config keys that are checked within
the different *_config_set_* methods.

The biggest technical hurdle is that the multivar versions might
need to send a subset of the given config into the worktree
config and the rest to the common config.

Let's see how this progresses in the future, and whether we
have more config that we believe _must_ be stored on a per-
worktree basis. At that point, we can see whether the current
"distributed responsibility" model is too cumbersome.

Thanks,
-Stolee

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

Derrick Stolee <stolee@gmail.com> writes:

> True, they could be split. The reason to create the _multivar_
> version is for the case that worktree config is not specified,
> so that is the only caller at the moment.

Sorry, but I am not following this part.

> This is an interesting idea. This would require creating a list
> of "should be per-worktree" config keys that are checked within
> the different *_config_set_* methods.

Yes.

> The biggest technical hurdle is that the multivar versions might
> need to send a subset of the given config into the worktree
> config and the rest to the common config.

Yes, instead of having the caller do it.

> Let's see how this progresses in the future, and whether we
> have more config that we believe _must_ be stored on a per-
> worktree basis. At that point, we can see whether the current
> "distributed responsibility" model is too cumbersome.

It is not too distributed, which is a saving grace.  The callers
know they are setting core.sparseCheckout* and they are responsible
to call the per-worktree version.  It would be like having in ref
API an update_HEAD() helper for modifying HEAD, instead of having a
more generic update_ref() that can modify any ref and pass "HEAD" as
an argument to the latter.  The caller needs to know a bit more
details about what needs to happen when dealing with a special thing,
but the special case knowledge is fairly concentrated.

#include "color.h"
#include "refs.h"
#include "worktree.h"

struct config_source {
struct config_source *prev;
Expand Down Expand Up @@ -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);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 12/20/2021 10:57 AM, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <dstolee@microsoft.com>

...

> +	/*
> +	 * Ensure that core.bare reflects the current worktree, since the
> +	 * logic for is_bare_repository() changes if extensions.worktreeConfig
> +	 * is disabled.
> +	 */
> +	if ((res = git_config_set_multivar_in_file_gently(config_filename, "core.bare",
> +							  r->worktree ? "false" : "true",
> +							  NULL, 0))) {
> +		error(_("unable to set core.bare setting in worktree config"));
> +		return res;
> +	}

As mentioned by Eric, this portion isn't correct. It fixes _this_ worktree, but
any other existing worktrees would become broken.

The fix would be to detect if the core config file has core.bare=false and then
to move that setting into the base repo's config.worktree file.

I believe that if we do that change, then the rest of this patch series is valid.

What do others think?

Thanks,
-Stolee

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Eric Sunshine wrote (reply to this):

On Mon, Dec 20, 2021 at 12:32 PM Derrick Stolee <stolee@gmail.com> wrote:
> On 12/20/2021 10:57 AM, Derrick Stolee via GitGitGadget wrote:
> > +     /*
> > +      * Ensure that core.bare reflects the current worktree, since the
> > +      * logic for is_bare_repository() changes if extensions.worktreeConfig
> > +      * is disabled.
> > +      */
> > +     if ((res = git_config_set_multivar_in_file_gently(config_filename, "core.bare",
> > +                                                       r->worktree ? "false" : "true",
> > +                                                       NULL, 0))) {
>
> As mentioned by Eric, this portion isn't correct. It fixes _this_ worktree, but
> any other existing worktrees would become broken.
>
> The fix would be to detect if the core config file has core.bare=false and then
> to move that setting into the base repo's config.worktree file.
>
> I believe that if we do that change, then the rest of this patch series is valid.

Sorry, but I'm not following what you're suggesting, and I'm not sure
what you mean by "core config file" and "base repo's config.worktree
file". Also, we aren't specifically concerned that `core.bare=false`.

Conceptually the proper fix is quite simple. (Whether the actual
implementation is simple is a different question; I haven't looked
closely at the code yet to be able to answer that.) First, though,
let's make clear what different config files are involved:

.git/config -- config shared by the repository and all worktrees
(including the main worktree)

.git/config.worktree - config specific to the main worktree (or to the
repository itself if bare)

.git/worktrees/<id>/config.worktree -- config specific to worktree <id>

In the above list, I'm using ".git/" loosely to mean either a bare
repository (i.e. "bare.git") or the ".git/" directory within the main
worktree; the difference is immaterial to this discussion. When
`extensions.worktreeConfig` is false or unset, only the first item in
the above list is consulted; when `extensions.worktreeConfig` is true,
then the `config.worktree` files are consulted, as well (depending
upon which worktree you're in).

Regarding the actual "fix": we want a new utility function which
enables per-worktree configuration and handles all the required
bookkeeping actions described in git-worktree.txt. Specifically, if
per-worktree configuration is not already enabled, the function will
need to:

(1) set `extensions.worktreeConfig=true` in .git/config

(1) relocate `core.bare` from .git/config to .git/config.worktree if
that key exists

(2) relocate `core.worktree` from .git/config to .git/config.worktree
if that key exists

That's it. It doesn't need to create or touch any
.git/worktrees/<id>/config.worktree file(s); it should _not_ add a
`core.bare=false` to .git/worktrees/<id>/config.worktree, as this v1
patch series does.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Eric Sunshine wrote (reply to this):

On Mon, Dec 20, 2021 at 7:01 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> Regarding the actual "fix": we want a new utility function which
> enables per-worktree configuration and handles all the required
> bookkeeping actions described in git-worktree.txt. Specifically, if
> per-worktree configuration is not already enabled, the function will
> need to:
>
> (1) set `extensions.worktreeConfig=true` in .git/config
>
> (1) relocate `core.bare` from .git/config to .git/config.worktree if
> that key exists
>
> (2) relocate `core.worktree` from .git/config to .git/config.worktree
> if that key exists

A couple additional notes:

First, I can't count to three.

Second, item (0) in the above list would be to upgrade the repository
to version 1 since that's a prerequisite of using `extensions` (which
you know already, but I want to be clear for any other readers that
the new utility function should perform this step, as well).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 12/20/2021 7:01 PM, Eric Sunshine wrote:
> On Mon, Dec 20, 2021 at 12:32 PM Derrick Stolee <stolee@gmail.com> wrote:
>> On 12/20/2021 10:57 AM, Derrick Stolee via GitGitGadget wrote:
>>> +     /*
>>> +      * Ensure that core.bare reflects the current worktree, since the
>>> +      * logic for is_bare_repository() changes if extensions.worktreeConfig
>>> +      * is disabled.
>>> +      */
>>> +     if ((res = git_config_set_multivar_in_file_gently(config_filename, "core.bare",
>>> +                                                       r->worktree ? "false" : "true",
>>> +                                                       NULL, 0))) {
>>
>> As mentioned by Eric, this portion isn't correct. It fixes _this_ worktree, but
>> any other existing worktrees would become broken.
>>
>> The fix would be to detect if the core config file has core.bare=false and then
>> to move that setting into the base repo's config.worktree file.
>>
>> I believe that if we do that change, then the rest of this patch series is valid.
> 
> Sorry, but I'm not following what you're suggesting, and I'm not sure
> what you mean by "core config file" and "base repo's config.worktree
> file". Also, we aren't specifically concerned that `core.bare=false`.
> 
> Conceptually the proper fix is quite simple. (Whether the actual
> implementation is simple is a different question; I haven't looked
> closely at the code yet to be able to answer that.) First, though,
> let's make clear what different config files are involved:
> 
> .git/config -- config shared by the repository and all worktrees
> (including the main worktree)
> 
> .git/config.worktree - config specific to the main worktree (or to the
> repository itself if bare)
> 
> .git/worktrees/<id>/config.worktree -- config specific to worktree <id>
> 
> In the above list, I'm using ".git/" loosely to mean either a bare
> repository (i.e. "bare.git") or the ".git/" directory within the main
> worktree; the difference is immaterial to this discussion. When
> `extensions.worktreeConfig` is false or unset, only the first item in
> the above list is consulted; when `extensions.worktreeConfig` is true,
> then the `config.worktree` files are consulted, as well (depending
> upon which worktree you're in).
> 
> Regarding the actual "fix": we want a new utility function which
> enables per-worktree configuration and handles all the required
> bookkeeping actions described in git-worktree.txt. Specifically, if
> per-worktree configuration is not already enabled, the function will
> need to:
> 
> (1) set `extensions.worktreeConfig=true` in .git/config
> 
> (1) relocate `core.bare` from .git/config to .git/config.worktree if
> that key exists
> 
> (2) relocate `core.worktree` from .git/config to .git/config.worktree
> if that key exists

You are describing (in better detail) what I meant in my message about
what needs to change in this patch.
 
> That's it. It doesn't need to create or touch any
> .git/worktrees/<id>/config.worktree file(s); it should _not_ add a
> `core.bare=false` to .git/worktrees/<id>/config.worktree, as this v1
> patch series does.

Yes, the current patch is incorrect. However, changing just that one
aspect of this patch in the current method (in config.c) should make
it behave the way you are advocating.

I should have a v2 up later today and we can talk in more specifics
about that if you want to wait until then.

Thanks,
-Stolee

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Eric Sunshine wrote (reply to this):

On Mon, Dec 20, 2021 at 10:57 AM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> When adding config values to the worktree config, we might enable the
> extensions.worktreeConfig setting and create the config.worktree file
> for the first time. When the base repository is bare, this creates a
> change of behavior for determining if the worktree is bare or not. A
> worktree off of a bare repository is assumed to be non-bare when
> extensions.worktreeConfig is disabled. When extensions.worktreeConfig is
> enabled but config.worktree is empty, the worktree is considered bare
> because the base repo's core.bare=true setting is used.
>
> To avoid issues like this, create a helper that initializes all the
> right settings in the correct order. A caller will be added in the next
> change.

As discussed already in [1], [2], and [3], the solution implemented by
this patch is undesirable, and I gave an outline in [4] about how I
think the new utility function ought to be implemented instead, so I
won't say anything further about that here. However, I do still have
one or two review comments to make about the general approach taken by
patch. See below...

[1]: https://lore.kernel.org/git/CAPig+cQPUe9REf+wgVNjyak_nk3V361h-48rTFgk6TGC7vJgOA@mail.gmail.com/
[2]: https://lore.kernel.org/git/CAPig+cTVzMtiHzkJq7VRg4Xa3xhrq7KKCdK5OSDY6bvwKu_ynA@mail.gmail.com/
[3]: https://lore.kernel.org/git/6d72a020-ded7-6ef2-825c-ce6421194b26@gmail.com/
[4]: https://lore.kernel.org/git/CAPig+cTuLYFc9fpAe8Uq9fvBYuSGcc9SA1O-q1BRw0DYxDF4Eg@mail.gmail.com/

> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
> diff --git a/config.c b/config.c
> @@ -2880,6 +2880,33 @@ int git_config_set_gently(const char *key, const char *value)
> +int repo_config_set_worktree_gently(struct repository *r,
> +                                   const char *key, const char *value)
> +{
> +       int res;
> +       const char *config_filename = repo_git_path(r, "config.worktree");
> +
> +       /*
> +        * Ensure that core.bare reflects the current worktree, since the
> +        * logic for is_bare_repository() changes if extensions.worktreeConfig
> +        * is disabled.
> +        */
> +       if ((res = git_config_set_multivar_in_file_gently(config_filename, "core.bare",
> +                                                         r->worktree ? "false" : "true",
> +                                                         NULL, 0))) {
> +               error(_("unable to set core.bare setting in worktree config"));
> +               return res;
> +       }
> +       if (upgrade_repository_format(r, 1) < 0)
> +               return error(_("unable to upgrade repository format to enable worktreeConfig"));
> +       if ((res = git_config_set_gently("extensions.worktreeConfig", "true"))) {
> +               error(_("failed to set extensions.worktreeConfig setting"));
> +               return res;
> +       }
> +
> +       return git_config_set_multivar_in_file_gently(config_filename, key, value, NULL, 0);
> +}
> diff --git a/config.h b/config.h
> @@ -253,6 +253,12 @@ void git_config_set_in_file(const char *, const char *, const char *);
> +/**
> + * Write a config value into the config.worktree file for the current
> + * worktree. This will initialize extensions.worktreeConfig if necessary.
> + */
> +int repo_config_set_worktree_gently(struct repository *, const char *, const char *);

I understand your desire to make this "setter" function as transparent
and simple for clients as possible, however, I think it does too much
by conflating two very distinct operations (one which changes the
nature of the repository itself, and one which simply sets a config
variable), and is far too magical. It doesn't help that the function
name gives no indication of just how magical it is, and it is easy to
imagine people calling this function thinking that it's just a simple
"config setter" like all the other similarly-named functions, without
realizing the impact it may have on the repository overall (i.e.
upgrading to version 1 and changing to per-worktree config).

I would feel much more comfortable for the new utility function to
have a single-purpose: namely, to upgrade the repository to a
per-worktree configuration mode (if not already upgraded) as outlined
in [4]. That's it. It shouldn't do anything other than that. This way,
callers which need per-worktree configuration must call the new
function explicitly to obtain the desired behavior, rather than
getting per-worktree configuration as a magical and implicit
side-effect of calling what looks like a plain old "config setter".
This should make it easier to reason about. Taking this approach also
means that you don't need to introduce a special-purpose "config
setter" just for worktrees; instead, you'd use the normal existing
"config setter" functions. For instance, if the new utility function
is named enable_per_worktree_config(), then `git sparse-checkout init`
might do something like this:

    enable_per_worktree_config(r);
    config_path = repo_git_path(r, "config.worktree")
    git_config_set_in_file_gently(config_path, "core.sparseCheckout", ...);
    git_config_set_in_file_gently(config_path, "core.sparseCheckoutCone", ...);

(This, of course, assumes that repo_git_path() latches the changes
made by enable_per_worktree_config() so that it "does the right
thing", but it seems that existing code in `git sparse-checkout init`
is already expecting it to do so, so perhaps it does work that way.)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 12/21/2021 12:53 AM, Eric Sunshine wrote:
> On Mon, Dec 20, 2021 at 10:57 AM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>> When adding config values to the worktree config, we might enable the
>> extensions.worktreeConfig setting and create the config.worktree file
>> for the first time. When the base repository is bare, this creates a
>> change of behavior for determining if the worktree is bare or not. A
>> worktree off of a bare repository is assumed to be non-bare when
>> extensions.worktreeConfig is disabled. When extensions.worktreeConfig is
>> enabled but config.worktree is empty, the worktree is considered bare
>> because the base repo's core.bare=true setting is used.
>>
>> To avoid issues like this, create a helper that initializes all the
>> right settings in the correct order. A caller will be added in the next
>> change.
> 
> As discussed already in [1], [2], and [3], the solution implemented by
> this patch is undesirable, and I gave an outline in [4] about how I
> think the new utility function ought to be implemented instead, so I
> won't say anything further about that here. However, I do still have
> one or two review comments to make about the general approach taken by
> patch. See below...
> 
> [1]: https://lore.kernel.org/git/CAPig+cQPUe9REf+wgVNjyak_nk3V361h-48rTFgk6TGC7vJgOA@mail.gmail.com/
> [2]: https://lore.kernel.org/git/CAPig+cTVzMtiHzkJq7VRg4Xa3xhrq7KKCdK5OSDY6bvwKu_ynA@mail.gmail.com/
> [3]: https://lore.kernel.org/git/6d72a020-ded7-6ef2-825c-ce6421194b26@gmail.com/
> [4]: https://lore.kernel.org/git/CAPig+cTuLYFc9fpAe8Uq9fvBYuSGcc9SA1O-q1BRw0DYxDF4Eg@mail.gmail.com/
> 
>> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
>> ---
>> diff --git a/config.c b/config.c
>> @@ -2880,6 +2880,33 @@ int git_config_set_gently(const char *key, const char *value)
>> +int repo_config_set_worktree_gently(struct repository *r,
>> +                                   const char *key, const char *value)
>> +{
>> +       int res;
>> +       const char *config_filename = repo_git_path(r, "config.worktree");
>> +
>> +       /*
>> +        * Ensure that core.bare reflects the current worktree, since the
>> +        * logic for is_bare_repository() changes if extensions.worktreeConfig
>> +        * is disabled.
>> +        */
>> +       if ((res = git_config_set_multivar_in_file_gently(config_filename, "core.bare",
>> +                                                         r->worktree ? "false" : "true",
>> +                                                         NULL, 0))) {
>> +               error(_("unable to set core.bare setting in worktree config"));
>> +               return res;
>> +       }
>> +       if (upgrade_repository_format(r, 1) < 0)
>> +               return error(_("unable to upgrade repository format to enable worktreeConfig"));
>> +       if ((res = git_config_set_gently("extensions.worktreeConfig", "true"))) {
>> +               error(_("failed to set extensions.worktreeConfig setting"));
>> +               return res;
>> +       }
>> +
>> +       return git_config_set_multivar_in_file_gently(config_filename, key, value, NULL, 0);
>> +}
>> diff --git a/config.h b/config.h
>> @@ -253,6 +253,12 @@ void git_config_set_in_file(const char *, const char *, const char *);
>> +/**
>> + * Write a config value into the config.worktree file for the current
>> + * worktree. This will initialize extensions.worktreeConfig if necessary.
>> + */
>> +int repo_config_set_worktree_gently(struct repository *, const char *, const char *);
> 
> I understand your desire to make this "setter" function as transparent
> and simple for clients as possible, however, I think it does too much
> by conflating two very distinct operations (one which changes the
> nature of the repository itself, and one which simply sets a config
> variable), and is far too magical. It doesn't help that the function
> name gives no indication of just how magical it is, and it is easy to
> imagine people calling this function thinking that it's just a simple
> "config setter" like all the other similarly-named functions, without
> realizing the impact it may have on the repository overall (i.e.
> upgrading to version 1 and changing to per-worktree config).
> 
> I would feel much more comfortable for the new utility function to
> have a single-purpose: namely, to upgrade the repository to a
> per-worktree configuration mode (if not already upgraded) as outlined
> in [4]. That's it. It shouldn't do anything other than that. This way,
> callers which need per-worktree configuration must call the new
> function explicitly to obtain the desired behavior, rather than
> getting per-worktree configuration as a magical and implicit
> side-effect of calling what looks like a plain old "config setter".
> This should make it easier to reason about. Taking this approach also
> means that you don't need to introduce a special-purpose "config
> setter" just for worktrees; instead, you'd use the normal existing
> "config setter" functions. For instance, if the new utility function
> is named enable_per_worktree_config(), then `git sparse-checkout init`
> might do something like this:

I understand your desire to separate these concerns, and maybe there
is value in having another method that _just_ does the "upgrade to
worktree config". However, if we don't also create this helper method
for setting worktree-specific config, then we are going to hit this
issue again.
 
>     enable_per_worktree_config(r);
>     config_path = repo_git_path(r, "config.worktree")
>     git_config_set_in_file_gently(config_path, "core.sparseCheckout", ...);
>     git_config_set_in_file_gently(config_path, "core.sparseCheckoutCone", ...);
> 
> (This, of course, assumes that repo_git_path() latches the changes
> made by enable_per_worktree_config() so that it "does the right
> thing", but it seems that existing code in `git sparse-checkout init`
> is already expecting it to do so, so perhaps it does work that way.)

If we expect every caller that assigns config to the worktree to follow
this sequence of events, then we should encapsulate that in a method so
developers can discover it and call it instead of needing to write these
lines over again. Just repeating the literal "config.worktree" in
multiple places is enough justification for making a helper, let alone
these more subtle issues around bare repos and non-bare worktrees.

The helper method will need clear documentation to say "this will upgrade
the repository format and add extensions.worktreeConfig" so those new
consumers are aware of the full functionality.

Thanks,
-Stolee

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Eric Sunshine wrote (reply to this):

On Tue, Dec 21, 2021 at 8:46 AM Derrick Stolee <stolee@gmail.com> wrote:
> On 12/21/2021 12:53 AM, Eric Sunshine wrote:
> > On Mon, Dec 20, 2021 at 10:57 AM Derrick Stolee via GitGitGadget
> > <gitgitgadget@gmail.com> wrote:
> >> +/**
> >> + * Write a config value into the config.worktree file for the current
> >> + * worktree. This will initialize extensions.worktreeConfig if necessary.
> >> + */
> >> +int repo_config_set_worktree_gently(struct repository *, const char *, const char *);
> >
> > I understand your desire to make this "setter" function as transparent
> > and simple for clients as possible, however, I think it does too much
> > by conflating two very distinct operations (one which changes the
> > nature of the repository itself, and one which simply sets a config
> > variable), and is far too magical. It doesn't help that the function
> > name gives no indication of just how magical it is, and it is easy to
> > imagine people calling this function thinking that it's just a simple
> > "config setter" like all the other similarly-named functions, without
> > realizing the impact it may have on the repository overall (i.e.
> > upgrading to version 1 and changing to per-worktree config).
> >
> > I would feel much more comfortable for the new utility function to
> > have a single-purpose: namely, to upgrade the repository to a
> > per-worktree configuration mode (if not already upgraded) as outlined
> > in [4]. That's it. It shouldn't do anything other than that. This way,
> > callers which need per-worktree configuration must call the new
> > function explicitly to obtain the desired behavior, rather than
> > getting per-worktree configuration as a magical and implicit
> > side-effect of calling what looks like a plain old "config setter".
> > This should make it easier to reason about. Taking this approach also
> > means that you don't need to introduce a special-purpose "config
> > setter" just for worktrees; instead, you'd use the normal existing
> > "config setter" functions. For instance, if the new utility function
> > is named enable_per_worktree_config(), then `git sparse-checkout init`
> > might do something like this:
>
> I understand your desire to separate these concerns, and maybe there
> is value in having another method that _just_ does the "upgrade to
> worktree config". However, if we don't also create this helper method
> for setting worktree-specific config, then we are going to hit this
> issue again.

There are several good reasons for having a single-purpose function
which upgrades to per-worktree config. Not only is it easier to
discover such a function, but it is also easier to reason about the
behavior when it does just this one thing. Moreover, aside from
providing a common implementation for modules which may want or
require the functionality (such as `git sparse-checkout init`), it
would form a solid basis for a git-worktree command for enabling
per-worktree configuration. And, such a command could be valuable for
people who want to utilize per-worktree configuration for their own
reasons (not related to `git-sparse-checkout`).

With only `git sparse-checkout init` wanting to write per-worktree
config, thus far, it does not feel like a convincing argument that an
automagical helper function which conflates upgrading the repository
to per-worktree config _and_ writing a per-worktree config key is a
good idea or that it will be needed again. But more on that below...

> >     enable_per_worktree_config(r);
> >     config_path = repo_git_path(r, "config.worktree")
> >     git_config_set_in_file_gently(config_path, "core.sparseCheckout", ...);
> >     git_config_set_in_file_gently(config_path, "core.sparseCheckoutCone", ...);
>
> If we expect every caller that assigns config to the worktree to follow
> this sequence of events, then we should encapsulate that in a method so
> developers can discover it and call it instead of needing to write these
> lines over again. Just repeating the literal "config.worktree" in
> multiple places is enough justification for making a helper, let alone
> these more subtle issues around bare repos and non-bare worktrees.

On the contrary, because it is such an unusual and potentially
dangerous step to take (i.e. it changes the repository in a way which
third-party tools may not understand), any module which absolutely
_requires_ per-worktree config support should be enabling it
explicitly rather than expecting it to happen implicitly and
magically. By keeping these concerns separate, it is not only easier
for people working on this code in the future to reason about the
behavior, but it also provides a cleaner path for electively aborting
the operation should that ever become desirable. For instance:

    % git sparse-checkout init
    WARNING: This operation will upgrade the repository format to
    WARNING: version 1 and enable per-worktree configuration, thus
    WARNING: potentially making the repository incompatible with
    WARNING: third-party tools.

    Are you sure you want to continue [y/N]?

Your response is also conflating the slight pain of repeated
`repo_git_path(r, "config.worktree")` with the need to upgrade to
per-worktree configuration, which highlights another issue...

The big question is: why does git-sparse-checkout mandate per-worktree
configuration? I haven't followed sparse checkout development closely,
so I may be missing some obvious reasons. I can see why you would want
to _recommend_ and even nudge people to use per-worktree
configuration, which you could do both in the documentation and even
as a "HINT" from the `git sparse-checkout init` command, but
absolutely forcing them into per-worktree configuration seems far too
opinionated for a general-purpose Git command and closes the door
unnecessarily on people who may have quite valid reasons for using
sparse checkout _without_ per-worktree configuration (i.e. perhaps
they only ever use a single worktree -- the main one -- or perhaps
they really do want the sparse checkout to apply to every worktree).
This unconditional enforcement of per-worktree config seems better
suited for `scalar` which is intentionally opinionated.

With the view that `git sparse-checkout init` is too opinionated and
closes doors unnecessarily, then `git sparse-checkout init` should not
be upgrading the repository to per-worktree configuration
unconditionally. Instead, either the documentation should recommend
this step to users; for example:

    It is recommended that sparse checkout be used with per-worktree
    configuration so that each worktree can have its own sparse
    settings. Per-worktree configuration can be enabled with the
    (fictitious) `git worktree config --enable-per-worktree` command:

    % git worktree config --enable-per-worktree
    % git sparse-checkout init

Or, enabling per-worktree configuration could be enabled _on-demand_
by `git sparse-checkout init`; for instance:

    % git sparse-checkout init --per-worktree

Although the immediate discussion is about git-sparse-checkout, this
idea that a command adapts to the repository rather than demanding a
specific repository arrangement, or indeed forcibly changing the
repository arrangement, is far friendlier and leaves doors open which
would otherwise be closed.

It also makes your proposed repo_config_set_worktree_gently() which
"does the right thing" much more palatable since "does the right
thing" no longer means forcibly changing the repository arrangement.
Instead, this convenience function would simply pick the correct
configuration file on behalf of the caller; namely, if
`extensions.worktreeConfig` is disabled, then it writes to
.git/config, whereas if `extensions.worktreeConfig` is enabled, then
it writes to .git/config.worktree or
.git/worktrees/<id>/config.worktree, depending upon the worktree
you're in. That behavior would satisfy your desire to have a
convenience function to modify the correct config file regardless of
whether the repository has per-worktree configuration or not, and
leaves git-sparse-checkout flexible enough to work with or without
per-worktree configuration. I would have no problem with a
repo_config_set_worktree_gently() function which works as described
here, whereas I feel plenty uncomfortable with the
repo_config_set_worktree_gently() implemented by this series.

Referring back to what you said earlier:

    However, if we don't also create this helper method for setting
    worktree-specific config, then we are going to hit this issue
    again. (Stolee)

Yes, we might hit this issue in the future if some command absolutely
requires per-worktree config, however, as outlined above, I think we
should strive as much as possible to make commands work without
requiring special repository arrangement, instead allowing people to
opt-in to repository-wide changes. By avoiding unconditionally
requiring the repository be configured in a particular way, we're less
likely to break third-party tools.

}

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);
Expand Down Expand Up @@ -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);
}

Expand Down
8 changes: 8 additions & 0 deletions config.h
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*/
Expand Down Expand Up @@ -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);

/**
Expand Down
10 changes: 3 additions & 7 deletions sparse-index.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
35 changes: 25 additions & 10 deletions t/t1091-sparse-checkout-builtin.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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
)
Expand Down Expand Up @@ -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 &&
Expand Down Expand Up @@ -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 &&
Expand Down Expand Up @@ -256,7 +271,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
)
'

Expand Down Expand Up @@ -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' '
Expand Down