[darcs-devel] [patch1711] resolve various issues with pending

[Ganesh Sittampalam]

Ganesh Sittampalam <ganesh at earth.li> added the comment:

>  * 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

----------
status: needs-review -> review-in-progress

__________________________________
Darcs bug tracker <bugs at darcs.net>
<http://bugs.darcs.net/patch1711>
__________________________________