1 patch for repository http://darcs.net/screened:

patch 0a1d9c66ded7595efa9542660d81b701961a2039
Author: Ben Franksen <ben.franksen@online.de>
Date:   Wed Oct  3 01:05:26 CEST 2018
  * resolve issue1959: catch permission errors when accessing the index

Attachments

• patch-preview.txt
• resolve-issue1959_-catch-permission-errors-when-accessing-the-index.dpatch
• unnamed

I think we should definitely do this on read-only operations, but
I don't think it's a good idea to just silently continue if there's
a permission error on a repo-affecting operation which calls
updateIndex or invalidateIndex.

If the index is out of date it will lead to incorrect behaviour
including recording an incorrect patch.

> I think we should definitely do this on read-only operations, but
> I don't think it's a good idea to just silently continue if there's
> a permission error on a repo-affecting operation which calls
> updateIndex or invalidateIndex.
> 
> If the index is out of date it will lead to incorrect behaviour
> including recording an incorrect patch.

Right. I thought that if there is a permission error then repo-changing
operations will fail anyway, sooner or later. But there is one scenario
I didn't consider: it could be that _darcs is writable and only the
index file itself or the index_invalid file is not.

I guess we can check if this is the case and then remove the index file
and similarly with the index_invalid file. We could do this inside
revertRepositoryChanges i.e. at the start of a transaction, so that
would nicely capture the difference between repo-changing and read-only
operations.

Have I overlooked anything?

If the index file isn't writable how can we remove anything? But
I guess we could at least fail up front.

My original comment was implicitly assuming that we can just fail
on the functions that would be called in a repository-modifying 
operation, but now I think about it again, I guess those aren't 
easily distinguished - this code  always confuses me.

Perhaps we need to do it more explicitly and pass down an explicit
flag?

> If the index file isn't writable how can we remove anything?

IIRC, on Unix-like systems, you need only write access to the parent
directory to remove a file for which you have at least user or group
ownership. Even if that fails you can definitely rename the file.

> My original comment was implicitly assuming that we can just fail
> on the functions that would be called in a repository-modifying 
> operation, but now I think about it again, I guess those aren't 
> easily distinguished - this code  always confuses me.

Same here.

> Perhaps we need to do it more explicitly and pass down an explicit
> flag?

I am strictly against that. We would end up adding the parameter to
*many* functions, i.e. almost everything in D.R.State and who knows what
else. We do have a perfectly fine mechanism for making this distiction,
namely whether we run in a transaction or not. We just have to make sure
that when we start a transaction we test writability up front, so we can
gracefully fail. This is quite similar to what we do if we can't take
the repo lock. The rest of the code can then silently ignore permission
errors for _darcs/index and related files. This isn't perfect but IMO
still the best we can do apart from fixing the index API (and its code).

Cheers
Ben

(Sorry for another long delay in catching up with reviews)

I think you're right that we would need to fix the index API/implementation
to clearly separate operations which can safely fail from operations that
should abort the whole operation if they do.

In the meantime the upfront check sounds good enough.

The test is failing on Windows. It looks it's because we just
get a "does not exist" error that isn't recognised by 
'isPermissionError'. I'll dig further, just making a note in
case I don't have time to finish it immediately.

I think the fact that this doesn't work on Windows together with 
everything I found out lately about the index means that I should 
either roll back this patch or at least start afresh and change 
things more systematically. This will involve completely disabling 
use of the index for commands that don't take the lock (see my 
latest comment on issue1959 about why we should do so regardless of 
the issue in question). To make that possible, I need to add a 
useIndex parameter to everything that accesses the index, directly 
or indirectly.

If I send a rollback, can you please make sure this bundle is only 
accepted after I attach the actual fix? Otherwise the issue will (I 
think) be incorrectly marked as resolved when the patch is accepted.

Is it pushing the patch to reviewed that makes the issue as 
resolved,
or marking the patch as accepted on the tracker, or something else?
I've lost track of that setup and what bits actually work.

Either way, I think we can manually change the issue status back
after it transitions, and then it'll stay as unresolved.

> Is it pushing the patch to reviewed that makes the issue as 
> resolved,
> or marking the patch as accepted on the tracker, or something else?

I thought it was the latter.

> I've lost track of that setup and what bits actually work.

If it works, then some of the issues I reported and fixed lately should
now be marked as resolved. Hm, let's see: resolve issue2604 is
definitely accepted, but the issue is still open. Seems marking issues
as resolved is broken.

This bundle rolls back the patch and instead solves the underlying problems
in a more systematical way. See patches descriptions for details.

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

patch 27e85830d38d35d7840ec54f8b7dea0c4da8dd1e
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Nov 16 21:43:07 CET 2018
  * Darcs.UI.Commands.Diff: work with a temporary copy of pristine.hashed
  
  This allows to use 'darcs diff' on a repo where we have no write access to
  _darcs/pristine.hashed.

patch cb81fa97fb972277b776139859ca484b6c2e8d78
Author: Ben Franksen <ben.franksen@online.de>
Date:   Fri Nov 16 17:09:59 CET 2018
  * check writability of index when we start a transaction

patch ae7aab2ac96d8e07b21a9194b99fd1af083310cd
Author: Ben Franksen <ben.franksen@online.de>
Date:   Mon Dec  3 19:13:10 CET 2018
  * rollback of "resolve issue1959"
  
  This change is unsound for several reasons. One is that catching permission
  errors is unreliable as it can depend on the OS what kind of IOError is
  thrown. Another issue is that catching these errors may hide problems for
  repo-modifying commands like add and remove where we rely on the index being
  updated or at least being invalidated properly.

patch cff227050b324cc76dd38076fecd888121ac0b85
Author: Ben Franksen <ben.franksen@online.de>
Date:   Mon Dec  3 19:24:24 CET 2018
  * extend test for issue1959
  
  It now covers many more read-only commands and also checks with
  --no-ignore-times (for those commands that support it). Also test that for
  repo-changing commands we get a decent error message if _darcs is writable,
  but either _darcs/index or _darcs/index_invalid is not.

patch 2fb6b284aaaaa20cf3c4c628fdc15510cbd71716
Author: Ben Franksen <ben.franksen@online.de>
Date:   Mon Dec  3 19:29:54 CET 2018
  * fully respect the useIndex option
  
  This hopefully eliminates any remaining unchecked uses of the index.
  Darcs.Repository.State now contains a CPP macro TEST_INDEX to control
  whether some testing code is included that accesses the index /and/ does a
  readPlainTree with an appropriate filter and then checks that the resulting
  trees are equal. We also pass the repoLocation explicitly to readPlainTree
  now (if available), because it turned out that some uses of the functions in
  D.R.State are made with a CWD that is not equal to the repo base dir.
  The implementation of readUnrecorded is now in terms of
  readUnrecordedFiltered instead of repeating ourselves.

patch 248f84c2847cc7c453c395be6b73c3cb0ada8d92
Author: Ben Franksen <ben.franksen@online.de>
Date:   Mon Dec  3 19:33:14 CET 2018
  * resolve issue1959: read-only commands should not need write access to the index

Attachments

• darcs_ui_commands_diff_-work-with-a-temporary-copy-of-pristine_hashed.dpatch
• patch-preview.txt
• unnamed

The darcs diff item in the test fails on Windows, I think when
trying to remove one of the trees being used for the diff. I'll
debug further when I have a chance.

The reason is that you can't just remove read-only files on Windows,
you have to change their permissions first. Using the newer
removePathForcibly fixes this:
https://hackage.haskell.org/package/directory-1.2.7.0/docs/System-
Directory.html#v:removePathForcibly (we already depend on
directory >= 1.2.7)

Do you see any problem with replacing all uses of 
removeDirectoryRecursive with that? It's the one in withDir in
Darcs.Util.Lock that really matters here, but there are a few
others dotted around the codebase.

I experimented a bit more and I don't think always using
removePathForcibly is a good idea, as users might have deliberately
protected things from being deleted. For example
the issue2271 test expects that "darcs optimize disable-patch-index" 
will fail to delete the patch index if it's not writable.

I'll do it conditionally only for cases like diff where we are 
making a private copy of the repository elsewhere.

1 patch for repository darcs-unstable@darcs.net:screened:

patch 351c06b6df1e556236108c392291c17bfee61282
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Sat Dec  8 21:50:29 GMT 2018
  * fix darcs diff in issue1959 test on Windows
  
  diff copies the repository with the current permissions,
  and on Windows removeDirectoryRecursive can't remove
  read-only files

Attachments

• fix-darcs-diff-in-issue1959-test-on-windows.dpatch
• patch-preview.txt
• unnamed

> The reason is that you can't just remove read-only files on Windows,
> you have to change their permissions first. Using the newer
> removePathForcibly fixes this:
> https://hackage.haskell.org/package/directory-1.2.7.0/docs/System-
> Directory.html#v:removePathForcibly (we already depend on
> directory >= 1.2.7)
> 
> Do you see any problem with replacing all uses of 
> removeDirectoryRecursive with that? It's the one in withDir in
> Darcs.Util.Lock that really matters here, but there are a few
> others dotted around the codebase.

Sounds perfectly fine to me.

> I experimented a bit more and I don't think always using
> removePathForcibly is a good idea, as users might have deliberately
> protected things from being deleted. 

Not sure about that. Do you have an example where that may be useful,
apart from making the whole repo read-only?

> For example
> the issue2271 test expects that "darcs optimize disable-patch-index" 
> will fail to delete the patch index if it's not writable.

If the patch-index is read-only it cannot be updated and will become out
of date. Makes no sense to me.

> I'll do it conditionally only for cases like diff where we are 
> making a private copy of the repository elsewhere.

Fine with me.

> > I experimented a bit more and I don't think always using
> > removePathForcibly is a good idea, as users might have deliberately
> > protected things from being deleted. 

> Not sure about that. Do you have an example where that may be useful,
> apart from making the whole repo read-only?

Not off the top of my head, but I prefer to be cautious when deleting
things :-)

> > For example
> > the issue2271 test expects that "darcs optimize disable-patch-index" 
> > will fail to delete the patch index if it's not writable.

> If the patch-index is read-only it cannot be updated and will become out
> of date. Makes no sense to me.

I wouldn't object to that test being removed, but probably we should
have some kind of test that read-only repositories are left alone.