According to the help output, ‘pijul unrecord’ without any arguments is supposed to provide a list of revertable changes:

• Without listing any <change-id>s. You will be presented with a list of changes to choose from. The length of the list is determined by the unrecord_changes setting in your global config or the --show-changes option, with the latter taking precedence.

However, running it errors instead:

% pijul unrecord       
Error: No such file or directory (os error 2)

Using pijul 1.0.0-beta.7. (Was going to test beta 8, but it looks like rustup is broken on my Mac so I have to use the nixpkgs package. Going to test once I fix or am back at a Linux machine.)

Aha, this happens when there is no configuration file. The config loader should probably just treat a missing file as an empty file. Writing a fix

I would also suggest completely removing the config timestamp returned from the config loader since it is currently unused and I can’t imagine what it could be used for.

^ This also makes an empty config file parse correctly (instead of needing there to be an empty [author] section)

☝️I’m not sure which is a better or more idiomatic solution (I’ll leave that to those more fluent in Rust), but the Author block issue is also addressed in #864. I made it an Option like the other global config fields.

#[serde(default)] is better if it makes sense IMO since then you don’t have to deal with the option type, and it reduces the amount of cases to handle. It’s basically automatic Option::unwrap_or_default when deserializing.

Sometimes you don’t want that (when a missing value has a special meaning, or you want to explicitly know that it was missing). Here I feel like it should behave the same as an empty author section since that is intuitive in a config file, but I see you’re actually handling it differently in your change. Does yours actually behave differently with no [author] vs empty [author]?

I believe I have just handled it as an Option in the same way as the other config settings.

If it’s missing it falls back to the same defaults as if there is no config. One thing I did notice when making it an Option is that in one of the files (can’t remember which one) that default value is not just Author::default. There were some other values applied to it. In that regard it seems that just setting it to default would be incorrect in that case.

In general, unless there is a good reason in a particular case not to, It seems like a good idea for all the settings to be handled in the same way, either as Option or with the magic unwrap to default comment you mentioned.

I don’t really have a strong opinion on which way is better (the comment above re the default value not withstanding) other than consistency seems like a good approach.

If it’s possible to (I don’t know) perhaps it’s a good solution to coalesce the default value to all the optional settings immediately, rather than deal with the optionals elsewhere?

I didn’t investigate too deeply to see if different defaults are being used for the other settings. But, as mentioned above, I did find that Author does use different default values in the two places it was used.

Perhaps that itself is a bug?

Yeah I would absolutely want to have a method on the Config struct (if not as part of the struct fields itself) to get the canonical value of Author with all values filled in, so you don’t have to fill it elsewhere and potentially introduce inconsistent behavior. Same for the other optional values in Config if it’s applicable for them. Let’s see what @pmeunier says.

+1

In #832 I wanted to address this problem too. However there I attempt to do a more deep change around the author configuration field. I still think this should be reworked more tailored to make the author/identity one thing and not have it somewhat duplicated in config.toml and identity.toml. Note that #832 has stalled quit a bit so probably it would require some fixing.

That said short term one of #861 or #864 seem a reasonable fast fix.

@hardy7cc: I’m sorry I didn’t apply your changes, some even older patches had been suggested and I haven’t been fast enough.

Hopefully I’ll have more time for Pijul in the coming months, so would you mind submitting an improvement on top of this discussion? You can start by pijul pull --from-channel :861, then record and pijul push --to-channel :861. This will avoid too many duplicates.

@pmeunier I had another look at this and think the changes provided by @dblsaiko with this discussion are great. It would be nice to get these into main and close this discussion.

Ok, thanks @hardy7cc for the review, this is super useful as well.