Re: [PATCH v2 4/5] config: add repo_config_set_worktree_gently()

[Eric Sunshine <sunshine@sunshineco.com>]

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