Code Review Workflow

Hey everyone! :slight_smile:
I’ve recently discovered about this project and I was really impressed with everything. I’ve tried reading everything I could find online about Pijul but I was left with a question about a correct workflow especially the code review process.

As I understand it, the way to submit changes for merging into the main channel is through a nest “discussion” which is basically a set of patches (the diff between my local main and the remote main or a local channel and a remote channel). I’ve looked at some discussions on nest and at least to me it seems like there is no good way to asses if these patches are good or even if they compile since they are totally devoid of context.
I understand that Pijul is not snapshot based like git is, and that in the patch itself we shouldn’t encode the surrounding context. Because patches only need to depend on patches that are required for their change to have any meaning (like deleting a line in a paragraph that was added in another change). And therefore we can’t know how the file “looks like” outside the patch. However I believe this is an issue when trying to assess whether a certain patch should enter the main channel as the channel stands at this time.
In my opinion, one possible solution for this is supplying the discussion with a “target channel” to see how would the channel “look like” after applying the patches in the discussion. That way someone could look at the patch in context without changing the internal implementation. Because after all the person who made the patch made it in the context of how the main channel looked like at the time of writing so I think the code review should also be done with context.

I maybe missing something very fundamental about Pijul and I’m still thinking like a Git user but to me at least this change makes sense, but I would love to learn more and see what I’m missing

Thank you for your time :slight_smile:

3 Likes

I think this is very similar to wanting to be able to see or export a file that corresponds to any state in the history of the repo. I don’t know if it can be done, but it should be. Thinking through how, wouldn’t the changes need to be applied in order, until that state is reached? Does the commutative nature of the changes make this a problem?

The Nest could use several enhancements, but pijul itself should be providing a way to “see” the changes in context before affecting the repository, for review, compiling, testing. Is using a separate channel for that sort of thing what channels are for? If so, it would be great if that were built-in, as a command.

iiuc tags were introduced to address this, and are the way you’d implement a “not rocket science” style CI

I don’t think tags are the solution unless they are auto-generated.
Anyone could submit a change for review at any time, not just against a tagged state.

I agree, tags can’t solve this problem because we would like to look at the patch in context to how it would look like in the channel after being applied. This could also tell us if there are any new conflicts that happened while the patch waited to be applied to the channel.

1 Like

Patches are not “diff[s] between my local main and the remote main”. They are independent recordings of mutations of your sources. They are not ordered but they are connected with each other in a dependency graph.
The (local or remote) state that you observe, is the result of applying all these recorded mutations, starting from an empty directory.
Thus, a single patch must be devoid of context, because it doesn’t live in a natural context. The context appears the moment you apply (at least) two patches. So, to evaluate the quality of a patch, you apply it to the relevant set of patches (e.g. your local main). This creates the necessary context to evaluate the patch in.
It may be beneficial to improve the representation on the Nest to allow you to pick a channel and see a patch in context of that channel. Yet, patches in pijul (and discussions on the Nest) are much more powerful and limiting the patch view to a channel would not do their power justice.
One example: A discussion allows you to push several variants of your solution simultaneously. A reviewer could pick and try each variant and comment on them.

As it is right now, the Nest is not in a particularly good shape to review code online. It is more of a host for the patches and discussions. The review itself should be done in a local repository.

From my experience with pijul up to now, I would love to have a pseudo-Shell in discussions on the Nest, that allows me to switch to a channel an (un-)record the patches of the discussion. Such that a file-tree view in that discussion would update and provide the necessary context. That would be awesome.

N.B.

I think this is a bit of an unfair statement. The regular reviewer will not be able to tell you if some change breaks the code by looking at the diff, with or without context.