Den mån 22 dec. 2025 kl 17:51 skrev Evgeny Kotkov < [email protected]>:
> Daniel Sahlberg <[email protected]> writes: > > > Do you agree that it is enough to initialize the variable to FALSE or > > would you prefer that I rework the whole feature in some way? > > Hi Daniel, > > After taking a closer look at the whole change, I'm not sure I agree with > the overall direction of the fix. > > As far as I can tell, the issue stems from inconsistencies in the Unix > implementations of functions like svn_io__is_finfo_read_only() and > svn_io_set_file_read_write(), such as: > > • A file without world write permissions is seen as read-only for root. > • After calling svn_io_set_file_read_write(TRUE) for such file without an > error, it is still reported as read-only by svn_io__is_finfo_read_only(). > > So incorrect notifications are probably just a symptom of a deeper set of > problems. And these problems may also affect other areas as well, e.g. > when used by libsvn_fs_fs/rev_file.c:auto_make_writable(). In other words, > I don't think this is a revert-specific issue; it just happens to be more > visible there because the notifications are user-facing. > > The current change results in a rather verbose notification that may also > look confusing to the users: > > > svn revert -R . > User doesn't have WRITE permissions to file 'etc/apache2/mods-available/ > dnssd.load' and the file isn't svn:needslock. But the file is already > writeable. Probably owned by another user. > User doesn't have WRITE permissions to file 'etc/apache2/mods-available/ > dnssd.conf' and the file isn't svn:needslock. But the file is already > writeable. Probably owned by another user. > User doesn't have WRITE permissions to file 'etc/apache2/conf-available/ > javascript-common.conf' and the file isn't svn:needslock. But the file > is already writeable. Probably owned by another user. > … > > Perhaps, instead of trying to communicate this via the notifications, we > could > look into fixing the root cause so that the existing notification logic > would > automatically work as intended? > > One potential step forward would be to try updating the functions to > correctly > work for the `root` user, as mentioned in the original issue report. > E.g., we > could create a matrix of permission variations and the expected behavior > for > each, and then see if the implementation can be adjusted to match. > > If we decide to go this route, I would say it might be better to move the > whole topic to 1.16. What are your thoughts on how this fits into the 1.15 > plan? > I have had another look at it and it is probably better to revert this code completely and rework the whole authorization validation. No time to do this before 1.15 though so it should be left for 1.16. I have reverted this code. Cheers, Daniel
