These are only weakly related small changes that came up during the UI
refactorings I am working on and are useful independently.

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

patch 60112ae92feb318a8a82901b1f865771447a82bb
Author: Ben Franksen <benjamin.franksen@helmholtz-berlin.de>
Date:   Sun Mar 19 21:20:04 CET 2017
  * remove umask and update_working parameters from withRepoLockCanFail
  
  This entry point is currently only used for attemptCreatePatchIndex. I added
  documentation to withRepoLockCanFail that makes it clear that neither
  pending nor pending.tentative should be touched by the job, making the call
  to revertRepositoryChanges unnecessary.
  
  BTW, the passed umask setting was a fake anyway: of both commands
  that use withRepoLockCanFail (log and annotate), none actually has the
  --umask switch (and rightly so, since the user doesn't expect them to write
  any files) so we always get the default NoUMask.

patch c1ace9b289fa4b9fbe62fae016bbbfaf7fabd565
Author: Ben Franksen <benjamin.franksen@helmholtz-berlin.de>
Date:   Mon Mar 20 12:22:53 CET 2017
  * removed Darcs.Util.Environment: we require base >= 4.8 anyway so just use lookupEnv

patch a96ee7233608ab56c26a195aab2bebb83254603d
Author: Ben Franksen <benjamin.franksen@helmholtz-berlin.de>
Date:   Wed Mar 22 01:11:44 CET 2017
  * fixed Darcs.Util.Printer.vsep by folding over a new binary operator vplus
  
  The new operator vplus is like <+>, only in the vertical direction. That is,
  it inserts an empty line between the operands, but only if both are
  non-empty. The previous implementation of vsep created two extra empty lines
  for each empty Doc in the input list.

patch 26ac53bb2e137da08cbc257b2554324429e73534
Author: Ben Franksen <benjamin.franksen@helmholtz-berlin.de>
Date:   Wed Mar 22 09:06:13 CET 2017
  * simplified implementation of Darcs.UI.Options.All.dryRun

patch 4beefaa6858a335ef067bd4497b9ee6f603829d1
Author: Ben Franksen <benjamin.franksen@helmholtz-berlin.de>
Date:   Wed Mar 22 17:47:07 CET 2017
  * improved haddocks for Darcs.Util.Printer

patch c8daec248690426666bcbbb29cdbcf3c6d9dcc4c
Author: Ben Franksen <benjamin.franksen@helmholtz-berlin.de>
Date:   Wed Mar 22 19:24:35 CET 2017
  * add cabal.project.local (created by cabal new-configure) to .boring

Attachments

• patch-preview.txt
• remove-umask-and-update_working-parameters-from-withrepolockcanfail.dpatch
• unnamed

I wonder if those commands should take a --umask option given that they 
could actually write files for the patch index? It's likely a bit of an 
edge case, but using the wrong umask could mess up future operations on 
the file. Not something that needs to be fixed in this refactoring, 
anyway.

About the umask option: I thought if dryRun  is in effect, nothing gets
changed? If this is not the case, perhaps we should make it so. Are
there other command where things are written (cached under _darcs) even
if dryRun is in effect?

BTW, does --umask work on Windows at all?

Working through these slightly randomly, so please excuse any out-of-order comments. I'll push the ones I've 
reviewed+tested as I go.

>  * add cabal.project.local (created by cabal new-configure) to .boring

accepted

>  * improved haddocks for Darcs.Util.Printer

accepted-pending-tests. One nitpick:

> -    -- * Unsafe constructors
> +    -- * TODO: It is unclear what is unsafe about these constructors

It's now unclear why that the comment refers to, as the original comment explicitly stating that they are unsafe is 
gone. Maybe rephrase as "TODO: These constructors were marked as unsafe, but it's unclear why"

>  * removed Darcs.Util.Environment: we require base >= 4.8 anyway so just use lookupEnv

accepted-pending-tests

Back to the withRepoLockCanFail/umask discussion:

What does dryRun have to do with it? I don't see any use in 
annotate/log.

But in general the patch index can still be created on supposedly 
"read-only" operations, the rationale being that it's just a cache.

Your current patch looks fine, anyway.

> * simplified implementation of Darcs.UI.Options.All.dryRun

To be honest I don't really understand this patch completely, but it 
looks plausible.

> +-- singleNoArg s l f h = withDefault False [RawNoArg s l f True h]

What's this about?

>> * simplified implementation of Darcs.UI.Options.All.dryRun
> 
> To be honest I don't really understand this patch completely, but it 
> looks plausible.

singleNoArg defines a Bool valued option (either there is --foo or not).
This was mapped to the DryRun type using an isomorphism. Now it uses
withDefault with a single raw option which is a bit shorter.

>> +-- singleNoArg s l f h = withDefault False [RawNoArg s l f True h]
> 
> What's this about?

Thrash. Didn't mean to record that. Probably happened because hunk
overlapped with deletion of __dryrun (which wasn't used).

> Ganesh Sittampalam <ganesh@earth.li> added the comment:
> 
>> -    -- * Unsafe constructors
>> +    -- * TODO: It is unclear what is unsafe about these constructors
> 
> It's now unclear why that the comment refers to, as the original comment explicitly stating that they are unsafe is 
> gone. Maybe rephrase as "TODO: These constructors were marked as unsafe, but it's unclear why"

The functions this comment refers to all have "unsafe" prefix in their
name. In fact, I remember putting in the original comment (actually a
heading for haddock) based solely on their names when I re-organized the
exports of this module.

re umask: I have opened a ticket for that (issue2533).

> The functions this comment refers to all have "unsafe" prefix
> in their name. In fact, I remember putting in the original
> comment (actually a heading for haddock) based solely on their names 
> when I re-organized the exports of this module.

Sorry, I should have read the context :-) Makes sense then.


> Thrash. Didn't mean to record that. Probably happened because hunk
> overlapped with deletion of __dryrun (which wasn't used).

OK.