darcs

Patch 1772 remove currentDirectory from TreeMonad

Title remove currentDirectory from TreeMonad
Superseder Nosy List bf
Related Issues
Status needs-screening Assigned To
Milestone

Created on 2018-12-03.13:36:16 by bf, last changed 2018-12-10.14:16:27 by bf.

Files
File name Status Uploaded Type Edit Remove
patch-preview.txt bf, 2018-12-03.13:36:16 text/x-darcs-patch
remove-currentdirectory-from-treemonad.dpatch bf, 2018-12-03.13:36:16 application/x-darcs-patch
unnamed bf, 2018-12-03.13:36:16 text/plain
See mailing list archives for discussion on individual patches.
Messages
msg20566 (view) Author: bf Date: 2018-12-03.13:36:16
1 patch for repository http://darcs.net/screened:

patch 1e1ffcee75dc90ad667197b513b7117e83cfe859
Author: Ben Franksen <ben.franksen@online.de>
Date:   Mon Dec  3 14:37:26 CET 2018
  * remove currentDirectory from TreeMonad
  
  Adding a notion of "current directory" to the TreeMonad is an unnecessary
  complication at best. It is clearer to explicitly pass the current directory
  when using the TreeMonad interface. Thankfully, this feature was used only
  in a single function in Darcs.UI.Commands.Util.
Attachments
msg20581 (view) Author: bf Date: 2018-12-04.18:31:09
I am not screening this one immediately. If you think that
currentDirectory is an important part of TreeMonad, please shout.
msg20591 (view) Author: ganesh Date: 2018-12-06.06:29:48
This change doesn't affect whether darcs does filesystem operations
relative to the current working directory, does it?

A while ago ago we spent some time looking at whether we could stop
darcs from depending on the current working directory for the 
process, as it's implicit global state that makes it hard to use the 
darcs library from multiple threads. One worry was that it would
have a significant performance impact because it's slower for the OS
to look up an absolute path than one relative to CWD. I think a
factor of two was mentioned, but I have no idea what the current
situation would be.

The patch also has a lot of reformatting along with the logic
changes that will make it harder to review.
msg20604 (view) Author: bf Date: 2018-12-10.14:16:26
> This change doesn't affect whether darcs does filesystem operations
> relative to the current working directory, does it?

No, of course not. It just means you cannot zoom into a subtree as a
"built-in" operation of TreeMonad.

> A while ago ago we spent some time looking at whether we could stop
> darcs from depending on the current working directory for the 
> process, as it's implicit global state that makes it hard to use the 
> darcs library from multiple threads. One worry was that it would
> have a significant performance impact because it's slower for the OS
> to look up an absolute path than one relative to CWD. I think a
> factor of two was mentioned, but I have no idea what the current
> situation would be.

I guess it also depends on how long the path to the repository is. My
guess is that this is especially bad in Haskell, since all the IO
operations assume FilePath = String.

The problem with the current working directory is the file system API,
which for traditional reasons has only one working directory for the
whole process. A more modern API would allow the user to create multiple
working directories and to pass them explicitly (and interpret relative
paths using explicit working directories). We can emulate such a modern
API but this would be pointless, as it would rob us of the performance
improvement.

However, this is alltogether a different issue.

> The patch also has a lot of reformatting along with the logic
> changes that will make it harder to review.

I has some reformatting, I wouldn't say "a lot". The import list
reformat shouldn't be an issue for review. The existsAnycase function
has changes on almost all its code lines, so I saw no sense in
preserving the existing layout. The data type reformat could have been
left out, though, sorry for that.
History
Date User Action Args
2018-12-03 13:36:16bfcreate
2018-12-04 18:31:09bfsetmessages: + msg20581
2018-12-06 06:29:49ganeshsetmessages: + msg20591
2018-12-10 14:16:27bfsetmessages: + msg20604