Den mån 22 jan. 2024 kl 04:05 skrev Branko Čibej <br...@apache.org>:
> On 13.01.2024 09:58, Daniel Sahlberg wrote: > > Since there wasn't any replies to this and I think the code was working > fine in all my tests, I comitted as r1915214. Although I finally decided to > solve the spurious revert messages in a different way, see a separate > followup/committ e-mail. > > > I think you just made the code more complicated for no good reason. The > situation you described and fixed applies only to working copies that are > shared by different users. > > You also have to ask yourself what happens when your user runs an "svn > update" in the working copy where it's not the owner of the files and when > your primary group doesn't have write access to the containing directory. > In other words, I think you just fixed an edge case that has lots of other > problems, not least that without using at least 'chgrp', the working copy > will be either useless for you or quickly corrupted for others. I'm not > even going to try to think about what happens to SQLite WAL files. Your > exhaustive tests don't seem to cover these cases. > > In practice, such working copies tend to be used by prefixing all svn > commands with 'sudo -u wc-owner' or starting with 'sudo -u wc-owner -s'. > That's a solution that always works and doesn't require more code in svn > that someone has to maintain and that only solves one very specific edge > case. > > -- Brane > > Are you referring to the changes in 1915214 or in 1915215? r1915214 replaces a homegrown check for the current UID and GID, then comparing with current file mask (total 23 loc) with calls to a library function (total 4 loc). I presume there is a cost for some stat calls within access(), so performance is probably a bit worse. (Maybe some of the performance could be recovered in revert_wc_data, there is comment to use svn_io_dirent2_t "[except] that we need the read only and execute bits later", I presume for the now replaced checks). I would argue that the new code is simpler and easier to maintain, but I'm of course open to other opinions. With r1915215, I can agree it makes the code more complicated. But it solves another issue, the "spurious revert" messages reported here several times (most recently by Vincent Lefevre). I'd be the first to admit that the fix is still half-baked: The message is completely bonkers (and it lacks a trailing \n), but at least the user is notified that something is going on instead of getting the "reverted" message over and over again. Kind regards, Daniel
