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?
Thanks,
Evgeny Kotkov
