This happens sometimes, when people move too fast and aren’t going through their usual CI: we should warn a user about a remote unrecord when pulling. This is very easy to do, all the information is available.

This is a very ugly sketch just to dredge up what information is actually needed implement this in the way that we discussed and then refine from there. The UX things that go beyond just the notification are auto-updating the local cache when there aren’t any actual conflicts, auto-updating when the remote unrecord was a patch that the user knew about but chose to ignore in an earlier pull, and allowing --update to force an update of the local cache. Most of the additional information needed is going to come from update_changelist, and there’s some more filtering that needs to be done in the pull command to figure out should actually be in to_download.

I’ll keep thinking about how to get the complexity of this down, but I kept thinking that the implementations of push/pull (and some other stuff in the pijul crate) are really screaming for some higher abstraction; there’s a lot of state and many small gears turning, but I kept feeling like there’s a more declarative way to describe the things I needed.

Thanks for the patch!

I am also not satisfied with the current state of push and pull. These grew out of control at the same time as I wrote the matching bits in pijul protocol and in the Nest.

I believe I good first step (which I’ll try to do today) would be to try and split the huge functions into much smaller one. This is probably the only place in Pijul currently where such long functions exist (pijul protocol is pretty bad too, but it’s clearly split into many independent cases).

Two minor comments:

• You don’t seem to be working with the latest version of the repository. Since a few things have changed in the semantics, I’m solving the conflicts, and I’ll push the resolution to this channel later.
• Because of this, it seems you’ve recorded hunks in files unrelated to your patch (mostly formatting stuff).

Update: See IVLLXQ below instead of FP2HJD.

FP2HJD is a much more pleasant take on this; it does a much better job of compartmentalizing the behavior and extracts some of the complexity out of Push::run and Pull::run. It adds:

1. Notification about remote unrecords for both push and pull
2. Notification if you’re going to push, and the remote has changes you don’t yet know about
3. a flag (--force-cache/-f) to force the cache to update. This will show only the initial notification about the unrecords and then force the cache to update.

The two things that probably need tweaking are: A. It might be better to add these notifications as comment blocks in the actual push/pull editor window since the user will probably want to be able to see the relevant change hashes while they’re editing the contents

B. I’ll have to look at how many elements are in the relevant lists as things get big, but it might be worth it to have both HashSet and Vec versions of some of the data in remote::mod (specifically new_ge_dichotomy) since I’m currently doing repeated iterations to look for certain elements.

@pmeunier

Prefer IVLLXQ for review. It fixes Issue A, uses more intuitive names, and fixes some doc links.

In IVLLXQ, there is a comment:

/// If the remote we're pulling from or pushing to is a LocalChannel,
/// (meaning it's just a different channel of the repo we're already in), then
/// `ours_ge_dichotomy`, `theirs_ge_dichotomy`, and `remote_unrecs` will be empty
/// since they have no meaning. If we're pulling from a LocalChannel,
/// there's no cache to have diverged from, and if we're pushing to a LocalChannel,
/// we're not going to suddenly be surprised by the presence of unknown changes.

This seems like a big assumption to me, but maybe I still don’t understand channels. Isn’t your IVLLXQ patch a channel in Nest, and the other patches are separate channels? If my repo is a mirror of a remote, and I pull down the entire thing and then manipulate the channels, wouldn’t it be similar to manipulating the channel remotely in terms of whether I know about changes? I’m saying that just because it’s local doesn’t mean that I know about it.

I wouldn’t want the user to get less information on local than remote.

@ammkrn: it seems you fixed B as well, I couldn’t find the repeated iterations you are talking about, and I saw a bunch of HashSets.

This is excellent work! As you already noted, the complexity of this seemingly simple feature is surprisingly high. Well done! The only surprising behaviour I saw when testing was that if I push a patch I crafted myself, the cache doesn’t yet know about it. The next time I push or pull, it’ll warn be about my own patch being unknown. I’ll send a patch to fix this.

I believe we can consider this discussion closed. Thanks a million!

@joyously: @ammkrn is right, actually, since his patch is about the cache we keep of remote channels. Local channels don’t have to be cached, since we have them readily available. It is true that one minor issue this could cause is the case where I unrecord something on my own repository in a different channel, pull again after forgetting that I had unrecorded.

However, this is not the issue this patch fixes: this patches is about the more common case where someone else unrecords a patch pushed to main by mistake. This doesn’t happen very often, but it does happen sometimes (practical example: @tankf33der pulls this repo quite often to run tests, and has been in the situation a few times where I had unrecorded a change, either my own, or one I hadn’t reviewed well enough).

@pmeunier

You’re right, I meant I fixed the vec/set thing, not the way the notifications were presented. I still think it’s a good idea to move them into the changelist file so users have access to the relevant change hashes while they’re looking at the list of stuff to potentially push/pull, but I’ll let you decide whether you think it’s appropriate.