This is an archive of the discontinued Mercurial Phabricator instance.

hg-core: add basic config module
ClosedPublic

Authored by Alphare on Nov 26 2020, 5:48 AM.

Details

Summary

The config module exposes a Config struct, unused for now.

It only reads the config file local to the repository, but handles all valid
patterns and includes/unsets.
It is structured in layers instead of erasing by reverse order of precedence,
allowing us to transparently know more about the config for debugging purposes,
and potentially other things I haven't thought about yet.

This change also introduces format_bytes! to hg-core.

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

Alphare created this revision.Nov 26 2020, 5:48 AM
pulkit added a subscriber: pulkit.Nov 26 2020, 7:43 AM

I am not sure if had a look at https://phab.mercurial-scm.org/D7575 or not.

This change also introduces format_bytes! to hg-core.

Kindly separate that into a different patch.

I am not sure if had a look at https://phab.mercurial-scm.org/D7575 or not.

I have, I'd even commented at the time. I decided against using this patch, because there was a lot of Facebook-specific work and differed too much from the Python implementation. This is a lot simpler and will grow according to future experimentation

This change also introduces format_bytes! to hg-core.

Kindly separate that into a different patch.

Just to make sure, do you want that I split the Cargo.toml change into a separate patch? This is the first patch that uses the macro in hg-core.

marmoute requested changes to this revision.Nov 26 2020, 9:02 AM
marmoute added a subscriber: marmoute.

This change also introduces format_bytes! to hg-core.

Kindly separate that into a different patch.

Just to make sure, do you want that I split the Cargo.toml change into a separate patch? This is the first patch that uses the macro in hg-core.

Looks like you found other users ;-)

Here is a group of questions and feedbacks.

rust/hg-core/src/config/config.rs
18–22

I don't think we should be talking about layer in the higher level API. Whats your motivation for it ?

46–52

note: I expect we will get more this kind of thing. Wehen we get them is will probably be worth avec a config.utils module somewhere.

92

Why is this called get_option ?

110–111

The layer should be a implementation details, why do we return it there ?

rust/hg-core/src/config/layer.rs
78

Do you mean the higher priority layer have index 0 and the lower have higher index ?

83–84

I liked the original comment better ;-)

137

I would expect this to be passed as reference instead of cloned everytime. What's the motivation to do it this way ?

This revision now requires changes to proceed.Nov 26 2020, 9:02 AM
Alphare marked 2 inline comments as done.Nov 26 2020, 10:12 AM
Alphare updated this revision to Diff 23705.
Alphare added inline comments.Nov 26 2020, 10:12 AM
rust/hg-core/src/config/config.rs
18–22

Yes, that should have been moved to ConfigLayer

92

I suppose get_bool is a better name indeed.

110–111

This should be the private interface, I'll update with the real public one.

rust/hg-core/src/config/layer.rs
78

Yes. Maybe this is not clear?

83–84

Sorry ;)

137

This is due to a limitation in the HashMap's Entry API. I could work around it but I doubt very much the config is going to show up in our profiles. I'll be happily proven wrong when we start using it.

marmoute added inline comments.Nov 26 2020, 11:06 AM
rust/hg-core/src/config/layer.rs
78

Is seems like the wrong order to me. Because it will make adding more layer annoying. Why did you pick this order (especially since happening layers is already giving you the right order).

(Also, not sure it is a good idea to make conversatin as "done" yourself)

Alphare added inline comments.Nov 26 2020, 11:09 AM
rust/hg-core/src/config/layer.rs
78

Why did you pick this order

Because then layers are ordered by their importance, and it made sense to me. Maybe I'm not seeing the broader picture?

(Also, not sure it is a good idea to make conversatin as "done" yourself)

This was Phabricator's doing, sorry!

marmoute added inline comments.Nov 26 2020, 11:36 AM
rust/hg-core/src/config/layer.rs
78

You can apply the very same argument saying that the most important one have the highest index ;-)

Broader picture wise, The benefit of using layered config it make it easy to have temporary override. For example when adding the repository config, or the share config, or internal overrde inside a the code. So these things will keep getting new layers. It will be much simple to add (and drop) layer at the end of the vec than at the start of it.

Bonus point being that we can use an immutable Vec from im_rs and get a very low cost override.

Alphare updated this revision to Diff 23706.Nov 27 2020, 4:31 AM
Alphare updated this revision to Diff 23707.Nov 27 2020, 4:33 AM
Alphare added inline comments.Nov 27 2020, 4:34 AM
rust/hg-core/src/config/layer.rs
78

You can apply the very same argument saying that the most important one have the highest index ;-)

Precisely why I wasn't sure why you'd want it reversed, hehe

It will be much simple to add (and drop) layer at the end of the vec than at the start of it.

Sure, that makes sense, I'll reverse it then.

Bonus point being that we can use an immutable Vec from im_rs and get a very low cost override.

I'm not certain pushing a few bytes to a non-empty Vec will be very important in terms of performance. :)

Alphare updated this revision to Diff 23883.Nov 30 2020, 11:01 AM
Alphare updated this revision to Diff 23902.Dec 1 2020, 8:55 AM

I see comment mentionning feedback taken in account, but no code update. did you forgot to refresh that Diff ?

rust/hg-core/src/config/config.rs
44–50

Comment from the future: When the number of these grows it will probably make sense to move them in their own module. (but this is premature).

92

Was this not supposed to become "get_bool" ?

110–111

seems not updated either

rust/hg-core/src/config/layer.rs
78

If you include all default config this grow a bit further than "a few bytes", but I agree this is a details.

I see comment mentionning feedback taken in account, but no code update. did you forgot to refresh that Diff ?

... yeah it appears that somehow I forgot (or failed) to update this. So I just did and rebased for good measure.

Seems like this one is awaiting an update.

Seems like this one is awaiting an update.

Ok there is definitely something fishy. I remember clearly phabsending the update at least the second time. Thanks Pulkit, I'll see what happens on the third try.

Alphare updated this revision to Diff 24377.Dec 18 2020, 4:07 AM
Alphare added inline comments.Dec 18 2020, 4:13 AM
rust/hg-core/src/config/config.rs
92

Kept get_option and added get_bool because I feel like both are going to be useful.

marmoute accepted this revision.Dec 22 2020, 3:53 AM

Looks good beside the two small nits.

rust/hg-core/src/config/config.rs
92

But why does get_option returns a bool too then ?

118–119

I would probably move all the private method after the pub one for clarity.

This revision was not accepted when it landed; it landed in state Needs Review.
This revision was automatically updated to reflect the committed changes.

@Alphare dropping this patch as it breaks the build. I had to prune it, so kindly hg touch to revive this.

Alphare updated this revision to Diff 24535.Dec 29 2020, 5:01 AM
Alphare updated this revision to Diff 24536.Dec 29 2020, 5:08 AM
Alphare added inline comments.Dec 29 2020, 5:08 AM
rust/hg-core/src/config/config.rs
92

It returns an Option<bool>, it may be useful to know whether the config item existed before being coerced to false.

118–119

Good point.

Queuing it again.