Sorry, I don't normally use mailing lists and I forgot to send the below message to the list.
---------- Forwarded message ---------- From: Cathy Fitzpatrick <ca...@cathyjf.com> Date: Thu, Nov 21, 2013 at 12:22 PM Subject: Re: [PATCH] Fix for `svn patch` changing permissions of patched files To: Philip Martin <philip.mar...@wandisco.com> On Thu, Nov 21, 2013 at 4:19 AM, Philip Martin <philip.mar...@wandisco.com> wrote: > Cathy Fitzpatrick <ca...@cathyjf.com> writes: > >> Currently the `svn patch` command changes the permissions on any file >> it patches to 600. This occurs because it creates a file under the >> system temporary directory for applying the patch, and then copies >> this file to the final destination. `apr_file_mktemp` sensibly assigns >> mode 600 to files created under the system temporary directory to >> avoid them being exposed to the entire system. So the result is that >> any file that `svn patch` patches ends up with 600 permissions rather >> than whatever it had before. > > Patch is no different from other commands like revert and update in this > behaviour: > > $ ls -l wc/A/f > -rw-r--r-- 1 pm pm 2 Nov 21 10:46 wc/A/f > > $ # change permissions with patch > $ (umask 077 ; svn patch x.x wc) > U wc/A/f > $ ls -l wc/A/f > -rw------- 1 pm pm 5 Nov 21 10:47 wc/A/f > > $ # change permissions with revert > $ (umask 070 ; svn revert -R wc) > Reverted 'wc/A/f' > $ ls -l wc/A/f > -rw----rw- 1 pm pm 2 Nov 21 10:50 wc/A/f > > $ # change permissions with update > (umask 007 ; xsvn up wc) > Updating 'wc': > U wc/A/f > Updated to revision 2. > $ ls -l wc/A/f > -rw-rw---- 1 pm pm 8 Nov 21 10:51 wc/A/f > > Is there some justification for changing patch so that it is different > from the other commands? Should we change all commands? > > -- > Philip Martin | Subversion Committer > WANdisco // *Non-Stop Data* Hi Philip: First, `svn patch` is different from some other svn commands in that it always sets mode 600 on the file regardless of umask or anything else. This behaviour doesn't really make any sense and is only understandable when you look at the source and discover that it's because it copies a file from /tmp, and the file under /tmp had 600 permissions so that it wasn't exposed to the entire system. It's clearly a bug, not a design decision. Second, the present `svn patch` behaviour is a pain in some practical applications. For example, if using svn to version a website being developed where files need group read to be readable by the web server, then every time you apply a patch, group read is lost from all files affected (regardless of umask). Third, the present `svn patch` behaviour is inconsistent with the `patch(1)` command (which does not alter permissions of patched files), which makes `svn patch` counterintuitive: $ umask 000 # does not matter $ ls -la INSTALL -rw-rw-r--. 1 Cathy Cathy 63057 Nov 21 12:13 INSTALL $ patch -i patch.diff patching file INSTALL Hunk #1 succeeded at 5 with fuzz 2. $ ls -la INSTALL -rw-rw-r--. 1 Cathy Cathy 63070 Nov 21 12:14 INSTALL I'm aware that `svn patch` has other differences from `patch(1)` but this particular difference is not something you would reasonably anticipate. Fourth, some of the other svn commands should likely be changed as well. With the other commands, it's not entirely clear what is a bug and what is a design decision, but I suspect at least some of the file permission changing is just a side effect of the specific svn_io_* calls used (like with `svn patch`) rather than a design decision (because it doesn't always make sense), and this should be re-evaluated in the comprehensive manner suggested by Stefan. In any case, the present `svn patch` behaviour is clearly a bug which should be fixed. Thanks, Cathy
