Sparse checkout: fix bug with worktree of bare repo
Re: [PATCH v6 2/6] worktree: create init_worktree_config()
In reply to Derrick StoleeQuoted 21 lines · Derrick Stolee
Quoted 14 lines · Junio C Hamano
worktree.c int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath, · line 839
Quoted 9 lines · Derrick Stolee via GitGitGadget
worktree.c +9
-0+static int move_config_setting (const char *key,const char *value, +const char *from_file,const char *to_file) +{ +if (git_config_set_in_file_gently (to_file, key, value)) +return error (_ ("unable to set %s in '%s'" ), key, to_file); +if (git_config_set_in_file_gently (from_file, key,NULL )) +return error (_ ("unable to unset %s in '%s'" ), key, from_file); +return 0 ; +}Interesting.
Quoted 5 lines
The verb "move" in its name made me expect a "get (and remove) whatever value(s) defined out of the old file, and set them identically in the new file" sequence, but that is not what is done here. "set to this new single value in the new file and unset from the old one".
Quoted 8 lines
I think this "copy into the worktree-specific config, then remove from the common file" is an important sequence of events in case a concurrent process comes in and reads the two config files in the intermediate state and does not see the config value anywhere.
But perhaps that's not actually what you are concerned about, because you're saying that the 'value' being provided does not actually guarantee that we are moving the setting.
Yes. "Why are we _ignoring_ what is in the old file when we claim to be _moving_?" was the question I had upon seeing this function.
Quoted 10 lines
Quoted 5 lines
I can see the need to say "move it only when its value is X", so having the caller to extract the value before deciding to call the function (hence not "moving from old") does make sense, but then the function is misnamed---it is not "moving", it is doing something else.
I think the end state is correct for all uses here, since we only run this after checking to see if the config value exists in the '
from_file', so 'value' is correct (and this is a static method, not a generally-useful method forconfig.h).
As long as this is used on a single-valued "last one wins" variable, the callers and this helper taken together will do the right thing.
Quoted 4 lines
Perhaps a "
write_in_new_and_remove_from_old()" would be a better, if verbose, name. I struggle to find a less cumbersome name, and "move" seems to match the intent pretty well in the context of its use.
The name is fine as long as the requirement for the caller is made clear. A short comment to help the next reader from having to ask the same question before the helper may be sufficient.
Quoted 8 lines
Quoted 7 lines
Is the assumption that the secondary worktrees are never bare, but the primary one could be (iow, adding worktrees to a bare repository would leave the original bare repository as the primary "worktree" that does not have "working tree")? I am trying to see what downsides it tries to avoid by not moving the core.bare==false setting. Shouldn't core.bare be set to false when "worktree add" creates a new one anyway, if the secondaries are never bare?
Quoted 6 lines
Secondary worktrees cannot be bare. If Git interprets the worktree config to have core.bare=true in a secondary worktree, it errors out.
You seem to be suggesting that we should explicitly write core.bare=false into each of the worktree-specific config files. Is that right? This move is effectively the same, since 'false' is the default.
Unless there is a lower-precedence configuration file that we have to override, yes, not writing core.bare=false upon "worktree add" is fine. I simply do not know if we need to do something special in order to defeat /etc/gitconfig or $HOME/.gitconfig with the repository or the worktree specific configuration file.
Quoted 5 lines
Here, the verbal gymnastics are somewhat necessary because secondary worktrees have a .git _file_, not a git directory, so using "common Git dir" is a way to explicitly reference the Git dir. And the strangeness here is exactly that core.worktree can change this working tree to be something other than the parent of the (common) Git dir.
OK. The .git _file_ is our moral equivalent to a symbolic link, and I forgot about that.
I also wonder if we should do something like what we do for refs (i.e. the API knows which refs are per-worktree and which are global, so the callers do not have to care and just can say things like "update HEAD to this value", and "give me the value of refs/bisect/good") when repo_set_config*() is called, but that is outside the scope of this step, which is about one-time migration.
As the code for migration go, I think I am happy with what it wants to do and how it does it.
Thanks.
Re: [PATCH v6 2/6] worktree: create init_worktree_config()
In reply to Junio C HamanoQuoted 59 lines · Junio C Hamano
worktree.c int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath, · line 839
Quoted 9 lines · Derrick Stolee via GitGitGadget
worktree.c +9
-0+static int move_config_setting (const char *key,const char *value, +const char *from_file,const char *to_file) +{ +if (git_config_set_in_file_gently (to_file, key, value)) +return error (_ ("unable to set %s in '%s'" ), key, to_file); +if (git_config_set_in_file_gently (from_file, key,NULL )) +return error (_ ("unable to unset %s in '%s'" ), key, from_file); +return 0 ; +}Interesting.
The verb "move" in its name made me expect a "get (and remove) whatever value(s) defined out of the old file, and set them identically in the new file" sequence, but that is not what is done here. "set to this new single value in the new file and unset from the old one".
I can see the need to say "move it only when its value is X", so having the caller to extract the value before deciding to call the function (hence not "moving from old") does make sense, but then the function is misnamed---it is not "moving", it is doing something else.
Quoted 5 lines
worktree.c +5
-0+int init_worktree_config (struct repository *r) +{ +int res =0 ; +int bare =0 ; +struct config_set cs = { {0 } };worktree.c int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath, · line 855
Quoted 10 lines
worktree.c +10
-0+const char *core_worktree; +char *common_config_file; +char *main_worktree_file; + +/* +* If the extension is already enabled, then we can skip the +* upgrade process. +*/ +if (repository_format_worktree_config) +return 0 ;OK.
worktree.c int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath, · line 857
Quoted 2 lines
worktree.c +2
-0+if ((res =git_config_set_gently ("extensions.worktreeConfig" ,"true" ))) +return error (_ ("failed to set extensions.worktreeConfig setting" ));OK.
Quoted 9 lines
worktree.c +9
-0+ common_config_file =xstrfmt ("%s/config" , r->commondir); + main_worktree_file =xstrfmt ("%s/config.worktree" , r->commondir); + +git_configset_init (&cs); +git_configset_add_file (&cs, common_config_file); + +/* +* If core.bare is true in the common config file, then we need to +* move it to the main worktree's config file or it will break all
Quoted 8 lines
worktree.c int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath, · line 870
Quoted 3 lines
worktree.c +3
-0+* worktrees. If it is false, then leave it in place because it +* _could_ be negating a global core.bare=true. +*/ Is the assumption that the secondary worktrees are never bare, but the primary one could be (iow, adding worktrees to a bare repository would leave the original bare repository as the primary "worktree" that does not have "working tree")?
Yes, and in fact that was the case which generated the original bug report -- a bare clone where the affected individual started using git worktree add to create some non-primary worktrees (and then also used sparse-checkout in some of them).
Quoted 4 lines
I am trying to see what downsides
it tries to avoid by not moving the core.bare==false setting. Shouldn't core.bare be set to false when "worktree add" creates a new one anyway, if the secondaries are never bare?
Moving the core.bare==false setting might make sense. In the previous discussions, we tried to hypothesize about usage of old git clients and non-git clients (jgit, etc.) on the same repos, and didn't know if some of those would break if they couldn't find a core.bare setting anywhere (since they wouldn't know to look in config.worktree). We needed to migrate core.bare=true to avoid an incorrect value affecting all worktrees (and thus we figured it was worth the risk of breaking older git/non-git clients because having older clients be broken is better than having all clients including current git be broken), but the same wasn't true for core.bare=false. That said, we don't actively know of any such clients that would be hurt by such a migration.
PATCH v6 3/6 config: add repo_config_set_worktree_gently()
In reply to Derrick Stolee via GitGitGadgetFrom: Derrick Stolee <[email protected]>
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.
--- config.c | 35 ++++++++++++++++++++++++++++++++--- config.h | 8 ++++++++ 2 files changed, 40 insertions(+), 3 deletions(-)
config.c +32 -3
@@ -21,6 +21,7 @@#include "dir.h" #include "color.h" #include "refs.h" +#include "worktree.h" struct config_source {struct config_source *prev;
@@ -2884,6 +2885,20 @@ int git_config_set_gently(const char *key, const char *value)return git_config_set_multivar_gently (key, value,NULL ,0 ); } +int repo_config_set_worktree_gently (struct repository *r, +const char *key,const char *value) +{ +/* Only use worktree-specific config if it is is already enabled. */ +if (repository_format_worktree_config) { +char *file =repo_git_path (r,"config.worktree" ); +int ret =git_config_set_multivar_in_file_gently ( + file, key, value,NULL ,0 ); +free (file); +return ret; + } +return repo_config_set_multivar_gently (r, key, value,NULL ,0 ); +} +void git_config_set (const char *key,const char *value) {git_config_set_multivar (key, value,NULL ,0 );
@@ -3181,14 +3196,28 @@ void git_config_set_multivar_in_file(const char *config_filename,int git_config_set_multivar_gently (const char *key,const char *value,const char *value_pattern,unsigned flags) { -return git_config_set_multivar_in_file_gently (NULL , key, value, value_pattern, - flags); +return repo_config_set_multivar_gently (the_repository, key, value, + value_pattern, flags); +} + +int repo_config_set_multivar_gently (struct repository *r,const char *key, +const char *value, +const char *value_pattern,unsigned flags) +{ +char *file =repo_git_path (r,"config" ); +int res =git_config_set_multivar_in_file_gently (file, + key, value, + value_pattern, + flags); +free (file); +return res; }void git_config_set_multivar (const char *key,const char *value,const char *value_pattern,unsigned flags) { -git_config_set_multivar_in_file (NULL , key, value, value_pattern, +git_config_set_multivar_in_file (git_path ("config" ), + key, value, value_pattern, flags); }
config.h +8 -0
@@ -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 );/**
Signature · 1 line
gitgitgadget
Re: [PATCH v6 3/6] config: add repo_config_set_worktree_gently()
Quoted 9 lines · Derrick Stolee via GitGitGadget
From: Derrick Stolee <[email protected]>
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.
config.c int git_config_set_gently(const char *key, const char *value) · line 2900
Quoted 13 lines
config.c +13
-0+int repo_config_set_worktree_gently (struct repository *r, +const char *key,const char *value) +{ +/* Only use worktree-specific config if it is is already enabled. */ +if (repository_format_worktree_config) { +char *file =repo_git_path (r,"config.worktree" ); +int ret =git_config_set_multivar_in_file_gently ( + file, key, value,NULL ,0 ); +free (file); +return ret; + } +return repo_config_set_multivar_gently (r, key, value,NULL ,0 ); +}
OK.
config.c void git_config_set_multivar_in_file(const char *config_filename, · line 3201
Quoted 9 lines
config.c +3
-2@@ -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?
config.c void git_config_set_multivar_in_file(const char *config_filename, · line 3214
Quoted 12 lines
config.c +11
-0+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.
config.c void git_config_set_multivar_in_file(const char *config_filename, · line 3222
Quoted 8 lines
config.c +2
-1void 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.
Quoted 26 lines
config.h +8
-0@@ -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 );/**
Re: [PATCH v6 3/6] config: add repo_config_set_worktree_gently()
Quoted 1 line · Junio C Hamano
"Derrick Stolee via GitGitGadget" <[email protected]> writes:
...
Quoted 11 lines
config.c void git_config_set_multivar_in_file(const char *config_filename, · line 3201
Quoted 9 lines
config.c +3
-2@@ -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.
Quoted 11 lines
config.c void git_config_set_multivar_in_file(const char *config_filename, · line 3222
Quoted 8 lines
config.c +2
-1void 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?
Quoted 3 lines
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
Re: [PATCH v6 3/6] config: add repo_config_set_worktree_gently()
Quoted 15 lines · Derrick Stolee
Quoted 1 line · Junio C Hamano
"Derrick Stolee via GitGitGadget" <[email protected]> writes:
...
Quoted 11 lines
config.c void git_config_set_multivar_in_file(const char *config_filename, · line 3201
Quoted 9 lines
config.c +3
-2@@ -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?
Quoted 4 lines
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 addrepo_config_set_multivar_gently() before we addrepo_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.
Re: [PATCH v6 3/6] config: add repo_config_set_worktree_gently()
Quoted 28 lines · Junio C Hamano
Quoted 15 lines · Derrick Stolee
Quoted 1 line · Junio C Hamano
"Derrick Stolee via GitGitGadget" <[email protected]> writes:
...
Quoted 11 lines
config.c void git_config_set_multivar_in_file(const char *config_filename, · line 3201
Quoted 9 lines
config.c +3
-2@@ -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?
Quoted 4 lines
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()andgit_config_set_multivar_gently()existed and we were completing the pair, then I would understand the explanation, but the title says that it is addingrepo_config_set_worktree_gently(), which is not, and that is where the "unrelated" comes from.
Quoted 3 lines
It needs to be a separate preparatory step to add
repo_config_set_multivar_gently()before we addrepo_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.
Quoted 6 lines
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".
Quoted 7 lines
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
Re: [PATCH v6 3/6] config: add repo_config_set_worktree_gently()
Quoted 3 lines · Derrick Stolee
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.
Quoted 3 lines
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.
Quoted 3 lines
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.
Quoted 4 lines
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.
PATCH v6 4/6 sparse-checkout: set worktree-config correctly
In reply to Derrick Stolee via GitGitGadgetFrom: Derrick Stolee <[email protected]>
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.
--- Documentation/git-sparse-checkout.txt | 16 +++++++++++---- builtin/sparse-checkout.c | 28 +++++++++++++-------------- sparse-index.c | 10 +++------- t/t1091-sparse-checkout-builtin.sh | 4 ++-- 4 files changed, 30 insertions(+), 28 deletions(-)
Documentation/git-sparse-checkout.txt +12 -4
@@ -31,13 +31,21 @@ COMMANDS Describe the patterns in the sparse-checkout file. 'set':: - Enable the necessary config settings - (extensions.worktreeConfig, core.sparseCheckout, - core.sparseCheckoutCone) if they are not already enabled, and - write a set of patterns to the sparse-checkout file from the + Enable the necessary sparse-checkout config settings + (`core.sparseCheckout`, `core.sparseCheckoutCone`, and + `index.sparse`) if they are not already set to the desired values, + and write a set of patterns to the sparse-checkout file from the list of arguments following the 'set' subcommand. Update the working directory to match the new patterns. + +To ensure that adjusting the sparse-checkout settings within a worktree +does not alter the sparse-checkout settings in other worktrees, the 'set' +subcommand will upgrade your repository config to use worktree-specific +config if not already present. The sparsity defined by the arguments to +the 'set' subcommand are stored in the worktree-specific sparse-checkout +file. See linkgit:git-worktree[1] and the documentation of +`extensions.worktreeConfig` in linkgit:git-config[1] for more details. ++ When the `--stdin` option is provided, the patterns are read from standard in as a newline-delimited list instead of from the arguments. +
builtin/sparse-checkout.c +13 -15
@@ -15,6 +15,7 @@#include "wt-status.h" #include "quote.h" #include "sparse-index.h" +#include "worktree.h" static const char *empty_base ="" ;
@@ -359,26 +360,23 @@ enum sparse_checkout_mode {static int set_config (enum sparse_checkout_mode mode) { -const char *config_path; - -if (upgrade_repository_format (1 ) <0 ) -die (_ ("unable to upgrade repository format to enable worktreeConfig" )); -if (git_config_set_gently ("extensions.worktreeConfig" ,"true" )) { -error (_ ("failed to set extensions.worktreeConfig setting" )); +/* Update to use worktree config, if not already. */ +if (init_worktree_config (the_repository)) { +error (_ ("failed to initialize worktree config" ));return 1 ; } - config_path =git_path ("config.worktree" ); -git_config_set_in_file_gently (config_path, -"core.sparseCheckout" , - mode ?"true" :NULL ); - -git_config_set_in_file_gently (config_path, -"core.sparseCheckoutCone" , - mode ==MODE_CONE_PATTERNS ?"true" :NULL ); +if (repo_config_set_worktree_gently (the_repository, +"core.sparseCheckout" , + mode ?"true" :"false" ) || +repo_config_set_worktree_gently (the_repository, +"core.sparseCheckoutCone" , + mode ==MODE_CONE_PATTERNS ? +"true" :"false" )) +return 1 ;if (mode ==MODE_NO_PATTERNS ) -set_sparse_index_config (the_repository,0 ); +return set_sparse_index_config (the_repository,0 );return 0 ; }
sparse-index.c +3 -7
@@ -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;
t/t1091-sparse-checkout-builtin.sh +2 -2
@@ -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 ) '
Signature · 1 line
gitgitgadget
PATCH v6 5/6 worktree: copy sparse-checkout patterns and config on add
In reply to Derrick Stolee via GitGitGadgetFrom: Derrick Stolee <[email protected]>
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.
--- builtin/worktree.c | 63 ++++++++++++++++++++++++++++++ t/t1091-sparse-checkout-builtin.sh | 31 +++++++++++---- t/t2400-worktree-add.sh | 58 ++++++++++++++++++++++++++- 3 files changed, 142 insertions(+), 10 deletions(-)
builtin/worktree.c +63 -0
@@ -335,6 +335,69 @@ static int add_worktree(const char *path, const char *refname,strbuf_addf (&sb,"%s/commondir" , sb_repo.buf);write_file (sb.buf,"../.." ); +/* +* If the current worktree has sparse-checkout enabled, then copy +* the sparse-checkout patterns from the current worktree. +*/ +if (core_apply_sparse_checkout) { +char *from_file =git_pathdup ("info/sparse-checkout" ); +char *to_file =xstrfmt ("%s/info/sparse-checkout" , + sb_repo.buf); + +if (file_exists (from_file)) { +if (safe_create_leading_directories (to_file) || +copy_file (to_file, from_file,0666 )) +error (_ ("failed to copy '%s' to '%s'; sparse-checkout may not work correctly" ), + from_file, to_file); + } + +free (from_file); +free (to_file); + } + +/* +* If we are using worktree config, then copy all current config +* values from the current worktree into the new one, that way the +* new worktree behaves the same as this one. +*/ +if (repository_format_worktree_config) { +char *from_file =git_pathdup ("config.worktree" ); +char *to_file =xstrfmt ("%s/config.worktree" , + sb_repo.buf); + +if (file_exists (from_file)) { +struct config_set cs = { {0 } }; +const char *core_worktree; +int bare; + +if (safe_create_leading_directories (to_file) || +copy_file (to_file, from_file,0666 )) { +error (_ ("failed to copy worktree config from '%s' to '%s'" ), + from_file, to_file); + goto worktree_copy_cleanup; + } + +git_configset_init (&cs); +git_configset_add_file (&cs, from_file); + +if (!git_configset_get_bool (&cs,"core.bare" , &bare) && + bare && +git_config_set_multivar_in_file_gently ( + to_file,"core.bare" ,NULL ,"true" ,0 )) +error (_ ("failed to unset 'core.bare' in '%s'" ), to_file); +if (!git_configset_get_value (&cs,"core.worktree" , &core_worktree) && +git_config_set_in_file_gently (to_file, +"core.worktree" ,NULL )) +error (_ ("failed to unset 'core.worktree' in '%s'" ), to_file); + +git_configset_clear (&cs); + } + +worktree_copy_cleanup: +free (from_file); +free (to_file); + } +strvec_pushf (&child_env,"%s=%s" ,GIT_DIR_ENVIRONMENT , sb_git.buf);strvec_pushf (&child_env,"%s=%s" ,GIT_WORK_TREE_ENVIRONMENT , path); cp.git_cmd =1 ;
t/t1091-sparse-checkout-builtin.sh +23 -8
@@ -146,9 +146,9 @@ test_expect_success 'interaction with clone --no-checkout (unborn index)' ' ' test_expect_success 'set enables config' ' - git init empty-config && + git init worktree-config && ( - cd empty-config && + cd worktree-config && test_commit test file && test_path_is_missing .git/config.worktree && git sparse-checkout set nothing &&
@@ -201,6 +201,21 @@ test_expect_success 'add to sparse-checkout' ' check_files repo "a folder1 folder2" ' +test_expect_success 'worktree: add copies sparse-checkout patterns' ' + cat repo/.git/info/sparse-checkout >old && + test_when_finished cp old repo/.git/info/sparse-checkout && + test_when_finished git -C repo worktree remove ../worktree && + git -C repo sparse-checkout set --no-cone "/*" && + git -C repo worktree add --quiet ../worktree 2>err && + test_must_be_empty err && + new="$(git -C worktree rev-parse --git-path info/sparse-checkout)" && + test_path_is_file "$new" && + test_cmp repo/.git/info/sparse-checkout "$new" && + git -C worktree sparse-checkout set --cone && + test_cmp_config -C worktree true core.sparseCheckoutCone && + test_must_fail git -C repo core.sparseCheckoutCone +' + test_expect_success 'cone mode: match patterns' ' git -C repo config --worktree core.sparseCheckoutCone true && rm -rf repo/a repo/folder1 repo/folder2 &&
@@ -520,13 +535,13 @@ test_expect_success 'interaction with submodules' ' ' test_expect_success 'different sparse-checkouts with worktrees' ' + git -C repo sparse-checkout set --cone deep folder1 && git -C repo worktree add --detach ../worktree && - check_files worktree "a deep folder1 folder2" && - git -C worktree sparse-checkout init --cone && - git -C repo sparse-checkout set folder1 && - git -C worktree sparse-checkout set deep/deeper1 && - check_files repo a folder1 && - check_files worktree a deep + check_files worktree "a deep folder1" && + git -C repo sparse-checkout set --cone folder1 && + git -C worktree sparse-checkout set --cone deep/deeper1 && + check_files repo "a folder1" && + check_files worktree "a deep" ' test_expect_success 'set using filename keeps file on-disk' '
t/t2400-worktree-add.sh +56 -2
64 lines not on this page · raw message
Signature · 1 line
gitgitgadget
Re: [PATCH v6 5/6] worktree: copy sparse-checkout patterns and config on add
Quoted 63 lines · Derrick Stolee via GitGitGadget
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 <[email protected]>---
builtin/worktree.c +52
-0@@ -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,
builtin/worktree.c static int add_worktree(const char *path, const char *refname, · line 396
Quoted 10 lines
builtin/worktree.c +10
-0+ "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.
PATCH v6 6/6 config: make git_configset_get_string_tmp() private
In reply to Derrick Stolee via GitGitGadgetFrom: Derrick Stolee <[email protected]>
This method was created in f1de981e8 (config: fix leaks from git_config_get_string_const(), 2020-08-14) but its only use was in the repo_config_get_string_tmp() method, also declared in config.h and implemented in config.c. Since this is otherwise unused and is a very similar implementation to git_configset_get_value(), let's remove this declaration.
--- config.c | 4 ++-- config.h | 1 - 2 files changed, 2 insertions(+), 3 deletions(-)
config.c +2 -2
@@ -2179,8 +2179,8 @@ int git_configset_get_string(struct config_set *cs, const char *key, char **destreturn 1 ; } -int git_configset_get_string_tmp (struct config_set *cs,const char *key, -const char **dest) +static int git_configset_get_string_tmp (struct config_set *cs,const char *key, +const char **dest) {const char *value;if (!git_configset_get_value (cs, key, &value)) {
config.h +0 -1
@@ -494,7 +494,6 @@ void git_configset_clear(struct config_set *cs);int git_configset_get_value (struct config_set *cs,const char *key,const char **dest);int git_configset_get_string (struct config_set *cs,const char *key,char **dest); -int git_configset_get_string_tmp (struct config_set *cs,const char *key,const char **dest);int git_configset_get_int (struct config_set *cs,const char *key,int *dest);int git_configset_get_ulong (struct config_set *cs,const char *key,unsigned long *dest);int git_configset_get_bool (struct config_set *cs,const char *key,int *dest);
Signature · 1 line
gitgitgadget
Quoted 47 lines · Derrick Stolee via GitGitGadget
This series is now based on v2.35.0 since that contains all of the necessary topics.
This patch series includes a fix to the bug reported by Sean Allred [1] and diagnosed by Eric Sunshine [2].
The root cause is that 'git sparse-checkout init' writes to the worktree config without checking that core.bare or core.worktree are set in the common config file. This series fixes this, but also puts in place some helpers to prevent this from happening in the future.
ATTENTION: I have significantly redesigned the series since previous versions, so most of this cover letter is new.
* Patch 1 updates documentation around extensions.worktreeConfig in a few places to improve discoverability. Several cross links are added to make it easy to find the related areas. (The documentation for the changes to 'git sparse-checkout' are delayed to patch 4.)
* Patch 2 introduces the
init_worktree_config()helper which follows the documented instructions to enable extensions.worktreeConfig as well as move the core.bare and core.worktree config values. This update does not modify core.repositoryFormatVersion, since this is not needed specifically for extensions.worktreeConfig.* Patch 3 adds a new
repo_config_set_worktree_gently()helper method so we can internally adjust a config value within a worktree, at least if extensions.worktreeConfig is enabled. (It will write to the common config file if the extension is not enabled.)* Patch 4 modifies the sparse-checkout builtin to use
init_worktree_config()andrepo_config_set_worktree_gently()in ways that fix the reported bug. The behavior change here is that it will no longer upgrade the repository format version, since that is not needed for extensions.worktreeConfig.* Patch 5 updates 'git worktree add' to copy the worktree config from the current worktree to the new one (while unsetting core.bare=true and core.worktree=*) along with copying the sparse-checkout patterns file.
[1]
https://lore.kernel.org/git/CABceR4bZmtC4rCwgxZ1BBYZP69VOUca1f_moJoP989vTUZWu9Q@mail.gmail.com/
[2]
https://lore.kernel.org/git/CAPig+cQ6U_yFw-X2OWrizB1rbCvc4bNxuSzKFzmoLNnm0GH8Eg@mail.gmail.com/
Quoted 5 lines
Updates in v6
=============* Updated documentation to use "working tree" over "worktree" and "" over ""
Not sure what "" over "" means.
Quoted 3 lines
* Delay some allocations to avoid leaking memory in error conditions.
* Use "main worktree" over "base worktree" in comments.
*
Was the empty bullet point meant to cover the new patch 6? Anyway, comments on the cover letter aside, the patches themselves are:
Quoted 7 lines · Elijah Newren
Quoted 3 lines · Derrick Stolee via GitGitGadget
Updates in v6
* Updated documentation to use "working tree" over "worktree" and "" over ""Not sure what "" over "" means.
Probably related to my review comment[1] about spelling it "$GIT_DIR/worktrees/<id>/" rather than "$GIT_DIR/worktrees/<worktree-name>/".
Quoted 5 lines
Quoted 3 lines
* Delay some allocations to avoid leaking memory in error conditions.
* Use "main worktree" over "base worktree" in comments.
*Was the empty bullet point meant to cover the new patch 6?
The "Updates in v6" section was botched a bit. If you look closely, the remaining bullet points actually ended up in the "Updates in v5" section. The complete "Updates in v6" section should have been (approximately):
* Updated documentation to use "working tree" over "worktree" and "<id>" over "<worktree-name>"
* Delay some allocations to avoid leaking memory in error conditions.
* Use "main worktree" over "base worktree" in comments.
* Removed use of git_configset_get_string_tmp() and added a patch that removes its public declaration.
* Fragile variables are replaced with better ones.
* Variable names and code style improved.
* Several test cleanups in patch 5.
[1]: https://lore.kernel.org/git/[email protected]/T/#m4926177771017bbea82c33e9e03e6a9a004ebf24
Quoted 35 lines · Eric Sunshine
Quoted 7 lines · Elijah Newren
Quoted 3 lines · Derrick Stolee via GitGitGadget
Updates in v6
* Updated documentation to use "working tree" over "worktree" and "" over ""Not sure what "" over "" means.
Probably related to my review comment[1] about spelling it "$GIT_DIR/worktrees/<id>/" rather than "$GIT_DIR/worktrees/<worktree-name>/".
Quoted 5 lines
Quoted 3 lines
* Delay some allocations to avoid leaking memory in error conditions.
* Use "main worktree" over "base worktree" in comments.
*Was the empty bullet point meant to cover the new patch 6?
The "Updates in v6" section was botched a bit. If you look closely, the remaining bullet points actually ended up in the "Updates in v5" section. The complete "Updates in v6" section should have been (approximately):
* Updated documentation to use "working tree" over "worktree" and "<id>" over "<worktree-name>"
* Delay some allocations to avoid leaking memory in error conditions.
* Use "main worktree" over "base worktree" in comments.
* Removed use ofgit_configset_get_string_tmp()and added a patch that removes its public declaration.
* Fragile variables are replaced with better ones.
* Variable names and code style improved.
* Several test cleanups in patch 5.
Quoted 1 line
[1]: https://lore.kernel.org/git/[email protected]/T/#m4926177771017bbea82c33e9e03e6a9a004ebf24
So, you clearly also read the patches in this round. Do they also
look good to you? :-)
Quoted 19 lines · Elijah Newren
Quoted 8 lines · Eric Sunshine
Quoted 1 line · Elijah Newren
Was the empty bullet point meant to cover the new patch 6?
The "Updates in v6" section was botched a bit. If you look closely, the remaining bullet points actually ended up in the "Updates in v5" section. The complete "Updates in v6" section should have been (approximately):
Quoted 9 lines
* Updated documentation to use "working tree" over "worktree" and "<id>" over "<worktree-name>"
* Delay some allocations to avoid leaking memory in error conditions.
* Use "main worktree" over "base worktree" in comments.
* Removed use ofgit_configset_get_string_tmp()and added a patch that removes its public declaration.
* Fragile variables are replaced with better ones.
* Variable names and code style improved.
* Several test cleanups in patch 5.
Quoted 2 lines
So, you clearly also read the patches in this round. Do they also
look good to you? :-)
I have not yet looked at either the patches or the range-diff, and I only scanned my eye quickly over the cover letter, but the empty bullet point made me stop and look a bit more carefully at that part (and only that part) of the cover letter. Not sure yet when I'll have time to carefully read this round.
Quoted 9 lines · Eric Sunshine
Quoted 2 lines · Elijah Newren
So, you clearly also read the patches in this round. Do they also
look good to you? :-)I have not yet looked at either the patches or the range-diff, and I only scanned my eye quickly over the cover letter, but the empty bullet point made me stop and look a bit more carefully at that part (and only that part) of the cover letter. Not sure yet when I'll have time to carefully read this round.
I've now read through the series and, I think, left only one or two minor not-actionable comments.
Quoted 17 lines · Eric Sunshine
Quoted 7 lines · Elijah Newren
Quoted 3 lines · Derrick Stolee via GitGitGadget
Updates in v6
* Updated documentation to use "working tree" over "worktree" and "" over ""Not sure what "" over "" means.
Probably related to my review comment[1] about spelling it "$GIT_DIR/worktrees/<id>/" rather than "$GIT_DIR/worktrees/<worktree-name>/".
Quoted 4 lines
Quoted 3 lines
* Delay some allocations to avoid leaking memory in error conditions.
* Use "main worktree" over "base worktree" in comments.
*
Quoted 6 lines
Quoted 1 line
Was the empty bullet point meant to cover the new patch 6?
The "Updates in v6" section was botched a bit. If you look closely, the remaining bullet points actually ended up in the "Updates in v5" section. The complete "Updates in v6" section should have been (approximately):
Whoops on mixing them up. Sorry about that.
Quoted 2 lines
* Updated documentation to use "working tree" over "worktree" and "<id>" over "<worktree-name>"
this is the issue for the empty quotes. GGG thought these were HTML tags, so made them disappear. I should have used <id> and <worktree-name>.
Quoted 9 lines
* Delay some allocations to avoid leaking memory in error conditions.
* Use "main worktree" over "base worktree" in comments.
* Removed use ofgit_configset_get_string_tmp()and added a patch that removes its public declaration.
* Fragile variables are replaced with better ones.
* Variable names and code style improved.
* Several test cleanups in patch 5.[1]: https://lore.kernel.org/git/[email protected]/T/#m4926177771017bbea82c33e9e03e6a9a004ebf24
Thanks for cleaning up my cover letter!
-Stolee