PatchSet related refactorings. The idea is to concentrate functions working
directly on the PatchSet data type into the Darcs.Patch.Set module, at least
where this is possible without completely dircupting the existing code.

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

patch f6b3575916d88ebfe7a5841bd4d2eba86c762b3f
Author: Ben Franksen <ben.franksen@online.de>
Date:   Mon Jun 11 19:19:52 CEST 2018
  * moved inOrderTags from convert command to D.P.Set

patch 6d52d8bb2531431ee831c20400d255c2f164f7ab
Author: Ben Franksen <ben.franksen@online.de>
Date:   Mon Jun 11 20:17:05 CEST 2018
  * avoid access to PatchSet constructors where possible
  
  This includes the full UI subsystem but also parts of Patch and Repository.

patch 016f59a83971a6256e4c273922dec8086d5e8728
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Jul 19 00:47:24 CEST 2018
  * replace D.UI.C.Util.repoTags with D.P.Set.patchSetTags
  
  Also remove unused functions 'tags' and 'patchSetfMap' from D.P.Set.

Attachments

• moved-inordertags-from-convert-command-to-d_p_set.dpatch
• patch-preview.txt
• unnamed

>   * moved inOrderTags from convert command to D.P.Set

Fine

>   * avoid access to PatchSet constructors where possible

> hunk ./src/Darcs/UI/SelectChanges.hs 1013
> +  _ :> x <- return $ contextPatches pps
> hunk ./src/Darcs/UI/SelectChanges.hs 1015
> -  FlippedSeal ps <-
> -    return $ case pps of PatchSet _ x -> FlippedSeal (x+>>+(pa:>:NilFL))
> -  let my_lps = labelPatches Nothing ps
> +  let my_lps = labelPatches Nothing (x+>>+(pa:>:NilFL))

This isn't obviously behaviour-preserving, given that
contextPatches does a slightlyOptimizePatchset. Can you
explain it a bit more?

Rest looks fine.

>   * replace D.UI.C.Util.repoTags with D.P.Set.patchSetTags

Fine

[first sent this to the list only, sorry for duplicates]

>>   * avoid access to PatchSet constructors where possible
>
>> hunk ./src/Darcs/UI/SelectChanges.hs 1013
>> +  _ :> x <- return $ contextPatches pps
>> hunk ./src/Darcs/UI/SelectChanges.hs 1015
>> -  FlippedSeal ps <-
>> -    return $ case pps of PatchSet _ x -> FlippedSeal (x+>>+(pa:>:NilFL))
>> -  let my_lps = labelPatches Nothing ps
>> +  let my_lps = labelPatches Nothing (x+>>+(pa:>:NilFL))
>
> This isn't obviously behaviour-preserving, given that
> contextPatches does a slightlyOptimizePatchset. Can you
> explain it a bit more?

You are right. The change means we won't be offered any patches beyond a
tag if that tag happens to be clean but darcs hasn't (yet) recognized
that. While previously we would be offered such patches up to the last
tag that was (already) recognized as clean. Note that all commands that
change the repo will do slightlyOptimizePatchset before changes are
finalized. So the only situation where this could happen is when we
unsuspend multiple patches with --ask-deps, one of which is a tag and we
want a later unsuspended patch to depend on a patch that comes before
that tag.

Anyway, I don't think this matters much. I am not sure what the
motivation was in limiting the selection to "loose" patches in the first
place. It makes no sense to me and I think it unnecessarily restricts
the use of --ask-deps. But if we take it as given, then surely /after/
we recorded or amended the patch in question, a slightlyOptimizePatchset
will have been done. So we wouldn't be able to add that dependency in a
new amend, no matter what. Which means that the behavior is now more
consistent than before.

I think the code should be fixed, but so as to allow to choose any patch
from the past, getting rid of the contextPatches call.