Confusing configuration sources might lead to (more) development errors

[nardoor]

Context and description

rustic has three complementary ways of being prompted a configuration.

• the CLI arguments
• the environment variables
• the TOML configuration profile file

the current design of rustic makes it that the implementation of command (such as forget or webdav) has access to two different configurations:

• a static global RUSTIC_APP.config() where all the sources were merged
• a &self reference to the CLI (clap parsed) instance that only has the CLI

Example

issue: #1163
fix PR: #1241

code before the fix (https://github.com/rustic-rs/rustic/blob/abf1835cbdf5804806a1106cfb88b4cd3b20e0d9/src/commands/webdav.rs#L68C1-L104C1):

impl WebDavCmd {
    // [+] &self is a reference to a `WebDavCmd` where the fields are only representing the CLI args
    // we musn't use it because it lacks the config from `ENV` or `TOML`
    fn inner_run(&self) -> Result<()> {
        // [+] this config is complete, the TOML, ENV and CLI were merged to produce this config struct instance.
        // the correct `WebDavCmd` can be found in `RUSTIC_APP.config().webdav`
        let config = RUSTIC_APP.config();
        let repo = open_repository_indexed(&config.repository)?;
        
        // [+] this is the bugged code. 
        // Here by using `self` we ignore the `path_template` that could be defined in the `rustic.toml`
        let path_template = self
            .path_template
            .clone()
            .unwrap_or_else(|| "[{hostname}]/[{label}]/{time}".to_string());
            
        // [+] same here
        let time_template = self
            .time_template
            .clone()
            .unwrap_or_else(|| "%Y-%m-%d_%H-%M-%S".to_string());

       // ...

        // [+] same here
        let addr = self
            .address
            .clone()
            .unwrap_or_else(|| "localhost:8000".to_string())
            .to_socket_addrs()?
            .next()
            .ok_or_else(|| anyhow!("no address given"))?;

        // ....

To correct this, the #1241 PR came with changes similar to:

-        let path_template = self
+        let path_template = config
+            .webdav
             .path_template
             .clone()
             .unwrap_or_else(|| "[{hostname}]/[{label}]/{time}".to_string());

More complex problem

The forget command has CLI-only options that are only accessible from its inner_run &self reference.
But the same issue exists with this command, the other options are merged from the TOML and the ENV.

See

rustic/src/commands/forget.rs

Lines 100 to 111 in abf1835

	impl ForgetCmd {
	fn inner_run(&self) -> Result<()> {
	let config = RUSTIC_APP.config();
	let repo = open_repository(&config.repository)?;
	let group_by = config.forget.group_by.unwrap_or_default();
	let groups = if self.ids.is_empty() {
	repo.get_forget_snapshots(&config.forget.keep, group_by, |sn| {
	config.forget.filter.matches(sn)
	})?
	} else {

Why an issue, if this is fixed

This bug is purely a developers' problem. By the current design, the type system is not able to verify that we don't use self when we shouldn't. And sometimes we should (forget command for instance).

For this reason, we need to reflect on design updates and refactors that would solve this issue.
The design reflection will include (but not only) the role of the abscissa framework, our usage of it, and how it fits to our needs.

This issue will track the related design change.

Do not hesitate if you have any prompt, question, advice and all.

[aawsome]

Just as a remark: The env variables are handled by clap, too.