| Message ID | cf9e86fe3a403d0ceaff9fdf484a9bf6b07799ac.1644269583.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | fe18733927c1d28ffcec3b52908989c591bc7b87 |
| Headers | show |
| Series | Sparse checkout: fix bug with worktree of bare repo | expand |
"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); > > /**
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
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.
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
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.
diff --git a/config.c b/config.c index 2bffa8d4a01..1a03ced1a54 100644 --- a/config.c +++ b/config.c @@ -21,6 +21,7 @@ #include "dir.h" #include "color.h" #include "refs.h" +#include "worktree.h" struct config_source { struct config_source *prev; @@ -2884,6 +2885,20 @@ int git_config_set_gently(const char *key, const char *value) return git_config_set_multivar_gently(key, value, NULL, 0); } +int repo_config_set_worktree_gently(struct repository *r, + const char *key, const char *value) +{ + /* Only use worktree-specific config if it is is already enabled. */ + if (repository_format_worktree_config) { + char *file = repo_git_path(r, "config.worktree"); + int ret = git_config_set_multivar_in_file_gently( + file, key, value, NULL, 0); + free(file); + return ret; + } + return repo_config_set_multivar_gently(r, key, value, NULL, 0); +} + void git_config_set(const char *key, const char *value) { git_config_set_multivar(key, value, NULL, 0); @@ -3181,14 +3196,28 @@ void git_config_set_multivar_in_file(const char *config_filename, int git_config_set_multivar_gently(const char *key, const char *value, const char *value_pattern, unsigned flags) { - return git_config_set_multivar_in_file_gently(NULL, key, value, value_pattern, - flags); + return repo_config_set_multivar_gently(the_repository, key, value, + value_pattern, flags); +} + +int repo_config_set_multivar_gently(struct repository *r, const char *key, + const char *value, + const char *value_pattern, unsigned flags) +{ + char *file = repo_git_path(r, "config"); + int res = git_config_set_multivar_in_file_gently(file, + key, value, + value_pattern, + flags); + free(file); + return res; } void git_config_set_multivar(const char *key, const char *value, const char *value_pattern, unsigned flags) { - git_config_set_multivar_in_file(NULL, key, value, value_pattern, + git_config_set_multivar_in_file(git_path("config"), + key, value, value_pattern, flags); } diff --git a/config.h b/config.h index f119de01309..1d98ad269bd 100644 --- a/config.h +++ b/config.h @@ -253,6 +253,13 @@ void git_config_set_in_file(const char *, const char *, const char *); int git_config_set_gently(const char *, const char *); +/** + * Write a config value that should apply to the current worktree. If + * extensions.worktreeConfig is enabled, then the write will happen in the + * current worktree's config. Otherwise, write to the common config file. + */ +int repo_config_set_worktree_gently(struct repository *, const char *, const char *); + /** * write config values to `.git/config`, takes a key/value pair as parameter. */ @@ -281,6 +288,7 @@ int git_config_parse_key(const char *, char **, size_t *); int git_config_set_multivar_gently(const char *, const char *, const char *, unsigned); void git_config_set_multivar(const char *, const char *, const char *, unsigned); +int repo_config_set_multivar_gently(struct repository *, const char *, const char *, const char *, unsigned); int git_config_set_multivar_in_file_gently(const char *, const char *, const char *, const char *, unsigned); /**