[darcs-devel] unsuspend-reify [was: [patch1824] v3: basic integration]

[Ben Franksen]

Am 31.08.19 um 22:58 schrieb Ganesh Sittampalam:
> On 16/07/2019 09:28, Ben Franksen wrote:
>> But we are rebasing, so we are free to amend patches. All we need is p'
>> and we amend it by replacing its content with its effect, so it becomes
>> unconflicted and doesn't refer to anything anymore; let's calls that q.
>> The freak (f^')^ is thrown away. Instead we do a second merge (q:\/:p) =
> 
> This merge doesn't make sense to me.
> 
> We force commuted to get p':
> 
>    p
>   --->
>  |
> f|
>  |
>   --->
>    p'
> 
> and q is logically equivalent to p' (at least it lives in the same
> starting context). So merge (q:\/:p) seems like we are merging two
> things from separate contexts. Did you mean merge (q:\/:f) ?

I am not sure I meant it but I agree that only f would be in a position
to be merged with q. But merging them would not give us what we want.
Suppose we merge (q:\/:f) and get (f':/\:_) and take q:>f' as our
result. The problem is that p' was conflicted. If we just replace it
with its effect, then the original changes in p that are in conflict
will be lost.

> On a related note, we currently use full patches rather than primitive
> patches as the ToEdit items in a rebase. I'm not sure this actually
> makes sense. Inside a rebase, we can better express the notion of
> conflicts with fixups. It also means we need fromAnonymousPrim when
> commuting things around inside a rebase (in D.P.R.Fixup).

Yeah, that part is somewhat fishy.

> I've had a play with changing ToEdit to be Named (PrimOf p) instead of
> Named p, and it seems to basically work.

So you replace patches to be edited with their effect right away instead
of only when we unsuspend. Correct?

> The fromAnonymousPrim calls end
> up only at the "boundary" of the rebase world, where we are either
> extracting a conflicted patch (extractRebaseSelect) or looking at
> changes (D.P.R.Viewing.changeAsMerge, used by
> RebaseChange.conflictedEffect).

I am not sure why you need fromAnonymousPrim at all. At the boundary
(out of rebase) we can build a Named patch in the normal way.

> In both cases the conflicts are straightforward ones which would always
> originate from Prim patches on both sides and are ephemeral.

This is is quite cryptic to me. Care to explain?

> I wonder
> about using a specific new patch type to represent these types of
> conflicts, 

What do you mean "these types of conflicts"? I don't get it. How are
they different from "normal" conflicts?

> that would be much simpler than any of V1/V2/V3. We could
> define the force commute by hand then. Even if we decide to turn them
> into a real Named later, we could avoid temporarily creating invalid
> patches.

None of this makes much sense to me until I understand the things about
conflicts above.

> (BTW I think the unsuspended patch is currently transformed into its
> effect in the updatePatchHeader code in D.UI.PatchHeader, which is a bit
> obscure.)

Interesting. I wondered before why I was unable to find that code...

>> The point of all this is that when we unsuspend-reify in this way we get
>> first the patch q (i.e. the rebased version of our old, formerly
>> suspended p) and then an artificial but otherwise valid conflicted patch
>> p''. So the conflict markup can be restored at any time using
>> mark-conflicts. When we are done with manual resolution we can unrecord
>> it and then amend everything into p. But we don't necessarily have to;
>> we might opt to keep it in our repo as is, or only amend it, and not the
>> q. It makes sense then to improve the PatchInfo we generate for p'', so
>> it has the correct author (whoever rebases the patch) and date of
>> creation; we could even let the user edit the description.
> 
> I think leaving something concrete in the repository to represent a
> conflict would be a really good step forward.

We already have that and we call them conflictors. Re-inventing
something different for a special case is not a good step forward.

> As you say, conflict
> markup is lossy if we don't keep the source of the conflict around.

Yes.

> As
> discussed in patch1871 we have the same problem with conflicts with
> unrecorded.

No we don't. Representing conflicts with unrecorded is not something I
have ever heaŕd any user ask for. Trying to do that is a completely
unnecessary complication.

> Should this be a Named patch? I'm not sure. Inventing new patches for
> the user is a bit weird, and as you say in patch1871 would be quite
> weird for their unrecorded changes. I don't see it as out of the
> question though. The Named patch could just contain the things that were
> actually in conflict, so it'd have a fairly localised effect and of
> course we'd have to explain it to the user.

I don't like the direction this takes. It reminds me of the idea to mix
the rebase patch with normal patches. SOunds like now you want to mix
somehow-conflicted-but-not-quite patches in there. With vague notions of
their effect being "fairly localised". But perhaps I misunderstood.

You are right that auto-generated named patches are not a good solution.
But then the whole idea doesn't work out anyway (see above).

> From the patch1871 discussion:
> 
>>> And perhaps we should actually consider allowing pending to have
>>> conflicts?
> 
>> What about unrecorded changes that are not in the pending patch? Also,
>> how do you suppose we could ever resolve a conflict in pending, given
>> that pending must always stay on top?
> 
> Not really sure, just thinking out loud :-) Perhaps it wouldn't be
> pending but something else. But the common thread between pending as it
> is now, conflicts with working, and conflicts with fixups, is that we
> want some kind of structure to represent some changes that the user
> hasn't yet asked to put in a real patch.

Perhaps all we need to do is to save the result of resolveConflicts i.e.
the unravelled, not-yet-mangled conflicts, somewhere? I mean on
unsuspend. BTW, what conflicts with fixups? We don't have conflicts with
fixups, we depend on them. We only get conflicts when we force-commute
them. Can you explain how that actually works in our current rebase?

Cheers
Ben