This lifts a limitation that has annoyed me ever since I started using
rebase regularly. Interestingly, the same annoying limitation exists in git;
I wouldn't dream of trying to fix it there.

The patch depends on two cleanup/refactor patches which are therefore
included.

3 patches for repository http://darcs.net/screened:

patch 926216a7c734b0e7fd8d0e6be324370ded4b86c4
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Mar  2 19:48:10 CET 2021
  * refactor rebase unsuspend

  The local function hijack that replaces the previous doAdd is more modular.
  It gets the selected suspended patches and those we want to keep as input
  and outputs a changed pair where the the patches to be unsuspended have been
  renamed. We then add these patches to the repo without updating the pending
  patch, so we can afterwards directly set pending to the resolution.

  For reasons that are mysterious to me, coercing just the added renames after
  commuting them no longer works; the mere act of matching the type of the
  rename patch introduces a fresh type variable for the underlying patch type.

patch ec5b78a3a97dcf6254b643faa5553b87e1e31f7a
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Mar  2 19:56:13 CET 2021
  * rebase unsuspend: code layout and a few renamings

patch 92530babc63f56638b73a94bc9c8279c96324d1e
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Mar  2 21:52:15 CET 2021
  * allow rebase unsuspend when there are unrecorded changes

  We still fail if the effect of the unsuspended patches (including the
  resolution for any residual fixups) conflicts with unrecorded.

Attachments

• patch-preview.txt
• refactor-rebase-unsuspend.dpatch

More precisely, what git doesn't allow is to start a rebase when there 
are unrecorded changes, while darcs allowed to start it (suspend) but 
not to finish it (unsuspend).

I managed to sort out the type errors with coercing
the added rename by using a type application.

I think this approach is a bit safer as it minimises
the extent of the coercing.

2 patches for repository darcs.net:/opt/darcs/screened:

patch fd1e91b64f68601b45c659ee74785aa6bd3a5f7d
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Sat Jun 24 21:54:19 BST 2023
  * limit the scope of the unsafeCoerceP in rebase unsuspend

  This is a followup to this patch:

    patch 926216a7c734b0e7fd8d0e6be324370ded4b86c4
      * refactor rebase suspend

patch fff17b8e8e4b15598d962a101f641aecd0b9016d
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Sat Jun 24 21:58:09 BST 2023
  * put back the Darcs.Witnesses.Eq import

Attachments

• limit-the-scope-of-the-unsafecoercep-in-rebase-unsuspend.dpatch
• patch-preview.txt

1 patch for repository darcs.net:/opt/darcs/screened:

patch 1c7a6598d0f1d87420387bb4facdb9fb4bbb8a0d
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Sat Jun 24 22:35:14 BST 2023
  * fixup comments

Attachments

• fixup-comments.dpatch
• patch-preview.txt

>  * refactor rebase unsuspend

I'm happy with this patch, particularly after I figured out how to put
the more limited coercion of an `EqCheck` back - see the followup I
sent to this patch.

I'm not sure about the name `hijack`" as I understood the concept to
relate to amending a patch that you aren't the author of, not the
general concept of changing the identity of a patch.

>   * rebase unsuspend: code layout and a few renamings

Fine

>   * allow rebase unsuspend when there are unrecorded changes

Looks good. Yay!

I'll leave this as review-in-progress for a bit so you can look at
my followup and consider whether to rename `hijack`, but I think it's
fine to accept as is.

(I sent the followups to this patch as it's not accepted yet, but
I could also move them to a separate one if preferred.)

Go ahead. I am not sure I fully understand the type logic here, but 
limiting the unsafeCoerceP to the commuted RebaseName is clearly an 
improvement. Is it possible to find more descriptive/suggestive 
names for the type variables you introduce here (currently wU, wV, 
wT2)?

(I assume you are planning to squash them to a single patch before 
screening.)

One thing I'd like to understand: where does the assumption that 
RebaseName "has no effect" come from? IOW, what justifies the

  rename :: RebaseName wU wU

Wouldn't it be more correct to say that the updatePatchHeader 
changes the context and that rename reverts that change?

Okay, I think I understand it now. Before rebase was added we had no 
need to track context changes arising from renaming patches, because 
whatever came after the recorded state (pending, unrevert) did not 
care about patch names. So we could get away with identifying the 
(start end end) contexts of a named patch with those of the prims 
that would become its content.

The clean solution is to change updatePatchHeader to return not only 
the new named patch, but also the RebaseName. I played with this a 
bit and found that in order to avoid coercions we then need to 
commute the Rename through all the prims that come after the 
recorded state (pending etc), which is quite a bit of extra code 
only to make witnesses match, so I'm not sure it's worth the effort.

> I played with this a bit and found that in order to avoid coercions
> we then need to commute the Rename through all the prims that come
> after the recorded state (pending etc)

Right - I think I've played with this before too. IMO the real issue
is that the witnesses for `AddName` etc should really live in
an orthogonal namespace to the witnesses for patch contents. The
no-op commutes are actually a smell that they should happen for
free.

But trying to encode that in the type system was a bit too heavyweight,
so we end up with the current slightly hacky compromise.

(I accidentally sent this to the wrong ticket first. Sorry for that.)

Not for screened yet, and please ignore the dependency here. I am sending it
to illustrate what I had in mind with my previous message. The change here
is not quite as radical as the one you talked about. Besides, I am not sure
we can regard contexts for patch name and patch content as entirely
orthogonal. So in this patch they are treated as things at the same level.
The remaining coercion, an unsafeCoerceU in the amend command, is needed
because commuting the rename through pending and working leaves us with a
different wU for the repository.

2 patches for repository http://darcs.net/screened:

patch 5089f3fee53578439a99d8ccf78714036bfbcba6
Author: Ben Franksen <ben.franksen@online.de>
Date:   Mon Jun 26 17:37:36 CEST 2023
  * revisit askAboutDepends after review of 0f939713

  This again changes AskAboutDepends to contain only an RL of the candidates
  to select from. The extra complication in the amend command to re-construct
  a suitable PatchSet is now gone: we have the required patch sequence readily
  available, since it is the same as what remains after selecting the patch to
  amend. Also added haddocks for askAboutDepends, including a TODO item about
  a rather annoying limitation.

patch f5f8d93cc49e104e93dbc900b6ce066c6418f7a1
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Jun 27 16:36:41 CEST 2023
  * WIP properly track context changes when we rename a patch

Attachments

• patch-preview.txt
• revisit-askaboutdepends-after-review-of-0f939713.dpatch

> (I assume you are planning to squash them to a single patch before 
> screening.)

They are actually already screened. Sorry for the slight mess - it
was a mix of the way I was reviewing and writing code as I was going.

I could clean them up if you feel strongly but obliterating from
mirrors is a bit of a nuisance.

> Is it possible to find more descriptive/suggestive names for
> the type variables you introduce here (currently wU, wV, wT2)?

> I'm not sure about the name `hijack`" as I understood the concept
> to relate to amending a patch that you aren't the author of, not
> the general concept of changing the identity of a patch.

>  * WIP properly track context changes when we rename a patch

Shall we accept this patch as it currently stands in screened,
i.e. your original bundle and my updates, and then follow up
on the other issues and ideas in separate patches?

The 'wU' type variables etc are as in the original code (and
probably also my fault from a long time ago!) and I reused them
so I could look at the aggregate diff from this set of patches
and have it be as small as possible:

1c7a6598d0f1d87420387bb4facdb9fb4bbb8a0d
fd1e91b64f68601b45c659ee74785aa6bd3a5f7d
926216a7c734b0e7fd8d0e6be324370ded4b86c4

I don't feel that strongly about your 3 patches, let's keep them as 
they are. Regarding my WIP patch (which I do have reservations about), 
I agree to re-send it to another ticket, with a link to what has been 
said here.

Regarding the type var names, thanks, I see now where they are coming 
from. It's not a big deal. Nevertheless, coercing the Rename to an 
effect-less operation is kind of a hack, so we should at least add a 
comment that explains why we do that.

I have followed up with patches to explain the effect-less rename
and to clarify the "hijack" terminology in patch2323

I've sent your patch to patch2324.