pijul_org / pijul

#22 pijul push: conflicted patches should be rejected

Opened by lthms, on March 28, 2017
Open
lthms commented on March 28, 2017

While I was experimenting with unrecord, I ended up pushing twice the same patch, except the second had several new files. After that, the nest was displaying the conflicts.

I think a sane rule is to prevent such situation and a fair solution is to reject a patch that conflict with an already present patch in the targeted branc.

lthms commented on March 28, 2017

There are some patches in the pijul recent history that make me think I am right.

pmeunier commented on March 29, 2017

Hi! There has indeed been a problem in Pijul's recent history, but it seems unrelated to what you suggested. I'm still unsure, but it seems rather related to unrecording the pending patch just after a record.

What do you think of the new "unrecord" feature on the Nest? Does it solve your issue?

lthms commented on March 29, 2017

I had a more in depth look at the related patches and I think you are right when you say it is not the same problem.

The “unrecord” feature of the Nest helps, as one can unrecord faulting patches. Preventing to push conflicted patches (or, better, mark them as “pending”) would prevent to have a repo in a state where it displays something like:

<<<<<<<
Something
=======
Something'
>>>>>>>

At least, that is my understanding of how pijul works.

What do you think?

pmeunier commented on March 29, 2017

I don't know. Your suggestion is indeed how darcs handles conflicts. Darcs and git users are usually afraid of conflicts, because:

  • git doesn't really know what a conflict is (in the sense that it has no correct notion of what it is).
  • darcs only what it isn't: two patches that don't commute.

In pijul, conflicts are not a special case, they are represented in the exact same way as any other patch application. In the current version of pijul and of the nest, conflicts are shown with these ugly signs, but we could imagine fancier representations to help users decide which side of the conflict they want to keep.

lthms commented on March 29, 2017

I see. That makes sense. I think that marking a patch that results in a conflict as “pending” is a good idea.

Two questions:

  1. when applying a patch results in a conflict, does pijul warn the user?
  2. is it possible to mark a conflicting patch as pending?

My point of view, at this point, is that its a sane property to expect a nest repo to be conflict free. That way, when someone pull a repository from the nest, she knows her version of the file is something that has been intended by at least of dev.

pmeunier commented on March 30, 2017

Well, the problem is that whenever the repository owner applies a pending conflicting patch, they should apply the resolution at the same time, or there will always be a conflict.

This could actually also be forbidden, and the test for conflicts should be on groups of patches, not on patches themselves.

Testing for conflicts is relatively expensive, by the way (linear in the size of the current repository). So, we need to see how that fits in the bigger picture of handling giant repositories with Pijul, something no one else can do well in a distributed setting (I'm aware of the existence of git and mercurial).

FlorentBecker commented on March 31, 2017

There are three possible semantics for push:

  • it shares an existing, well known state with the world. That's what git does by enforcing that pushes are "fast forward";
  • it shares a maybe-unknown but conflict-free states. That's what darcs does (by default) as it "refuses to apply a patch that would lead to a conflict";
  • it gives you the merge of your branch with the distant branch, whatever that maybe, which is the current behaviour of pijul.

Note that this is only a summary of the behaviour of the respective console tools, not the buttons on their web versions: gitlab / darcsden / pijul nest.

The three semantics make sense at various time. The property that you can restrict yourself to only offer versions that have already been seen somewhere else ("fast-forward") is probably the right one in practice for publishing. It's what darcs developpers end up doing anyway when pushing into public repos. pijul's current behaviour, while theoretically sound is not very ergonomic as soon as the distant repository is shared (not even necessarily public).

What I propose is to have the following commands:

  • pijul merge works between remote as well as local branches in either direction. pijul merge --from A --into B moves branch B to incorporate the changes in A. It can --allow-conflicts or --abort-conflicts. The default should probably be --allow-conflicts at all times, but a case can be made for --abort-conflicts by default if B is remote.
  • pijul fetch / pijul send (maybe even pijul sync A B, where A and B can be remote or local) moves branch B in fast-forward to the current state of branch 'A'. If B does not exist, creates it. This guarantees that B is exactly in the state of A afterwards. Maybe an explicit +B should be required when B does not exist yet.

I think this covers most use cases, with pijul merge --from remote --into local and pijul sync local remote being in the end the most common commands.

lthms commented on March 31, 2017

That sounds like a good api for pijul indeed! Yet, the capability to push one (or more) patch among many (which current pijul push gives) is convenient. Maybe the --abort-conflict option should be added to push too. Or do you suggest to remove pijul push?)

FlorentBecker commented on March 31, 2017

If we give both merge and sync (arguably, transfer is a better name) an interactive version which allows you to select a subset of the patches in branch A, then push and pull become redundant. Whether they should be kept as aliases or not is another question. I certainly like that merge and transfer make the post-condition overt rather than the direction of the transfer. A missing --from or --to should default to the local current branch. Thus, pijul pull X becomes pijul merge --from X, and the pijul push you want becomes pijul transfer --to Y. pijul pull without argument and pijul push without argument do not exist yet, but pijul merge --from and pijul transfer --to sound like reasonable ways to rely on a defaul value. Thoughts?

FlorentBecker commented on March 31, 2017

Then on the nest, merge and transfer get different wordings: either "merge into " or "update to this". merge should at the very least ask for confirmation if there are conflicts.

FlorentBecker commented on March 31, 2017

of course, markdown ate my wordings: "merge into <branchname>" or "update <branchname> to this".

pmeunier commented on February 27, 2019

Ok, we have a preliminary implementation of this: the implementation of push in master refuses to push in a non-fast-forward way (there is a --force switch to override this).

I believe this is pretty much what we want, since potential new users are used to the push/pull vocabulary. What do yous think?