Some more refactorings, originally only for the diff command. These depend
on a refactor of the matchFirst/SecondPatchset range matching functions
which in turn pulled in a dependency on a minor UI change to the 'show
dependencies' command.

Also follow up on review of patch1954 with comments in the diff command
implementation.

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

patch c10013a29a6341db6ed596378539162dd4d75391
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Jan 26 20:35:15 CET 2020
  * diff command: make construction of trees more obviously correct

patch 3f5def91ce548480b0e9aad4f1f67816769ff623
Author: Ben Franksen <ben.franksen@online.de>
Date:   Wed Jan 29 10:33:31 CET 2020
  * diff command: simplify display of the log

patch 428804718ada72c4dd91cc217586134cfb248798
Author: Ben Franksen <ben.franksen@online.de>
Date:   Wed Jan 29 10:35:19 CET 2020
  * refactor matchFirst/SecondPatchset
  
  These functions now return a Maybe instead of being partial functions. This
  simplifies the call sites and avoids duplicated checks.

patch 12bead4cc6802d4dd2f25c8672f36c7f750e10dd
Author: Ben Franksen <ben.franksen@online.de>
Date:   Wed Jan 29 13:43:01 CET 2020
  * review matching options for show dependencies
  
  Showing the dependency graph makes the most sense for a range of patches. We
  rename the combined matching option matchRange to matchOneOrRange (used for
  diff) and add a different matchRange function that excludes the single patch
  variants. Since the log command already has a function matchRange, we factor
  that into D.UI.Command.Util and re-use it.

patch 2c65457ed16e2eb0c0d09de437e91a61b5e8a6fa
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Feb  9 21:53:26 CET 2020
  * diff command: regroup and re-layout statements in doDiff

patch 9b08855dcf8cd10da3f067b9277bdfcee4eb0ebd
Author: Ben Franksen <ben.franksen@online.de>
Date:   Tue Feb 11 09:09:54 CET 2020
  * diff command: add comments to explain the subtleties

Attachments

• diff-command_-make-construction-of-trees-more-obviously-correct.dpatch
• patch-preview.txt
• unnamed

Partial review:

On 11/02/2020 08:13, Ben Franksen wrote:

>   * diff command: make construction of trees more obviously correct

Fine

>   * diff command: simplify display of the log

Fine (the old code does indeed seem quite convoluted)

>   Sealed all <- return $ case (secondMatch matchFlags, patchset) of
>     (True, _) -> seal patchset
>     (False, s) -> seal $ patchSetSnoc s unrecorded'
> 
>   Sealed ctx <- return $ if firstMatch matchFlags
>                             then matchFirstPatchset matchFlags patchset
>                             else seal patchset
> 
>   Sealed match <- return $ if secondMatch matchFlags
>                              then matchSecondPatchset matchFlags patchset
>                              else seal all
> 
>   Sealed logmatch <- return $ if secondMatch matchFlags
>                                 then seal match
>                                 else seal patchset

The different ifs for secondMatch matchFlags feel a bit convoluted. It
looks like it could be reduced to something like this (or the equivalent
post the Maybe refactoring below)

(Sealed all, Sealed match, Sealed logmatch) <-
  if secondMatch matchFlags
    then let match = matchSecondPatchSet matchFlags patchset
         in (seal patchset, seal match, seal match)
    else let all = patchSetSnoc patchset unrecorded'
         in (seal all, seal all, seal patchset)

Anyway, just a comment, I might send a followup myself.

> patch 428804718ada72c4dd91cc217586134cfb248798
> Author: Ben Franksen <ben.franksen@online.de>
> Date:   Wed Jan 29 10:35:19 CET 2020
>   * refactor matchFirst/SecondPatchset
>   
>   These functions now return a Maybe instead of being partial functions. This
>   simplifies the call sites and avoids duplicated checks.

Can firstMatch and secondMatch be rewritten as isJust .
matchFirstPatchSet etc? I think they can, so we should. If not this
refactoring may be unsound :-)

>   * refactor matchFirst/SecondPatchset

In addition to my previous comments about firstMatch and secondMatch, we
might be close to just removing them entirely.

For example getLastPatches is now partial (moved outwards from
matchFirstPatchset) but actually all its call sites are themselves
guarded by firstMatch.

Again this is just a comment, your changes are an improvement and we can
do more in future.

>   * review matching options for show dependencies
>   
>   Showing the dependency graph makes the most sense for a range of patches. We
>   rename the combined matching option matchRange to matchOneOrRange (used for
>   diff) and add a different matchRange function that excludes the single patch
>   variants. Since the log command already has a function matchRange, we factor
>   that into D.UI.Command.Util and re-use it.

OK

> patch 2c65457ed16e2eb0c0d09de437e91a61b5e8a6fa
> Author: Ben Franksen <ben.franksen@online.de>
> Date:   Sun Feb  9 21:53:26 CET 2020
>   * diff command: regroup and re-layout statements in doDiff


OK

> patch 9b08855dcf8cd10da3f067b9277bdfcee4eb0ebd
> Author: Ben Franksen <ben.franksen@online.de>
> Date:   Tue Feb 11 09:09:54 CET 2020
>   * diff command: add comments to explain the subtleties

Looks good, thanks.

>>   * refactor matchFirst/SecondPatchset
>>   
>>   These functions now return a Maybe instead of being partial functions. This
>>   simplifies the call sites and avoids duplicated checks.
> 
> Can firstMatch and secondMatch be rewritten as isJust .
> matchFirstPatchSet etc? I think they can, so we should. If not this
> refactoring may be unsound :-)

Good point. I checked and they can and I agree they should.

Attachments

• pEpkey.asc