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

[Eric Sunshine <sunshine@sunshineco.com>]

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.