origin: https://pijul.zulipchat.com/#narrow/stream/270693-general/topic/malleable.20identifiers.20followup/near/246741842

primary objective: get rid of publickey.json (merge it into secretkey.json, it shouldn’t be necessary in general because the public key is also saved into the identities folder, which is, as publickey.json, world readable; it may be necessary to implement a migration routine)

secondary objective: make it possible to use multiple secret keys / keypairs.

hm, made a mistake…

rust_pijul> error[E0425]: cannot find value `pk` in this scope
rust_pijul>    --> src/commands/key.rs:60:31
rust_pijul>     |
rust_pijul> 60  |                     dir.push(&pk.key);
rust_pijul>     |                               ^^ help: a tuple variant with a similar name exists: `Ok`
rust_pijul> error[E0425]: cannot find value `pk` in this scope
rust_pijul>    --> src/commands/key.rs:66:41
rust_pijul>     |
rust_pijul> 66  | ...                   public_key: pk,
rust_pijul>     |                                   ^^ help: a tuple variant with a similar name exists: `Ok`
rust_pijul> error[E0425]: cannot find function `load_key` in module `super`
rust_pijul>    --> src/commands/mod.rs:271:45
rust_pijul>     |
rust_pijul> 271 |             None => FinalAuthor::Key(super::load_key()?),
rust_pijul>     |                                             ^^^^^^^^ not found in `super`
rust_pijul> error[E0308]: mismatched types
rust_pijul>    --> src/commands/mod.rs:270:27
rust_pijul>     |
rust_pijul> 268 |     pub fn with_maybe_name(maybe_name: Option<String>) -> Result<Self, anyhow::Error> {
rust_pijul>     |                                                           --------------------------- expected `Result<FinalAuthor, anyhow::Error>` because of return type
rust_pijul> 269 |         match maybe_name {
rust_pijul> 270 |             Some(name) => FinalAuthor::Name(name),
rust_pijul>     |                           ^^^^^^^^^^^^^^^^^^^^^^^
rust_pijul>     |                           |
rust_pijul>     |                           expected enum `Result`, found enum `FinalAuthor`
rust_pijul>     |                           help: try using a variant of the expected enum: `commands::_::_serde::__private::Ok(FinalAuthor::Name(name))`
rust_pijul>     |
rust_pijul>     = note: expected enum `Result<FinalAuthor, anyhow::Error>`
rust_pijul>                found enum `FinalAuthor`
rust_pijul> error[E0282]: type annotations needed
rust_pijul>    --> src/commands/mod.rs:281:9
rust_pijul>     |
rust_pijul> 280 |         let mut ret = Default::default();
rust_pijul>     |             ------- consider giving `ret` a type
rust_pijul> 281 |         ret.insert(k.to_string(), v);
rust_pijul>     |         ^^^ cannot infer type
rust_pijul>     |
rust_pijul>     = note: type must be known at this point
rust_pijul> error: aborting due to 5 previous errors

Hi! I’d like to apply some of these changes, starting with #EUFISICKAQUFF5OSMSOG4SCJHUZKALBMAJGMACPOMYW325HGLNCAC.

I believe it would be better if you could split it into smaller changes, since there are definitely things I’ll apply without even thinking about it (fixing the tests), while others are trickier.

For example, I don’t want to touch chardetng, since this is vendored code (I have a PR open on the original repository to try and get my minor changes into the official version).

Also, Libpijul has a CLA, mostly in order to allow for a later relaxation of the license. In practice, since all changes commute in Pijul, this means that you have to make your first contribution explicitly depend on #IUH7IMWES3KQTHVWA5UNHAO7QWVCC5PQJ6VLK3RC3T4F2MS74P3AC (if you contribute this on your free time, or if your employer agrees with this CLA, which is a very standard one taken from a template from https://contributoragreements.org).

In general, massive Clippy-suggested patches are a bit scary for Libpijul, since they do change the semantics in subtle ways sometimes, and hence need particularly careful reviews. But I’m definitely willing to apply these too, especially if they don’t touch chardetng.

afaik I already made sure that the big patch commutes with the other ones. It already depends on the CLA. I will further truncate it to get rid of the chardet stuff and the one modification in front of that (the matchification) and merge it with the conflict resoltion, which will get rid of another modification.

I would like to know how I should split the KJPGBEQUZQNFIZ4LFDU6ZJQZIPH26PTVQ6QRFIFV75YE4LFN72AQC patch. Usually, I like to both keep stuff self-contained, which means does not break compilation; on the other hand, I like to bundle similar changes together, because I suppose that it might make review easier.

I guess I will split the pijul-macros stuff from the rest, because it is completely independent, and the replacement pattern is unique to proc macros. I guess because libpijul depends on pijul-macros, that patch should also depend on #IUH7IMWES3KQTHVWA5UNHAO7QWVCC5PQJ6VLK3RC3T4F2MS74P3AC.

Oo, I ran into some suboptimality… I run into some order conflicts, but I don’t really know how to solve them….

By the way, I don’t think I’ll apply the Clippy patches anyway, exactly because of that: they tend to cause a lot of conflicts, and aren’t always right. So if your conflicts are related to that, there is nothing to do.

nope, that #XNZWLCTDBRIERVUGJMGUWMIHAXXGV2ZLGS5AXMN2BSXWSFZDYEXAC runs into conflict with the recent refactoring of get_id… afaik I was able to fix it…

but the conflict presentation is suboptimal, it is completely unclear how to solve the conflict in local.rs based solely on the conflict markers… it is also unclear which patches conflict without looking at the output of pijul diff after removing the conflict markers.

btw. @pmeunier you should probably merge #MCAUAMZZBW4VURDFPLOIYTRRYXCXUWP5BD3TO3OEVK4WBD2HBLUAC because it starts to annoy people…

Yes, I agree this one is a priority.

(are you annoyed yourself?)

(are you annoyed yourself?)

not that much, because I can easily apply the patch locally, I just saw that two discussions were created about that exact issue…

By the way, the main reason merging stuff into Libpijul is slow is that my working copy is usually full of WIP where a single patch can take many days to get done, and I’m always a little bit afraid of stashing changes (and too optimistic about the time it will take the WIP stuff to stabilise).

In #RFHP… there’s an example of why I think Clippy should really be taken with a grain of salt:

I read the following as:

• Check whether the file exists. This already has a side effect formally, even though I don’t think ext4 changes anything with this call, maybe other filesystems do.
• If not, try to hardlink it (side effect as well).
• If this fails, copy it.

            if std::fs::metadata(&self.changes_dir).is_err() {
                if std::fs::hard_link(&local, &self.changes_dir).is_err() {
                    std::fs::copy(&local, &self.changes_dir)?;
                }

Whereas the following (for me), while semantically equivalent, reads as:

• Evaluate whether the result of doing the same two calls with side effects on the disk return errors, and trust that Rust and LLVM will never reorder the sides of &&, and that these two system calls will never be called concurrently on any platforms I can think of, which aren’t many since I only ever test my code on x86_64-linux-gnu (plus sometimes on wasm, and very rarely on x86_64-windows-msvc and arm32-linux-gnu). The reason I know this won’t be reordered or parallelised is because I remember the evaluation order of && is guaranteed in Rust, mainly because (1) I’ve had a similar problem with && a number of years ago in another language that doesn’t have the same guarantee, and (2) I know that on Unix, system calls block threads, and hence since these two functions ultimately trigger system calls, they cannot be called in parallel, and Rustc and LLVM know that.

• If both calls return an error, copy the file.

            if std::fs::metadata(&self.changes_dir).is_err()
                && std::fs::hard_link(&local, &self.changes_dir).is_err()
            {
                std::fs::copy(&local, &self.changes_dir)?;
            }

Conclusion: while it doesn’t actually take me longer to read one or the other, the second one fires a lot more circuits in my brain while reading it, and makes me doubt a lot more on the correctness of the code.

Alright, I’m closing this discussion because I don’t understand what to do. If your concerns are still unresolved, please open another discussion, with patches that are (1) on-topic and (2) small enough to be reviewable.

Sorry about that.

yes, I wasn’t able to continue here, and if I continue, the state of the rest of the project would probably have evolved so much, that it’s easier to just redo stuff.