If you pull a patch into a repository, "rebase suspend" it, then pull the same patch in again 
and try to "rebase unsuspend" it, you get an internal error caused by an 'impossible' in 
Darcs.Patch.Rebase.Viewing.forceCommuteName.

The simplest fix might be to rewrite the name of the patch as soon as it is suspended.

The "simplest fix" doesn't quite work - it breaks other tests that are 
trying to track dependencies properly. Need to think a bit harder about 
this, but I've attached the attempt anyway for my own or others' future 
reference. Note it conflicts with some of the current patches in 
screened.

Attachments

• draft_-resolve-issue2445_-update-the-patch-header-on-suspend-to-avoid-collisions.dpatch

Is this solved by the new rebase implementation?

Ping

Unfortunately I still get an error with current reviewed:

This is a bug! Please report it at http://bugs.darcs.net or via email to bugs@darcs.net:
impossible case
CallStack (from HasCallStack):
  error, called at src\Darcs\Patch\Rebase\Change.hs:352:16 in darcs-2.16.1-inplace:Darcs.Patch.Rebase.Change

Sigh, I did have this fixed in my variant C branch. We must indeed 
rename patches immediately on suspend. Just think of what happens if 
we re-pull and then suspend the same patch again. We will now have 
the /same/ patch twice in the rebase state. This will wreak havoc 
for Rename fixups.

I think this is a blocker for 2.16.1. This /has/ to be fixed.

Oh I forgot. Renaming a patch on unsuspend requires a deep rename of 
all suspended patches that might refer to that name, i.e. 
conflictors. I think we should not go there.

A more practical approach is to allow (DelName n) to force-commute 
past a (Named n _ _) instead of calling error. The force-commuted 
DelName can now hit a (Named _ ds _) with (n `elem` ds), so we must 
also handle this case by not allowing the commute so that when we 
push an (AddName n) later, it can cancel the stuck DelName.

Regarding double suspends: This situation must be avoided. We need to 
check if this patch is already suspended, and if it is display a 
warning and then push just fixups.

Looking at my previous draft, it doesn't seem to push a rename between
the old and new names at suspend time. Thinking about it afresh now,
it (a) seems obvious that we must do that, and (b) I can't think of any
reason it wouldn't work.

I'll have a go at that today and also have a think about your DelName idea.

I don't think we absolutely need to avoid double suspends completely, but 
they will make a bit of a mess with fixups/conflicts. But I can imagine
cases where you've messed up the suspended patch so badly you want a
fresh copy, so I would like to avoid doing something special that could
sometimes get in the way.

[slightly reordered your replies]

> Looking at my previous draft, it doesn't seem to push a rename
> between the old and new names at suspend time. Thinking about it
> afresh now, it (a) seems obvious that we must do that

Yes, definitely.

> and (b) I can't think of any reason it wouldn't work.
> 
> I don't think we absolutely need to avoid double suspends
> completely,

You are right. A deep rename is only necessary if what follows the
renamed patch can have conflictors in them. That was only the case
with the other two rebase variants, whereas our chosen variant contains
only prims.

This makes the idea of renaming patches immediately on suspend feasible.
I very much prefer this solution over any ad-hoc changes to the commute
or force-commute rules.

> I'll [..] also have a think about your DelName idea.

I think that idea is obsolete if we rename on suspend.

> but 
> they will make a bit of a mess with fixups/conflicts. But I can imagine
> cases where you've messed up the suspended patch so badly you want a
> fresh copy, so I would like to avoid doing something special that could
> sometimes get in the way.

Not sure I understand this remark. Fresh copy?

BTW, you can always obliterate or inject to clean up the rebase state.

I have an extended version of tests/rebase-repull.sh that tests this
issue. I can send it as a draft if you are interested.

On 31/07/2020 10:08, Ben Franksen wrote:

>> but 
>> they will make a bit of a mess with fixups/conflicts. But I can imagine
>> cases where you've messed up the suspended patch so badly you want a
>> fresh copy, so I would like to avoid doing something special that could
>> sometimes get in the way.
> 
> Not sure I understand this remark. Fresh copy?

I just mean you might want to re-start working on the suspended patch
from scratch by pulling in a fresh copy of it.

> BTW, you can always obliterate or inject to clean up the rebase state.

True.

> I have an extended version of tests/rebase-repull.sh that tests this
> issue. I can send it as a draft if you are interested.

Ah, I just wrote a failing test in a new script. I guess it's pretty
straightforward either way.

I am spending a bit of time looking at the fix now but otherwise will
only have a chance this evening. If it seems simple to you feel free to
work on it yourself in the meantime.

>> Not sure I understand this remark. Fresh copy?
> 
> I just mean you might want to re-start working on the suspended
> patch from scratch by pulling in a fresh copy of it.

I see. Indeed.

> I am spending a bit of time looking at the fix now but otherwise
> will only have a chance this evening. If it seems simple to you feel
> free to work on it yourself in the meantime.

Did that, see patch2079.

The following patch sent by Ben Franksen <ben.franksen@online.de> updated issue issue2445 with
status=resolved;resolvedin=2.15.0 HEAD

* resolve issue2445: rename patches on suspend 
Ignore-this: dfbd006ec7c379e409948de77e525e714d5981f99789c291f90a9f03d233b698924631ab6f8ac963