On Tue, 2011-03-15, Philip Martin wrote:
> > URL: http://svn.apache.org/viewvc?rev=1081892&view=rev
> > Log:
> > Make revert change file permissions on the basis of the current magic
> > properties and the current permissions, rather than on any change in
> > the magic permissions. This means file permissions follow the same
s/magic permissions/magic properties/ ?
> > rules as file content, and that permissions-only reverts occur.
> >
> > * subversion/include/private/svn_private_io.h: New.
Doc string for svn_io__is_finfo_executable() should say, like for
svn_io_is_file_executable():
"On Windows and on platforms without userids, always returns @c
FALSE."
> > * subversion/libsvn_subr/io.c
> > (): Include svn_private_io.h.
> > (svn_io__is_finfo_read_only, svn_io__is_executable): New.
s/is_executable/is_finfo_executable/
> > (svn_io_is_file_executable): Call svn_io__is_executable.
> >
> > * subversion/libsvn_wc/wc.h
> > (svn_wc__internal_file_modified_p): New.
> >
> > * subversion/libsvn_wc/questions.c
> > (): Include svn_private_io.h.
> > (svn_wc__internal_file_modified_p): New.
> > (svn_wc__internal_text_modified_p): Call svn_wc__internal_file_modified_p.
> >
> > * subversion/libsvn_wc/adm_ops.c
> > (new_revert_internal): Use svn_wc__internal_file_modified_p to
> > determine whether to reset contents or permissions.
> >
> > * subversion/tests/cmdline/revert_tests.py
> > (revert_permissions_only): New, marked XFail but passes with new revert.
> > (test_list): Add revert_permissions_only.
>
> The implementation is a little ugly, a private svn_io API.
That seems fine to me.
> In the past a file with text and permission changes would have its
> permissions reset by revert, and a file with magic property changes
> would have its permissions reset by revert. However a file with just
> permission changes would not have its permission reset by revert.
>
> The new code I'm working on will now revert permissions in that last
> case. This could be viewed as fixing a bug, but I suppose some people
> might not like it. Permission-only changes don't show up in status.
Thinking about the read-only flag, there could conceivably be cases
where a user has become accustomed to the current behaviour. The only
likely thing I can think of is the user has a bunch of needs-lock files
which are read-only by default, and sets them to writable for
experimental editing, intending to decide later whether to commit or
revert each change. The user may be accustomed to "revert" leaving the
unmodified files as writable even though it resets any text-modified
files to read-only.
However, I think the number of such users would be very small, and they
would not be very much inconvenienced at all if 'revert' now resets all
of those files to read-only, and I think the most such users would
accept that change as a bug fix without argument.
For the 'executable' flag, I can't think of any reasonable scenario
where the old behaviour could be useful.
On the 'pro' side, it's obvious to me that it's good for consistency to
revert the file's attributes to the state in which Subversion would have
put them after a checkout or update. It's especially good to be
consistent with what it would have done if there was any other change to
the file.
So I would definitely view it as fixing a bug.
Would I be right in guessing you made this change not from an intent to
change the behaviour, but because you needed to re-implement part of
'revert' for other reasons and this behaviour was the obvious behaviour
to implement, the old behaviour being the anomaly?
- Julian
Received on 2011-03-16 10:08:57 CET