darcs

Patch 1711 resolve various issues with pending

Title resolve various issues with pending
Superseder Nosy List bf
Related Issues
Status review-in-progress Assigned To
Milestone

Created on 2018-07-18.19:59:09 by bf, last changed 2018-08-08.18:11:10 by ganesh.

Files
File name Status Uploaded Type Edit Remove
resolve-issue2548_-inconsistent-pending.dpatch bf, 2018-07-22.22:31:19 application/x-darcs-patch
See mailing list archives for discussion on individual patches.
Messages
msg20208 (view) Author: bf Date: 2018-07-18.19:59:09
This bundle solves issue2548, issue2592, and issue1316, as well as an
issue with no number (tests/look_for_replaces1.sh).

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

patch 940da5fdbc54e64875e3ef9a962e582cef9b0423
Author: Ben Franksen <ben.franksen@online.de>
Date:   Wed Jun 13 19:50:34 CEST 2018
  * resolve issue2548: inconsistent pending
  
  In Darcs.Repository.Diff.treeDiff we did not correctly handle the case
where
  a file is changed to a subTree: we only emptied the file but failed to add
  an rmfile patch before adding the dir.
  
  With a pending "addfile ./f" and a working tree where f has been
changed to
  a directory, we now get only the "adddir ./f" when recording with -l.
  
  However, this leaves us with a new pending "rmdir ./f; addfile ./f". This
  pending change can be reverted but does not show up with whatsnew -l and
  cannot be recorded with record -l.

patch 85c22f36716b5abd2a398a7c3a6cd81d7b6f3070
Author: Ben Franksen <ben.franksen@online.de>
Date:   Mon Jun 18 21:05:30 CEST 2018
  * accept issue2592: unclean pending with look-for options

patch ffa25a85a94ebb4ec8ac856fdb33135387cb34e4
Author: Ben Franksen <ben.franksen@online.de>
Date:   Mon Jul  2 15:40:08 CEST 2018
  * fix handling of pending patch in amend command
  
  In case of look-for-moves/replaces, the code called
  tentativelyRemoveFromPending on the old patch. This was done after
  tentativelyRemovePatch and tentativelyAddPatch had already adapted the
  pending patch (via YesUpdatePending). This is clearly wrong and worked
only
  because tentativelyRemovePatches called prepend, which pre-filtered
the new
  prim patches with crudeSift. This is no longer done, and so we now get
  failures when we try to apply pending.
  
  The clean solution is to pass NoUpdatePending to both
tentativelyRemovePatch
  and tentativelyAddPatch and then adapt pending in one go with the
difference
  between old and new patch. The function tentativelyRemoveFromPending now
  takes an FL of prim patches, has more correct type witnesses, uses less
  coercion, and has its UpdatePending parameter removed.
  
  This restores the regression of issue2209-look_for_replaces after the fix
  for issue2548-inconsistent-pending. It also makes the previously failing
  test look_for_replaces1 succeed.

patch 6bbdbe7d6c50ae89a86aa2461c82b42df5658fe5
Author: Ben Franksen <ben.franksen@online.de>
Date:   Mon Jul  2 16:51:01 CEST 2018
  * make D.R.Pending.prepend more type safe (by removing it)
  
  This function coerced witnesses and had corresponding warnings
attached. The
  need for coercing was that it reads the pending patch and also writes it
  back with some changes that were removed from the repo prepended. If we
  split this action up and read pending before adding the patches to the
repo
  and afterwards write the pending, then the witnesses all match up just
fine.
  This requires removeFromTentativeInventory to return a repo with
  appropriately coerced witnesses.
  The cost of this operation is that we must export
read/writeTentativePending.

patch a3e7b0254ad94acc5c30b056f27a5a032f0eba17
Author: Ben Franksen <ben.franksen@online.de>
Date:   Mon Jul  2 16:51:12 CEST 2018
  * simplified setTentativePending and fix its type witnesses

patch 90c1861cb3d104833964c1328075febc49dd9e98
Author: Ben Franksen <ben.franksen@online.de>
Date:   Mon Jul  2 18:38:03 CEST 2018
  * resolve issue2592: update pending with coalesced look-for changes

patch b072c4eba14e88f51a5d1b8b6ac0076c9cd40a0d
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Jul 13 15:44:49 CEST 2018
  * tests for issue1316 no longer fail
  
  This is most probably due to the recent fix for how the amend command
  handles the pending patch.
msg20221 (view) Author: bf Date: 2018-07-22.22:31:19
Oops, forgot to attach the bundle.
Attachments
msg20239 (view) Author: ganesh Date: 2018-08-08.18:11:09
>  * resolve issue2548: inconsistent pending

> hunk ./src/Darcs/Repository/Diff.hs 134
> -        do rmFileP <- diff p (Changed a' (File emptyBlob))
> +        do emptyFileP <- diff p (Changed a' (File emptyBlob))
> +           rmFileP <- diff p (Removed a')
> hunk ./src/Darcs/Repository/Diff.hs 137
> -           return $ joinGap (+>+) rmFileP addDirP
> +           return $ joinGap (+>+) (joinGap (+>+) emptyFileP rmFileP) addDirP

OK - should the analogous case just below for going the other way
also be changed (to get an addfile patch)?

>  * accept issue2592: unclean pending with look-for options

OK

>  * fix handling of pending patch in amend command

OK

>  * make D.R.Pending.prepend more type safe (by removing it)

OK

>  * simplified setTentativePending and fix its type witnesses

OK (you actually simplified tentativelyAddToPending as well)

>  * resolve issue2592: update pending with coalesced look-for changes

> --- | @tentativelyRemoveFromPending p@ is used by Darcs whenever it
> ---   adds a patch to the repository (eg. with apply or record).
> ---   Think of it as one part of transferring patches from pending to
> ---   somewhere else.

I actually find this comment you've removed quite helpful for building
intuition, so it would be good if it could be restored in some form,
and if tentativelyRemoveFromPW could also have a doc comment.

If I understand correctly, tentativelyRemoveFromPending is now only used
via tentativelyAddPatch with 'YesUpdatePending', which in turn may
only be used from Rebase and Tag - the former is probably for no good
reason, and I doubt it really matters with Tag.

> +-- We aim for the following property: the result should be the same as if patch
> +-- selection had been done with the un-coalesced @pending+>+working@ (with the
> +-- original patches selected in place of the coalesced ones), and if, of that
> +-- sequence, the elements that were originally in @pending@ are now removed
> +-- from it.

Thanks for writing a specification for this :-) What exactly do you mean by
"patch selection" here? Just that it's like looking for the relevant patches
in the patch selection UI? I also can't understand "if, of that sequence".

It's also not immediately obvious to me why working now is a parameter to this
stack of operations - if you could explain that a bit in the comment it'd be
great.


> -                if nullFL chs && null deps
> -                  then putStrLn "Ok, if you don't want to record anything, that's fine!"
> -                  else do setEnvDarcsFiles chs
> -                          (name, my_log, logf) <- getLog (patchname cfg) (pipe cfg) (logfile cfg) 
(askLongComment cfg) Nothing chs
> -                          debugMessage ("Patch name as received from getLog: " ++ show (map ord name))
> -                          doActualRecord repository cfg name date my_author my_log logf deps chs
> +                when (nullFL chs && null deps) $
> +                  putStrLn "Ok, if you don't want to record anything, that's fine!"
> +                setEnvDarcsFiles chs
> +                (name, my_log, logf) <- getLog (patchname cfg) (pipe cfg) (logfile cfg) (askLongComment cfg) 
Nothing chs
> +                debugMessage ("Patch name as received from getLog: " ++ show (map ord name))
> +                doActualRecord repository cfg name date my_author my_log logf deps chs pw

This looks like an unintended change in behaviour: as the 'when' clause won't cause
an early exit from the rest of the do block, we will now try to record an empty
patch.

>  * tests for issue1316 no longer fail

Fine
History
Date User Action Args
2018-07-18 19:59:09bfcreate
2018-07-22 22:31:19bfsetfiles: + resolve-issue2548_-inconsistent-pending.dpatch
messages: + msg20221
2018-08-08 18:11:10ganeshsetstatus: needs-review -> review-in-progress
messages: + msg20239