Consumer config confusion

[bittrance]

Current behavior 😯

After #1093 I tried to integrate this new feature into my gitops tool, but concluded that when depending on Gitoxide as a library dependency, the config interface is a bit confusing. This issue collects my observations so far. Feel free to say it should be split into multiple issues, but I suspect that part of the fix is to change the API, which would needs to be established first. It is possible that the various issues reported here should simply be added to the #467 task list and discussion should continue there?

Section/subsection confusion

It seems that this snipet sets credentials.terminalPrompt:

repo.snapshot_mut().set_value(&Gitoxide::Credentials::TERMINAL_PROMPT, "false").unwrap();

Not sure how set_subsection_value is supposed to be used, but it seems unusable for setting values in subsections? All these fail with SubSectionForbidden. (I suppose the method was introduces for those cases where the subsection is e.g. a branch name or similar?)

repo.snapshot_mut().set_subsection_value(
    &Gitoxide::Credentials::TERMINAL_PROMPT,
    "credentials".as_bytes().as_bstr(),
    "false"
)?;
repo.snapshot_mut().set_subsection_value(
    &Gitoxide::Credentials::TERMINAL_PROMPT,
    "gitoxide".as_bytes().as_bstr(),
    "false"
)?;
repo.snapshot_mut().set_subsection_value(
    &Gitoxide::Credentials::TERMINAL_PROMPT,
    "gitoxide.credentials".as_bytes().as_bstr(),
    "false"
)?;

Snapshot/SnapshotMut confusion

Snapshot boolean accessor is

fn boolean(&self, key: impl Into<&BStr>) -> Option<bool>;

but SnapshotMut boolean accessor is

fn boolean(
        &self,
        section_name: impl AsRef<str>,
        subsection_name: Option<&BStr>,
        key: impl AsRef<str>,
    ) -> Option<Result<bool, value::Error>>;

There seems to be no accessors that take 'static dyn Key. (Possibly, Key could be made to implement Into<&Bstr>.)

Expected behavior 🤔

Expectations on current interface

I would have expected

repo.snapshot_mut().set_value(&Gitoxide::Credentials::TERMINAL_PROMPT, "false").unwrap();

to deactivate terminal prompting by setting gitoxide.credentials.terminalPrompt. I would also have expected the API to encourage using Keys as much as possible for type safety.

I would also have expected the set_subsection_value to be able to set values in ordinary "static" subsections.

Suggested new Interface

As a lib consumer, I think that I would prefer the Snapshot to expose just:

/// Get the first value referenced by the key, panicing if `key` is unknown.
fn get_value<K>(key: Into<Key<K>>) -> Option<K>;
/// ...
fn get_values<K>(key: Into<Key<K>>) -> Vec<K>;
/// Get the value referenced by the key, returning and KeyNotFoundError if the key is unknown.
fn try_get_value<K>(key: Into<Key<K>>) -> Result<Option<K>, Error>;
/// ...
fn try_get_values<K>(key: Into<Key<K>>) -> Result<Vec<K>, Error>;

Symmetrical methods for "add" and "free" are presumably needed. The

Additionally, some sort of traversal API would be needed, e.g. iter_keys (or iter_values or perhaps iter_entries).

Similarly `SnapshotMut` would have

```rust
/// Set a known configuration value, panicking if the key is unknown. Returns the previous value.
fn set_value<T: ValueType>(key: Into<Key<T>>, value: Into<Value<T>>) -> Option<Value<T>>;
/// Sets a known configuration value, returning the previous value and KeyNotFoundError if the key is unknown.
fn try_set_value<T: ValueType>(key: Into<Key<T>>, value: Into<Value<T>>) -> Result<Option<Value<T>>, Error>;
/// Sets an arbitrary configuration value, possibly creating the necessary config sections automatically. It returns the previous value.
fn set_arbitrary_value<T: ValueType>(key: Into<impl &'b BStr>, value: Into<Value<T>>) -> Option<Value<T>>;

In this marvelous world, there would be a impl Into<Key<K>> for &BStr and impl Into<Key<K>> for &str so that you can just pass in data you received from the user without sacrificing type safety. Presumably, the caller would have to construct a key instance for e.g. the branch.<name>.remote subsection by passing in the name.

I suspect that gix the command needs at least some of the various methods that exist today, so it might be reasonable to either have a sub-trait for either case, or perhaps two accesseor methods config_snapshot(_mut) and config_???_snapshot(_mut).

Git behavior

When considering a new API, git-config reports the following actions:

Action
    --get                 get value: name [value-pattern]
    --get-all             get all values: key [value-pattern]
    --get-regexp          get values for regexp: name-regex [value-pattern]
    --get-urlmatch        get value specific for the URL: section[.var] URL
    --replace-all         replace all matching variables: name value [value-pattern]
    --add                 add a new variable: name value
    --unset               remove a variable: name [value-pattern]
    --unset-all           remove all matches: name [value-pattern]
    --rename-section      rename section: old-name new-name
    --remove-section      remove a section: name
    -l, --list            list all
    --fixed-value         use string equality when comparing values to 'value-pattern'
    -e, --edit            open an editor
    --get-color           find the color configured: slot [default]
    --get-colorbool       find the color setting: slot [stdout-is-tty]

Steps to reproduce 🕹

#[test]
fn set_value() -> crate::Result {
    let mut repo = named_repo("make_config_repo.sh")?;
    let mut config = repo.config_snapshot_mut();
    config.set_value(&Credentials::TERMINAL_PROMPT, "false")?;
    config.commit()?;
    assert_eq!(repo.config_snapshot().boolean("gitoxide.credentials.terminalPrompt"), Some(false));
    // This passes:
    // assert_eq!(repo.config_snapshot().boolean("credentials.terminalPrompt"), Some(false));
    Ok(())
}

#[test]
fn set_subsection_value() -> crate::Result {
    let mut repo = named_repo("make_config_repo.sh")?;
    let mut config = repo.config_snapshot_mut();
    config.set_subsection_value(&Credentials::TERMINAL_PROMPT, "credentials".as_bytes().as_bstr(), "false")?;
    config.commit()?;
    // Boom!
    assert_eq!(repo.config_snapshot().boolean("gitoxide.credentials.terminalPrompt"), Some(false));
    Ok(())
}

[bittrance]

I'll be happy to address these issues once we have an idea of what scope the change should be.

[Byron]

Thanks a lot for this collection of issues - I am glad you did it as config is always a little bit neglected until the is an external stimulus for improvements, apparently 😅.

It is possible that the various issues reported here should simply be added to the #467 task list and discussion should continue there?

That could indeed be done once concrete tasks emerge, which could then be tackled one at a time. I'd expect most tasks be related to how gix exposes the configuration though - let's see (I haven't read the rest of this issue yet).

About: Section/subsection confusion

repo.snapshot_mut().set_value(&Gitoxide::Credentials::TERMINAL_PROMPT, "false").unwrap();

This should work and the issue is addressed in the sibling-PR.

Not sure how set_subsection_value is supposed to be used, but it seems unusable for setting values in subsections? All these fail with SubSectionForbidden. (I suppose the method was introduces for those cases where the subsection is e.g. a branch name or similar?)

Yes, that's what it is there for. One could argue that maybe there are more intuitive ways to do this. But in general, keys need to be configured to work with it.

https://github.com/Byron/gitoxide/blob/1f5f695407b034377d94b172465ff573562b3fc3/gix/src/config/tree/sections/branch.rs#L7-L8

So at the very least, the name of the method could change to clearly express that the subsection isn't 'statically' known.

I think once there is a more general concept on how to use the static key hierarchy, this could be made so there is no such distinction at all. I can imagine to not even use config::tree::Key directly, but have a custom Key type that is implemented by more types. This would allow it to be constructed more flexibly, allowing strings as input, and static keys from the configuration hierarchy.

About: Snapshot/SnapshotMut confusion

Probably that issue would go away once Snapshot overrides provides its own interface that completely improves on what gix-config can offer. Maybe gix-config can also be made so that it is nice enough for gix, maybe with an extra trait for key.

The goal here would be to either not be forced to override anything in Snapshot or SnapshotMut to provide direct access to the underlying gix_config::File methods, or to override everything and not have to implement Deref/DerefMut at all.

Of course it would be best if gix-config would improve so it's more usable to everyone.

About: Expectation

I would also have expected the set_subsection_value to be able to set values in ordinary "static" subsections.

I think having a separate method for subsections shouldn't even be necessary and would hope that an actual Key trait, maybe even coming from gix-config could make all this work very fluently and consistently.

About: Suggested new interface

In this marvelous world, there would be a impl Into<Key<K>> for &BStr and impl Into<Key<K>> for &str so that you can just pass in data you received from the user without sacrificing type safety. Presumably, the caller would have to construct a key instance for e.g. the branch.<name>.remote subsection by passing in the name.

The Key could always be some type which carries the needed information. For example I can imagine something like Branch::MERGE.with_name("branch-name") to return something that implements Key and thus describes branch.branch-name.merge.

Parameterizing the return value wasn't nice to use at all, and is present in gix_config::File at least in principle. There is probably no harm in having it though, even though in practice I keep calling the specific typed methods like boolean() as it's most of the time sufficiently clear what is what.

I suspect that gix the command needs at least some of the various methods that exist today, so it might be reasonable to either have a sub-trait for either case, or perhaps two accesseor methods config_snapshot(_mut) and config_???_snapshot(_mut).

I agree that in order to prevent Deref confusion, greater separation is a way of solving the issue. Another way would be to make gix-config so good that one can use it nicely from gix as well. I have a feeling that's actually the case if one can make it happen in addition to the various (section, subsection, key) methods which are the basis for everything.

Conclusion/Next Steps

Yes, this can be improved and I have made a few suggestions on top of the ones presented here. Ideally, there is a way to have a convenient API in gix-config already, and re-use it from gix where possible to avoid deref-confusion. Lastly, with such improvement, set_subsection_value() should not be needed anymore.

Becoming API-complete to have a 1:1 match with git that would make implementing gix config with similar flags trivial wouldn't necessarily be my goal here. Once one is actually implementing those, one can make improvements then if truly needed. Right now, gix config doesn't support making changes at all, as gix doesn't yet provide a way to write configuration back (even though there are plans and ideas).

I'll be happy to address these issues once we have an idea of what scope the change should be.

Thanks! I wrote a lot and maybe some of it 'clicks' or seems useful enough to go with it.

Please let me know what you think.

[bittrance]

Sorry for the silence. I'm thinking that the first step is to incrementally clean up the public interface of gix-config to something that makes it more useful on its own. I'm currently reading sources and trying things out. Hopefully, I'll be able to offer a sketch of the interface in a week or so.

[bittrance]

How about these as first steps:

• move the config key tree into gix-config (or a new gix-config-tree crate or even gix-config-value ?)
• move Snapshot and SnapshotMut to gix-config, which implies that they can no longer know about Repository and need some other way to commit
• probably some cleanup in gix to make config use sane

I think we agree that "the gix config crate" gix-config should have the ability to support isolated read semantics, mutation and type-safe access. These steps moves those capabilities into gix-config. After that, we can move on to considering what an ideal API for gix-config looks like. How does that sound?

[Byron]

Making the config-tree available as plumbing seems like a good idea! Having less code in gix is always appreciated, and making certain plumbing more standalone is also beneficial for everyone.

With Snapshot/Mut I am entirely unconvinced this has a place in gix-config at least for now, even though I can imagine it to be a 'transaction' utility that if generalised could have a place in plumbing. Even if so, I don't see how doing that would solve (a huge) problem.

• probably some cleanup in gix to make config use sane

This is a bit vague but I agree that generally attribute access is involved and cumbersome. Thus far I was unable to make it better, but also didn't find that the complexity is accidental.

The uncontested next step would then be the creation of a new gix-config-tree crate which contains all code and tests of gix::config::tree. It's busy-work though that won't affect any issue presented here, but it's the foundation for then integrating the use of Keys more tightly into gix-config.

I believe that this should not be done by depending on gix-config-tree though, but instead to have a separate trait for this. Instead of having Into<&BStr> one might have impl Key. Then Snapshot/Mut doesn't have to provide its own access or mutation anymore which should resolve some of the confusion mentioned here.

[bittrance]

I see. So gix-config abstracts over the actual git configuration. It can be used to implement any git config parameter set🙂 Presumably gix-config-tree depends on gix-config for pub trait Key and possibly some error types.

That sort of implies that the gitoxide section should be provided separately, since a third-party using these crates may not use gix. It could be feature-toggled, perhaps?

On the crate name, is the tree "aspect" worth emphasizing in this new crate? gix-config-keys might be more descriptive and more symmetric with gix-config-values?

(I'll make a draft PR on a new crate essentially containing gix/src/config/tree/ in a few days.)

[Byron]

Presumably gix-config-tree depends on gix-config for pub trait Key and possibly some error types.

Yes, it would implement that trait and it probably needs one specific error type which can also be obtained from the gix-config-value crate. Maybe that Key trait could also be put into gix-config-value so gix-config-tree can stay even more minimal. gix-config-value is used by gix-config as well. However, it's not the end of the world if gix-config-tree depended on gix-config just for a trait impl, it just feels a little wasteful. Maybe gix should bring both together… even though that might run into 'can't implement foreign trait for foreign type'-issue.

That sort of implies that the gitoxide section should be provided separately, since a third-party using these crates may not use gix. It could be feature-toggled, perhaps?

That's a good catch! I'd avoid the extra complexity though and put a foot down, claiming that gitoxide keys are part of the package, it's a gix-* crate.

On the crate name, is the tree "aspect" worth emphasizing in this new crate? gix-config-keys might be more descriptive and more symmetric with gix-config-values

I love that crate name! gix-config-keys it must be :D.

(I'll make a draft PR on a new crate essentially containing gix/src/config/tree/ in a few days.)

Awesome, thanks so much for your help!

[bittrance]

There is now a draft pull request #1153 setting up a new gix-config-keys. No brain was engaged during the production of this code; it's all copy-paste and whacking till it compiles. I'll now try to understand what a reasonable API would look like. All feedback is welcome.

[bittrance]

I'm well into an implementation where the gix-config crate exports a Key trait that you implement to interact with its API. This appears to be going reasonably well, but I realized gix-submodule uses gix-config to parse the .gitmodules file, which would then need to have its own Key implementation or share the implementation with gix . At the moment, I'm leaning towards kicking the can down the road by having gix-config providing a plain/generic key implementation. Opinions?

[Byron]

That's great to hear and I am looking forward to taking a look once you are ready.

At the moment, I'm leaning towards kicking the can down the road by having gix-config providing a plain/generic key implementation. Opinions?

The way I imagined the Key trait, it would be implemented for &BStr and &str and various others for convenience, and that it would replace the &str in the *_by_key() methods. gix-submodule wouldn't need to do anything special that way, everything would work out of the box. What am I missing?

[bittrance]

Well, I'm thinking that gix will still want to provide its own validating and documentation-friendly hierarchy of typed keys, so implementing Key on bstr et al would really only be for gix-submodule's benefit? Also, said impl Key for bstr { ... } would have to parse the key every time you pass it to gix-config (all lookup in gix-config wants to know section and subsection since it organizes its storage by section internally). gitoxide seems to put quite a premium on performance, so I was thinking a dedicated struct that tracks name, section and subsection separately was in order (though I don't think gix-submodule cares about subsections)?

[Byron]

Well, I'm thinking that gix will still want to provide its own validating and documentation-friendly hierarchy of typed keys, so implementing Key on bstr et al would really only be for gix-submodule's benefit?

That's how gix itself chooses to handle configuration access, and it does have benefits to do so, but it's considered optional for anybody else. People should just be able to type in the key as string or build it dynamically, just like they can with git2.

gitoxide seems to put quite a premium on performance, so I was thinking a dedicated struct was in order?

I don't think parsing speed for these keys matters at all. It's my guess that before this shows up in the profile, everything else shows up before, particularly allocations. Did you know that this implemetnation of gix-config currently allocates a string for each parsed piece of the configuration? It's quite inefficient and I never planned it to be that way, but it is what it is and it's not a problem in practice.

So I wouldn't worry about it and keep everything working as is. There also is a dedicated Key struct already which I think you meant, but I didn't 'read' it that way if that makes sense.

I hope that helps, and thanks again for tackling this!