There are a few god functions that would be handy to refactor.

Breaking these down into smaller functions allows you to describe what intermediate steps are trying to achieve, which should make the library less impenetrable for contributors

take this function for example

fn new_vertex<T: TxnT>(
    txn: &T,
    channel: &Channel<T>,
    pos: Position<ChangeId>,
) -> Option<AliveVertex> {
    let vertex = crate::pristine::find_block(txn, &channel, pos).unwrap();
    crate::pristine::iter_adjacent(
        txn,
        &channel,
        vertex,
        EdgeFlags::PARENT | EdgeFlags::BLOCK,
        EdgeFlags::PARENT | EdgeFlags::BLOCK | EdgeFlags::FOLDER,
    )
    .next()?;
    Some(AliveVertex {
        vertex,
        flags: if crate::pristine::iter_adjacent(
            txn,
            &channel,
            vertex,
            EdgeFlags::PARENT | EdgeFlags::DELETED | EdgeFlags::BLOCK,
            EdgeFlags::all(),
        )
        .any(|e| {
            e.flag
                .contains(EdgeFlags::PARENT | EdgeFlags::DELETED | EdgeFlags::BLOCK)
        }) {
            Flags::ZOMBIE
        } else {
            Flags::empty()
        },
        children: 0,
        n_children: 0,
        index: 0,
        lowlink: 0,
        scc: 0,
    })
}

it’s not clear what the intermediate steps are actually doing.

perhaps something along the lines of -

fn new_vertex<T: TxnT>(
    txn: &T,
    channel: &Channel<T>,
    pos: Position<ChangeId>,
) -> Option<AliveVertex> {
    let vertex = crate::pristine::find_block(txn, &channel, pos).unwrap();

    important_but_mysterious_step(txn, &channel, vertex)?;

    let flags = if is_zombie(txn, &channel, vertex) {
        Flags::ZOMBIE
    } else {
        Flags::empty()
    };

    Some(AliveVertex {
        vertex,
        flags,
        children: 0,
        n_children: 0,
        index: 0,
        lowlink: 0,
        scc: 0,
    })
}

fn important_but_mysterious_step<T: TxnT>(
    txn: &T,
    channel: &Channel<T>,
    key: Vertex<ChangeId>,
) -> Option<Edge> {
    crate::pristine::iter_adjacent(
        txn,
        channel,
        key,
        EdgeFlags::PARENT | EdgeFlags::BLOCK,
        EdgeFlags::PARENT | EdgeFlags::BLOCK | EdgeFlags::FOLDER,
    )
    .next()
}

fn is_zombie<T: TxnT>(txn: &T, channel: &Channel<T>, key: Vertex<ChangeId>) -> bool {
    crate::pristine::iter_adjacent(
        txn,
        channel,
        key,
        EdgeFlags::PARENT | EdgeFlags::DELETED | EdgeFlags::BLOCK,
        EdgeFlags::all(),
    )
    .any(|e| {
        e.flag
            .contains(EdgeFlags::PARENT | EdgeFlags::DELETED | EdgeFlags::BLOCK)
    })
}

(there are loads of examples like these)

i’m happy to push some changes, but I will need some help understanding (and naming) things along the way!

What a strange way to start contributing to a project. I don’t see the titular “massive if statement” anywhere, and the function seems fine (sure, a doc comment might be nice).

Why not try implementing some change you’re interested in, asking for explanation if you run into a concrete issue while doing that, and refactoring the code you touch to your liking?

hi robx, what a strange way to enter a discussion?

I’ve not been counting the code contributions i’ve made so far, but this is certainly not the first.

the missing ‘massive if statement’ no longer exists, because it has been refactored more or less exactly inline with the suggestion i made above. But there are other examples.

in the general case, doc comments are a code ‘smell’ (of course there are cases where they are useful). If a function is sufficiently complex that the algorithm it encodes requires additional documentation then that this a good sign it should be refactored.

In a brand new project with large community impact such as this one, on-boarding of would-be developers is perhaps even more critical than in other projects. To that end, refactors such as this are not only aesthetic choices, but also contribute to the long-term maintainability of a project. The more readable something is, the more eyes you can get on it. There’s an awful lot of literature out there on this exact topic. do let me know if you need a reading list.

I hope I don’t have to explain the irony to you of taking time out of your day to write a completely unconstructive comment on how you found a comment unconstructive. Let’s try and keep it positive on here, hey?

libpijul::src::find_alive::find_alive_up is another good example of a function with a wild cognitive complexity. the ‘boolean’ in the if statement alone is a 13 line statement.

Again, happy to contribute code here, but will need some input understanding/naming the intermediate steps.

It’s ok to blame me for the frictions in this discussion. The frictions are actually in the code: all these things were explained before using literate programming comments. Unfortunately, this proved very impractical when refactoring, as the emacs mode was completely lost, and even line numbers from cargo build in the terminal where hard to match with source files. So, I restarted everything from the generated source, with the consequence is that this isn’t very easy to understand now (to say the least).

Also, I’m trying to clarify some bits regarding licenses. It seems that some people would be willing to buy a license for libpijul, in order to build stuff on top of it, and:

• I want keep it open source forever.
• I also want to move as fast as possible to get all the tooling around it in order, but that needs money. So, I’m thinking that a dual-licensing scheme could fund the community, but takes a little bit of administration.

@danieleades: before starting this huge undertaking, I believe we would need to talk a little bit. I don’t know what communication channel is best for this, but I’ve setup Matrix in the past. Others have suggested Zulip, but there is no NixOS module yet.

Since the code has evolved quite a bit since this discussion started, I’m closing it. Feel free to reopen if the issue is still there.