[darcs-devel] [patch1911] WIP: use Prim patches in rebase toedit

Wed Oct 2 10:08:53 UTC 2019

Ben Franksen <ben.franksen at online.de> added the comment:

>>> So we'd need a special commute rule for pairs like a;a^?
>>
>> Yes, I think so (and also similarly for FLs and RLs). Note that this is
>> a theoretical consideration and not meant to be implemented.
> 
> OK - I guess that encapsulates the theoretical differences between the
> two approaches (fork off to the side versus linear sequence).

I think this may be related to the question: why is it sound to cancel
inverses in sequences of fixups or in the context of a patch we forked
off to the side? I have tried on and off to find a simple theoretical
explanation for this but failed so far.

>>> and the "fixup" prims would end up in some new dummy Named patch.
>>
>> Where do you store this patch? You cannot possibly plan to mix it with
>> normal patches... =:-/
> 
> I thought that was exactly what you were suggesting here:
> 
> https://lists.osuosl.org/pipermail/darcs-devel/2019-July/019823.html
What I had in mind was always a /proper/ conflicted patch, not something
like SimpleConflict with possibly unsound commute and merge behavior! I
thought I had made that clear, but apparently I did not.

Either we can create a proper conflicted patch resulting from a regular
merge (or a sound and well-tested version of force-commute) or we have
to store the conflict somewhere else. IMO mixing SimpleConflict with
regular patches is completely out of the question. With RepoPatchV3 we
/finally/ have a sound implementation of conflictors and I am not
willing to jeopardize that just to get a better rebase unsuspend.
Besides, wouldn't that require us to define how a SimpleConflict
commutes and merges with /all three/ RepoPatch types?

However, there may be a solution that avoids this mixing. The idea is
similar to how patch queues work in Mercurial. When we unsuspend a
patch, it will not immediately be converted to a regular patch. Instead
we have a third sequence of patches in between the regular recorded
patches and the suspended patches. These patches are applied to the
working tree and they have their own version of a pristine tree, which
allows us to directly edit them using amend etc. But we maintain a
strict separation from the regular patches. When we are done with
conflict resolution etc, we can convert them to regular patches.

This adds a significant amount of complication to the UI, though. And to
support it requires lots of changes to the Repository layer.

>>>> I think the correct solution here would be to push a merge between
>>>> the old and new versions of the amended patch.
>>>
>>> The special treatment that amend gets in rebase is important in
>>> practice - at least, I wrote it after realising that amending
>>> otherwise doesn't work well. You really need to use the actual diff
>>> being amended in. The RebaseName operation rename is also important
>>> for updating explicit deps.
>>
>> I agree with all that, but my intuition tells me that we can achieve the
>> same with merging. I must admit that I haven't thought this through in
>> detail, yet. Anyway, here is a rough sketch.
>>
>> Suppose we have suspended patches R that start after A. We amend A to B
>> and have to adapt the rebase state. So we merge B\/(A;R) and get
>> A';R'/\_ of which the lhs A';R' is the resulting new rebase state. The
>> A' retains all the information we need: we could recover the difference
>> between A and B as prim fixups from it (including rename fixups) if we
> 
> Why would A' contain any information about the name of B? I guess that
> if as is likely A and B conflicted, then A' might have some V3 prims
> from B that could be used to recover the name of B, but that is quite
> roundabout.

You are right. I wasn't thinking clearly here. RebaseName gives us the
ability to express that the user views A and B as "the same" patch even
though their identities (names) are different. This is a new feature
that cannot be expressed with the existing Named and RepoPatch machinery.

> BTW a good example where you really care about what actually happened in
> an amend comes from things like move and replace patches. There's an
> example in the 'rebase-move.sh' test:
> 
> Add a file A and record a series of patches that touch A, then suspend
> all but the one that originally created it. Now rename it to B and
> amend-record that original patch. The rename gets coalesced with the add
> to get "add B" instead of "add A". If you just diff the old and new
> patches, you have "del A; add B" as a fixup which makes a horrible mess
> of conflicts. If you remember what the user did, and push it as a fixup
> "move A B", as amend does, it commutes beautifully and the rebase state
> becomes a sequence of edits to B without any conflicts.

Right again. I knew all that at some point but forgot the details. Sorry
for needing you to remind me ;-)

So amend needs to be treated specially. We could create a new named
patch that expresses the users intention of how the amended patch is to
be modified (or rather, the inverse of that). And then we push that as a
named fixup into rebase. And we have to somehow capture that a patch and
its amended version are regarded as the same thing, so we need to attach
rename fixups to this thing.

There is one more potential problem with the idea. Prim fixups can be
commuted independently past suspended patches and then dropped or
coalesced with later fixups. But if we keep them together in a Named
patch, we loose that ability, at least we cannot do it eagerly. But
perhaps we don't have to. It may be possible we can be address this by a
suitable implementation of force-commute for Named patches: it could
first try to commute the components of a Named patch one by one using
the regular commute, and only then "forcibly" deal with what remains.
(Whatever that means, I am still pretty unclear about that.)

>> Okay, so now we unsuspend R (or some part of it) and let us suppose this
>> depends on the A'. So we use our primitive force-commute on A':>R to
>> get... what? I don't know yet what the result of a force-commute should
>> be. Need to think about that. This is the interesting part where all the
>> rebase magic happens...
> 
> Right, a generalised force-commute is fundamental and would be of more
> general interest.

I guess the difficulty in defining it lies in the fact that it must be
able to create new identities. None of the code we have in e.g.
RepoPatchV3 does that, yet.

>> One thing that is clear to me now is that we would need a special
>> implementation of force-commute for Named patches that handles the
>> explicit dependencies: if we force-commute P:>R, where R has an explicit
>> dependency on R, we need to represent the loss of that dependency; this
>> is what we currently do with the WDDNamed wrapper and I guess we'd need
>> something quite similar here.
> 
> Yep. This kind of thing is why, at least theoretically, I'd like to
> treat names and dependencies as proper patches of their own, so that we
> can make this kind of thing just happen automatically by reusing the
> infrastructure for prims. But keeping track of it adds a lot of overhead
> to the code that's hard to justify.

I agree, it would ne nice to have this logic factored out. Perhaps some
future Haskell extension will make that more practical.

__________________________________
Darcs bug tracker <bugs at darcs.net>
<http://bugs.darcs.net/patch1911>
__________________________________
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pEpkey.asc
Type: application/pgp-keys
Size: 4211 bytes
Desc: not available
URL: <http://lists.osuosl.org/pipermail/darcs-devel/attachments/20191002/d1210eee/attachment-0001.key>