Re: [PATCH v3 6/6] worktree: copy sparse-checkout patterns and config on add

On Wed, Dec 29, 2021 at 1:39 PM Derrick Stolee [off-list ref] wrote:

On 12/29/2021 2:51 PM, Elijah Newren wrote:

quoted

On Wed, Dec 29, 2021 at 9:31 AM Derrick Stolee [off-list ref] wrote:

quoted

On 12/29/2021 1:37 AM, Eric Sunshine wrote:

quoted

On Tue, Dec 28, 2021 at 4:32 PM Derrick Stolee via GitGitGadget
[off-list ref] wrote:

quoted

quoted

The obvious way to work around this problem is to (again) special-case
`core.bare` and `core.worktree` to remove them when copying the
worktree-specific configuration. Whether or not that is the best
solution or even desirable is a different question. (I haven't thought
it through enough to have an opinion.)

It makes sense to special case these two settings since we want to
allow creating a working worktree from a bare repo, even if it has
worktree config stating that it is bare.

Agreed.

quoted

As far as the implementation goes, we could do the copy and then
unset those two values in the new file. That's an easy enough change.

quoted

I'll wait for more feedback on the overall ideas (and names of things
like the init-worktree-config subcommand).

What value does the init-worktree-config subcommand provide; why
shouldn't we just get rid of it?

I know Eric was strongly suggesting it, but he was thinking in terms
of always doing that full switchover step, or never doing it.  Both
extremes had the potential to cause user-visible bugs, and thus he
suggested providing a command to allow users to pick their poison.  I
provided a suggestion avoiding both extremes that doesn't have that
pick-your-poison approach, so I don't see why forcing users into this
extra step makes any sense.

But perhaps I missed something.  Is there a usecase for users to
explicitly use this?

I think the motivation is that worktree config is something that is
harder to set up than to just run a 'git config' command, and we
should guide users into a best practice for using it. The
documentation becomes "run this command to enable it".

Okay, but that's an answer to a different question -- namely, "if
users want/need to explicitly set it up, why should we have a
command?"  Your answer here is a very good answer to that question,
but you've assumed the "if".  My question was on the "if": (Why) Do
users need or want to explicitly set it up?

Secondarily, if users want to set it up explicitly, is the work here
really sufficient to help guide them?  In particular, I discovered and
started using extensions.worktreeConfig without ever looking at the
relevant portions of git-worktree.txt (the references in
git-config.txt never mentioned them).  I also pushed this usage to
others, including even to you with `git-sparse-checkout`, and no
reviewer on this list ever caught it or informed me of the `proper`
additional guidelines found in git-worktree.txt until this thread
started.  So, relying on folks to read git-worktree.txt for this
config item feels a bit weak to me.  Granted, your new command will be
much more likely to be read since it appears near the top of
git-worktree.txt, but I just don't think that's enough.  The
references to extensions.worktreeConfig in git-config.txt should
reference any special command or extended steps if we expect users to
manually configure it (whether via explicit new subcommand or via also
playing with other config settings).


Anyway, if we think users want to set it up explicitly, and we address
the discoverability problem above, then I'd vote for
"independent-config" or "private-config" (or _maybe_
"migrate-config").  Because:
  * no sense repeating the word `worktree` in `git worktree
init-worktree-config`.  It's redundant.
  * The words "independent" or "private" suggest what it does and why
users might want to use the new subcommand.
  * It's not an "init":
    * The documentation makes no attempt to impose a temporal order of
using this command before `git worktree add`.  (Would we even want
to?)
    * As per my recommendation elsewhere, this step just isn't needed
for the vast majority of users (i.e. those with non-bare clones who
leave core.worktree alone).
    * ...and it's also not needed for other (core.bare=true or
core.worktree set) users since `git worktree add` will automatically
run this config migration for them

And, actually, with the name "independent-config" or "private-config",
I might be answering my own question.  It's a name that speaks to why
users might want it, so my objection to the new command is rapidly
diminishing.

It also provides a place to update the steps if we were to change
something in the future around worktree config, but I'm guessing
the ship has sailed and backwards compatibility will keep us from
introducing a new setting that would need to be added here.

Yeah, the best time to force a config change is probably when we
introduce a new command with it.  If we had forced
core.repositoryFormatVersion=1 in order to read extensions.* at the
time that extensions.* was introduced, that would have been fine (When
we tried it later, we had to revert it for backward compatibility
reasons.).  If we had forced extensions.worktreeConfig=true when
introducing git-worktree, that would have been fine.  We did force
extensions.worktreeConfig=true when we introduced git-sparse-checkout,
which was good, though we localized it to sparse-checkout and avoided
adding it to worktree usage in general at that point.

The other good time to force a config change is when we have cases
where behavior is already broken anyway.  For example, the
(core.bare=true or core.worktree is set) case that started this
thread.  But in such cases, we have to localize the forced-config
change to the cases that are "broken anyway" unless we can verify it
won't break other cases.

I'm not sure that this new subcommand will ever fall into either of
these categories, so I also agree that we'll be unlikely to ever
change it.