This is an archive of the discontinued Mercurial Phabricator instance.

rhg: Parse per-repository configuration
ClosedPublic

Authored by SimonSapin on Feb 5 2021, 4:26 AM.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

SimonSapin created this revision.Feb 5 2021, 4:26 AM
Alphare added a subscriber: Alphare.Feb 8 2021, 9:38 AM
Alphare added inline comments.
rust/hg-core/src/config/config.rs
28

I'm not a big fan of adding a cache that can become potentially fragile for what I'm assuming to be so little performance gained. Maybe iterating in reverse through the layers until you hit one that is not from the CLI would be good enough?

SimonSapin added inline comments.Feb 8 2021, 11:38 AM
rust/hg-core/src/config/config.rs
28

I was not at all thinking of this as a cache. Do you mean we could filter based on ConfigOrigin? I had not considered that but yeah it’s probably less fragile. Or, what do you think of having three Vecs instead of one? System+user, repo, and CLI layers kept separately.

Alphare added inline comments.Feb 8 2021, 11:46 AM
rust/hg-core/src/config/config.rs
28

Currently, the ordering of layers gives us an easy answer to precedence that we lose if we split into multiple Vec. I say we keep it this way until we feel that this compromise is not worth it anymore and we need another abstraction layer.

SimonSapin added inline comments.Feb 8 2021, 11:52 AM
rust/hg-core/src/config/config.rs
28

With three vecs the precedence logic would be centralized in a method that returns impl Iterator<Item=&ConfigLayer>.

Which do you mean by keep it this way? With RangeFrom<usize>?

Alphare added inline comments.Feb 8 2021, 11:56 AM
rust/hg-core/src/config/config.rs
28

With three vecs the precedence logic would be centralized in a method that returns impl Iterator<Item=&ConfigLayer>.

Right, while it's not particularly harder, I didn't feel like it was needed when adding the config logic. If you think that it'll be frequent enough to query the separate "groups" of layers then by all means, go ahead. :)

Which do you mean by keep it this way? With RangeFrom<usize>?

I meant using a single Vec. I am still -1 on using RangeFrom<usize>.

SimonSapin added inline comments.Feb 8 2021, 12:48 PM
rust/hg-core/src/config/config.rs
28

It’s not frequent, combine_with_repo is the only thing that needs handle these hypothetically-separate vecs separately. And the separation would be purely to make the code simpler and less fragile.

A single vec, do you by with combine_with_repo filtering based on ConfigOrigin?

SimonSapin updated this revision to Diff 25507.Feb 9 2021, 3:07 AM
Alphare added inline comments.Feb 9 2021, 3:41 AM
rust/hg-core/src/config/config.rs
28

Yes, that's how I would do it.

Alphare accepted this revision.Feb 9 2021, 3:42 AM

I saw that you updated the code, looks good to me!

pulkit accepted this revision.Feb 10 2021, 7:14 AM
This revision is now accepted and ready to land.Feb 10 2021, 7:14 AM
This revision was automatically updated to reflect the committed changes.