darcs

Issue 2445 internal error if suspended patch is pulled into repository again

Title internal error if suspended patch is pulled into repository again
Priority critical Status resolved
Milestone 2.15.0 Resolved in 2.15.0
Superseder Nosy List ganesh
Assigned To ganesh
Topics

Created on 2015-03-16.18:48:53 by ganesh, last changed 2020-07-31.19:41:02 by noreply.

Files
File name Uploaded Type Edit Remove
draft_-resolve-issue2445_-update-the-patch-header-on-suspend-to-avoid-collisions.dpatch ganesh, 2015-09-18.13:28:05 application/octet-stream
Messages
msg18310 (view) Author: ganesh Date: 2015-03-16.18:48:51
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.
msg18730 (view) Author: ganesh Date: 2015-09-18.13:28:05
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
msg22094 (view) Author: bf Date: 2020-06-21.10:13:18
Is this solved by the new rebase implementation?
msg22304 (view) Author: bf Date: 2020-07-30.15:36:07
Ping
msg22315 (view) Author: ganesh Date: 2020-07-30.19:05:30
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
msg22318 (view) Author: bf Date: 2020-07-30.23:05:14
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.
msg22319 (view) Author: bf Date: 2020-07-31.00:15:01
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.
msg22320 (view) Author: ganesh Date: 2020-07-31.08:20:47
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.
msg22323 (view) Author: bf Date: 2020-07-31.09:08:46
[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.
msg22324 (view) Author: ganesh Date: 2020-07-31.09:17:57
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.
msg22326 (view) Author: bf Date: 2020-07-31.11:25:35
>> 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.
msg22331 (view) Author: noreply Date: 2020-07-31.19:40:58
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
History
Date User Action Args
2015-03-16 18:48:53ganeshcreate
2015-03-20 22:27:40ghsetmilestone: 2.10.0
2015-04-18 17:39:09ghsetmilestone: 2.10.0 -> 2.12.0
2015-09-18 13:28:06ganeshsetfiles: + draft_-resolve-issue2445_-update-the-patch-header-on-suspend-to-avoid-collisions.dpatch
messages: + msg18730
2020-06-21 10:13:21bfsetmessages: + msg22094
milestone: 2.12.0 -> 2.15.0
2020-07-30 15:36:09bfsetmessages: + msg22304
2020-07-30 19:05:33ganeshsetstatus: needs-implementation -> needs-diagnosis/design
messages: + msg22315
2020-07-30 23:05:17bfsetpriority: critical
messages: + msg22318
2020-07-31 00:15:04bfsetmessages: + msg22319
2020-07-31 08:20:51ganeshsetmessages: + msg22320
2020-07-31 09:08:49bfsetmessages: + msg22323
2020-07-31 09:18:00ganeshsetmessages: + msg22324
2020-07-31 11:25:37bfsetmessages: + msg22326
2020-07-31 19:41:02noreplysetstatus: needs-diagnosis/design -> resolved
messages: + msg22331
resolvedin: 2.15.0