i have a big juicy diff ready to push and i cannot for the life of me remember the syntax/get it to work

i’m using this guide - https://pijul.org/manual/the_nest/contributing.html

what is the syntax?

pijul push danieleades@https://nest.pijul.com:pijul/pijul --to-channel 578? (this doesn’t work)

I use something like this

pijul push danieleades@ssh.pijul.com:pijul/pijul --to-channel :578

This is indeed the way to go. By the way, please make sure your patches are small enough to make them mergeable independently. I’m usually a bit wary of some of clippy’s suggestions (I do disagree with many of the design choices in clippy).

a bunch of unnecessary clones, redundant conversions, etc.

This is a small diff in the sense that each edit is small, and doesn’t involve massive refactoring. What is a “small change” in the context of pijul? what is the best practice?

Nice, your patch was indeed “small” and easy to review. I remember your earlier “quality-style patches” which were easy as well, I just reviewed a number of really hard patches of that kind in the last few months.

I generally agree with these changes, here are some comments and questions:

1. In the following, we really want to test whether the length is at least the expected length, not that it is strictly greater than the expected length minus 1. It makes no performance difference once compiled and makes it easier to read.

Replacement in libpijul/src/pristine/hash.rs at line 72 [16.644439]
B:BD[16.646354] → [16.646354:646434]

        if s.len() >= 1 + BLAKE3_BYTES && s[0] == HashAlgorithm::Blake3 as u8 {

[16.646354]
[16.646434]

        if s.len() > BLAKE3_BYTES && s[0] == HashAlgorithm::Blake3 as u8 {

2. Is there a specific reason for the following? I can’t see it (I’m not saying it’s bad, I just want to learn the good practices).

Replacement in libpijul/src/pristine/edge.rs at line 107 [16.648958]
B:BD[34.117745] → [34.117745:117782]

        let ref mut f = (self.0)[0];

[34.117745]
[34.117782]

        let f = &mut (self.0)[0];

3. I don’t want to touch chardetng at all, since it comes from the chardetng crate, and the plan there is to move back to the original implementation as soon as possible. The farther we are from that implementation, the harder it will be to move.

4. The following is a bit tricky to change, because it reads differently. I originally wanted to first filter on the direction (parent|block), then on the status (deleted). But now that I know what I’m doing a bit better, I agree it’s a bit strange, and a simpler way could actually be .contains(parent|block|deleted).

Replacement in libpijul/src/apply/vertex.rs at line 153 [28.90459]
B:BD[28.95699] → [28.95699:96060]

        if parent.flag().contains(EdgeFlags::PARENT | EdgeFlags::BLOCK) {
            if parent.flag().contains(EdgeFlags::DELETED) {
                let introduced_by = txn.get_external(&parent.introduced_by())?.unwrap().into();
                if !ch.knows(&introduced_by) {
                    ws.deleted_by.insert(parent.introduced_by());
                }

[28.95699]
[28.96060]

        if parent.flag().contains(EdgeFlags::PARENT | EdgeFlags::BLOCK)
            && parent.flag().contains(EdgeFlags::DELETED)
        {
            let introduced_by = txn.get_external(&parent.introduced_by())?.unwrap().into();
            if !ch.knows(&introduced_by) {
                ws.deleted_by.insert(parent.introduced_by());

5. Finally, since your patches touches parts of libpijul, which has a contributor license agreement, I want to ask you to add #IUH7IMWES3KQTHVWA5UNHAO7QWVCC5PQJ6VLK3RC3T4F2MS74P3AC as a dependency to acknowledge you’re aware of that. I’m not a lawyer, but that CLA rougly allows copyright holders to relicense libpijul under a different license, in addition to the current GPL2.

I forgot to say: thanks! this is excellent work.

In the following, we really want to test whether the length is at least the expected length, not that it is strictly greater than the expected length minus 1. It makes no performance difference once compiled and makes it easier to read. Replacement in libpijul/src/pristine/hash.rs at line 72 [16.644439] B:BD[16.646354] → [16.646354:646434]

    if s.len() >= 1 + BLAKE3_BYTES && s[0] == HashAlgorithm::Blake3 as u8 {

[16.646354] [16.646434]

    if s.len() > BLAKE3_BYTES && s[0] == HashAlgorithm::Blake3 as u8 {

correct, there’s no performance difference. Clippy makes the argument (and I’m inclined to agree) that the latter is slightly more readable.

there’s a more esoteric advantage too- one version generates a clippy warning, while the other does not. That might sound artificial, but there are a lot of advantages to eliminating warnings. If you can completely eliminate (or specifically whitelist) every single warning in the crate, then you can apply clippy in CI to prevent code quality regressions and enforce standards. That doesn’t mean accepting every lint that Clippy throws at you necessarily. For example we could add an #[allow(...)] for this specific line.

Is there a specific reason for the following? I can’t see it (I’m not saying it’s bad, I just want to learn the good practices). Replacement in libpijul/src/pristine/edge.rs at line 107 [16.648958] B:BD[34.117745] → [34.117745:117782]

    let ref mut f = (self.0)[0];

[34.117745] [34.117782]

    let f = &mut (self.0)[0];

see https://rust-lang.github.io/rust-clippy/master/#toplevel_ref_arg

I don’t want to touch chardetng at all, since it comes from the chardetng crate, and the plan there is to move back to the original implementation as soon as possible. The farther we are from that implementation, the harder it will be to move.

Can you give me some instructions for resetting files in a directory?

The following is a bit tricky to change, because it reads differently. I originally wanted to first filter on the direction (parent|block), then on the status (deleted). But now that I know what I’m doing a bit better, I agree it’s a bit strange, and a simpler way could actually be .contains(parent|block|deleted).

I’ll update

Finally, since your patches touches parts of libpijul, which has a contributor license agreement, I want to ask you to add #IUH7IMWES3KQTHVWA5UNHAO7QWVCC5PQJ6VLK3RC3T4F2MS74P3AC as a dependency to acknowledge you’re aware of that. I’m not a lawyer, but that CLA rougly allows copyright holders to relicense libpijul under a different license, in addition to the current GPL2.

sure, i think i can figure that out…