Some general cleanups and documentation for the rebase code

7 patches for repository darcs-unstable@darcs.net:screened:

Sat Nov  3 11:43:26 GMT 2012  Ganesh Sittampalam <ganesh@earth.li>
  * break out Darcs.Patch.Rebase.Select module

Mon Nov  5 07:01:15 GMT 2012  Ganesh Sittampalam <ganesh@earth.li>
  * split Darcs.Patch.Rebase.Name out from Darcs.Patch.Rebase.Fixup

Mon Nov  5 07:10:25 GMT 2012  Ganesh Sittampalam <ganesh@earth.li>
  * move RebaseName instances to Rebase.Name module

Mon Nov  5 19:26:44 GMT 2012  Ganesh Sittampalam <ganesh@earth.li>
  * move RebaseName commute functions to Rebase.Name

Tue Nov  6 06:15:05 GMT 2012  Ganesh Sittampalam <ganesh@earth.li>
  * clean up extensions/flags in Darcs.Rebase.Select

Tue Nov  6 18:38:21 GMT 2012  Ganesh Sittampalam <ganesh@earth.li>
  * make RebaseSelect use RebaseFixup

Wed Nov  7 00:18:06 GMT 2012  Ganesh Sittampalam <ganesh@earth.li>
  * add some documentation for the Rebase code

Attachments

• break-out-darcs_patch_rebase_select-module.dpatch
• patch-preview.txt
• unnamed

I've reviewed this and the original two rebase patches
http://bugs.darcs.net/patch920.

I only seem to have trivial comments, maybe this is a good thing!

- code: commuterRebasing - why is the Suspended :> Suspended case not using
  impossible, if it shouldn't happen?

- trivial: perhaps use Suspended ps/qs rather than Suspended p/q, to give a
  hint that it's a sequnce.

- hmm: The Repair classes are all a bit unexplained and wtf-y

- code: Why are the 2 cases for unsafeCompare commented out? They are
covered
  by the wildcard cases for the Add/Del, no?

- code: Does the ordering in the definition of MyEq for RebaseSelect matter?
  (one checks fixups, edit, the other in the reverse order)

- typo: error case for Rename of mkDummy says it's a delete.

- code: desc in mkReified could just be []?

- hmm: What are the implications of calling revertRepositoryChanges in
  Repository/Rebase?

On 10/11/2012 23:10, Owen Stephens wrote:

> Owen Stephens <darcs@owenstephens.co.uk> added the comment:
> 
> I've reviewed this and the original two rebase patches
> http://bugs.darcs.net/patch920.
> 
> I only seem to have trivial comments, maybe this is a good thing!
> 
> - code: commuterRebasing - why is the Suspended :> Suspended case not using
>   impossible, if it shouldn't happen?

I guess because I view the patch layer as something separate from the
layer that uses it; it would be possible to have multiple Suspended
patches in a repo, we just don't. So I just included the code as it was
easy and might be useful in future. I think if we did get multiple
Suspended patches by accident we'd notice some other way in any case.

> - trivial: perhaps use Suspended ps/qs rather than Suspended p/q, to give a
>   hint that it's a sequnce.

Agreed.

> - hmm: The Repair classes are all a bit unexplained and wtf-y

As discussed on IRC, RepairInternalFL does have a comment now which
should be adequate.

> - code: Why are the 2 cases for unsafeCompare commented out? They are
> covered
>   by the wildcard cases for the Add/Del, no?
> 
> - code: Does the ordering in the definition of MyEq for RebaseSelect matter?
>   (one checks fixups, edit, the other in the reverse order)

It's actually necessary for witnesses; you need to know the beginning
contexts are the same before you can call =\/= to check that the ending
contexts are the same. In the backwards case you therefore have to do it
in the other order.

> - typo: error case for Rename of mkDummy says it's a delete.
> 
> - code: desc in mkReified could just be []?

Thanks, fixed both.

> - hmm: What are the implications of calling revertRepositoryChanges in
>   Repository/Rebase?

revertRepositoryChanges is badly named. It's actually needed to start
making changes to a repository. See for example
Darcs.Repository.Job.withRepoLock. I'll add a bit of documentation to
make that clearer. I had to refresh my own memory of this just now by
seeing what happened without it!

Since that code is running outside the main repository job, it needs to
manage the transaction itself (which is probably one cause of the
outstanding bug where a rebase is initiated even if no patches are
suspended).

I've sent the updates: http://bugs.darcs.net/patch979

Ganesh