darcs

Patch 1738 resolve issue1959: catch permission errors when access...

Title resolve issue1959: catch permission errors when access...
Superseder Nosy List bf
Related Issues
Status review-in-progress Assigned To
Milestone

Created on 2018-10-02.23:12:43 by bf, last changed 2018-10-11.09:50:00 by bf.

Files
File name Status Uploaded Type Edit Remove
patch-preview.txt bf, 2018-10-02.23:12:43 text/x-darcs-patch
resolve-issue1959_-catch-permission-errors-when-accessing-the-index.dpatch bf, 2018-10-02.23:12:43 application/x-darcs-patch
unnamed bf, 2018-10-02.23:12:43 text/plain
See mailing list archives for discussion on individual patches.
Messages
msg20352 (view) Author: bf Date: 2018-10-02.23:12:43
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
msg20379 (view) Author: ganesh Date: 2018-10-06.16:48:37
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.
msg20386 (view) Author: bf Date: 2018-10-06.21:12:08
> 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?
msg20390 (view) Author: ganesh Date: 2018-10-10.05:57:59
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?
msg20392 (view) Author: bf Date: 2018-10-11.09:49:59
> 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
History
Date User Action Args
2018-10-02 23:12:43bfcreate
2018-10-02 23:13:32bfsetstatus: needs-screening -> needs-review
2018-10-06 16:48:37ganeshsetstatus: needs-review -> review-in-progress
messages: + msg20379
2018-10-06 21:12:13bfsetmessages: + msg20386
2018-10-10 05:58:00ganeshsetmessages: + msg20390
2018-10-11 09:50:00bfsetmessages: + msg20392