- Notifications
You must be signed in to change notification settings - Fork2.5k
Separate config reader and writer backend priorities (for worktree configs)#6756
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Introduce the logical concept of a worktree-level config. The newvalue sits between _LOCAL and _APP to allow `git_config_get_*` to'just work'.The assumption of how `git_config_get_*` works was testedexperimentally by setting _WORKTREE to some nonsense value (like -3)and watching the new test fail.
Now that GIT_CONFIG_LEVEL_WORKTREE exists logically, define and load$GIT_DIR/config.worktree into that level.
Now that worktree-level configuration can be read from disk andmanipulated in memory, we should be able to say we support'extensions.worktreeConfig'.
This structure provides for cleaner diffs in upcoming commits.
It would seem that `get_backend_for_use` is primarily used whenwriting config data -- either to set keys or delete them (based on thepossible values of `backend_use`). When git-config(1) is used forside-effects, it will modify only the local (repository-level)configuration unless explicitly overridden.From git-config(1): --local For writing options: write to the repository .git/config file.This is the default behavior.`get_backend_for_use` does not have the ability to specify a configlevel and typically is expected (it seems) to 'do the right thing'.Taking its cue from git-config(1), don't update worktree-specificconfig unless it's the only option.If that functionality is needed by consumers, I assume they would findthe appropriate backend with `git_config_open_level` and feed that`git_config` object through to the `git_config_set_*` functions (asdemonstrated in the provided test).
When deleting a key from a repository with multiple configbackends (like a --local config and a --worktree config), it'simportant to return the correct backend to modify.This patch ensures that we don't return a backend that is incapable ofdeleting a given piece of configuration when that is the required use.
Before passing the config key name to backend->get(), it needs to benormalized first. Not all current callers are performing thisnormalization, so perform it centrally instead.Co-authored-by: Brian Lyles <brianmlyles@gmail.com>
This looks like a simple copy/paste error.
Now that get_backend_for_use can return other error codes (by virtueof key-name normalization), make sure to propagate the appropriateerror code when used.
`cl_git_fail_with` is preferred over asserting a return value; theformer gives more detailed information about the mismatch.
b78bb4f
to1fcc5de
CompareConfiguration read order and write order should be separated. Forexample: configuration readers have a worktree level that is higherpriority than the local configuration _for reads_. Despite that, theworktree configuration is not written to by default.Use a new list, `writers`, to identify the write target.To do this, we need another level of indirection between backendinstances (which are refcounted and shared amongst different git_configinstances) and the config reader/writer list (since each of thosedifferent git_config instances can have different read/writepriorities).
Introduce `GIT_EREADONLY` and return it when a read-only configurationis attempted to be mutated. This is preferred over the prior`GIT_ENOTFOUND` which is not accurate.
When a configuration transaction is freed with `git_transaction_free` -without first committing it - we should not `git_config_free`. We neverincreased the refcount, so we certainly shouldn't decrease it.Also, add a test around aborting a transaction without committing it.
When we `git_config_unlock`, we shouldn't _unlock the thing that welocked_ instead of assuming that the highest-priority target _remains_the highest-priority target.
When we start a transaction, we should refcount the backend so that wedon't lose it. This prevents the case where we have a transaction openand a configuration entry locked and somebody forces a new backend atthe same level and the backend is freed. Now a user can gently wind downtheir transaction even when the backend has been removed from the liveconfiguration object.
1fcc5de
toe8910c1
Compareethomson commentedMar 11, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Fixed! |
Only read the worktree configuration when `extensions.worktreeconfig` isset to true.
Instead of reading the worktree configuration from the commondir, readit from the gitdir. This ensures that each worktree gets its ownconfiguration.
cc@vermiculus - curious if you have any feedback here. |
Going to merge this, I think it’s a nice little api change. Thanks again@vermiculus |
@ethomson apologies for the lack of response, both myself and@vermiculus got a little busy and this slipped through. Thanks for running with this! |
For#6202, we add additional customization on read/write priority, so that we can have independent read and write ordering. This allows us to send writes to the local configuration backendeven if there is a higher-priority read backend (like a worktree configuration). Repositories will configure the read/write order specifically, and users can add a
APP
level configuration which takes priority (over both). Users can further tweak priority themselves.