Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extract gix::config::tree into crate gix-config-keys #1153

Closed
wants to merge 7 commits into from

Conversation

bittrance
Copy link
Contributor

Draft PR. I'm creating this PR because I appreciate any feedback. So far, all I have done is copy the code from gix/src/config/tree to this module, update use statements and recreate the features that happened to come along. There are many tasks left. I'm now going to start pondering what the actual API surface.

At this point, my only ambition is to add a new crate; integrating the crate with gix needs a transition strategy and additional PRs.

Because it was previously part of gix, the apex predator, it borrowed enums left and right and called them values. In my opinion, the depenency list is too long (and probably contains non-plumbing stuff too). We need a strategy for enums/constants and similar that allows us to remove at least some of the dependencies. In order to get compiling code, I have copied some enums and types (marked with TODOs) that needs to be replaced.

  • Design the crate's public API (additional tasks may be needed here)
  • Clean up the chaotic use situation (I made no attempt to create or retain a common format during migration)
  • Create tasks here for each dependency we want to remove once we have a strategy

Part of addressing #1125 .

@Byron
Copy link
Owner

Byron commented Dec 5, 2023

Thanks so much for getting started on this.

Because it was previously part of gix, the apex predator, it borrowed enums left and right and called them values. In my opinion, the depenency list is too long (and probably contains non-plumbing stuff too). We need a strategy for enums/constants and similar that allows us to remove at least some of the dependencies. In order to get compiling code, I have copied some enums and types (marked with TODOs) that needs to be replaced.

Maybe this means that it's best keep the keys with gix then?
Alternatively, what if such config-keys could be defined by the plumbing crate that handles them? Doing so then might mean that there would need to be a runtime mechanism to assemble the whole tree, or gix just re-assembles the tree by mixing and merging the sub-trees from plumbing crates with constants? One major benefit of the const-ness of the tree is that it can appear in the documentation naturally.

Clean up the chaotic use situation (I made no attempt to create or retain a common format during migration)

It was done by hand and is (meant to be) deliberate, while the term 'chaotic' suggests that a lot went wrong there. As use statements are related to organisation, I am pretty sure this will be one of the easier issues to improve on, and think it will neatly come together once it's clear how the configuration tree 'should be' in the first place.

All these comments are based on the PR's text only, I haven't looked at the change at all just yet.

@bittrance
Copy link
Contributor Author

Alternatively, what if such config-keys could be defined by the plumbing crate that handles them? Doing so then might mean that there would need to be a runtime mechanism to assemble the whole tree, or gix just re-assembles the tree by mixing and merging the sub-trees from plumbing crates with constants? One major benefit of the const-ness of the tree is that it can appear in the documentation naturally.

Distributing this across the plumbing might be possible with some fancy-pants lazy-load-generating macro, but that assumes that all config keys are used by exactly one crate, which might not be true for the contents of .git/config?

The question is perhaps, how much of a goal it is to enable an external party to use individual plumbing crates, rather than depend on gix (i.e. the full tree)? In this case, we are assuming someone out there wants to build tools that works with a user's git config files; a user-friendly TUI perhaps, or a config linter. Here is an approximate list of the concerned types/enums:

gix_diff/blob::Algorithm
gix_filter/CrlfRoundTripCheck
gix_filter/Encoding
gix_filter/eol::Mode
gix_lock/acquire::Fail
gix_negotiate/Algorithm
gix_pack/index::Version
gix_ref/WriteReflog
gix_submodule/FetchRecurse
gix_transport/FollowRedirects
gix_transport/HttpVersion
gix_transport/ProxyAuthMethod
gix_transport/SslVersion
gix_transport/Protocol
gix_transport/ProgramKind

Most of these could be solved with gix doing impl From<ConfigEnum> for TargetEnum { ... }. If external usage of plumbing is a goal, I think it would be manageable for gix to add a bunch of translations from config types/enums into whatever types the configuree needs?

It was done by hand and is (meant to be) deliberate, while the term 'chaotic' suggests that a lot went wrong there.

Sorry, bad choice of words. I just meant that I did not make a serious attempt at either keeping the old style intact, nor tried to apply a new consistent style. sometimes I used super::, sometimes a partial config::... turned into absolute crate::... A use style that works for the new code organization will need to be decided at some point.

@Byron
Copy link
Owner

Byron commented Dec 7, 2023

Distributing this across the plumbing might be possible with some fancy-pants lazy-load-generating macro, but that assumes that all config keys are used by exactly one crate, which might not be true for the contents of .git/config?

It's probably not true, and when I ready that a macro would be needed to make it happen, I think anything with extra-complexity should not be pursued.

The question is perhaps, how much of a goal it is to enable an external party to use individual plumbing crates, rather than depend on gix (i.e. the full tree)?

The directive here is that gix should be good enough to be usable by everyone, and those who don't want to use it or can't are the minority if they exist at all. So it's a group that definitely won't be catered to preemptively, but only on demand.

Most of these could be solved with gix doing impl From<ConfigEnum> for TargetEnum { ... }. If external usage of plumbing is a goal, I think it would be manageable for gix to add a bunch of translations from config types/enums into whatever types the configuree needs?

The more I realise what this would truly mean, I more and more think it shouldn't be done. It's absolutely fine to keep keys in gix, but have them implement a trait from gix-config which allows them to be used natively there. Let's not forget that the goal is to remove the confusion between custom-methods on Snapshot/Mut which should rather be provided directly by gix-config::File.

What do you think? Are you OK abandoning the gix-config-keys idea in favor of a more focussed approach to cut losses?

@bittrance
Copy link
Contributor Author

Yes, it seems having gix-config-keys would incur a real performance and complexity cost for a rather speculative gain. I'll attack the problem from the API end instead.

@bittrance bittrance closed this Dec 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants