Sorry, I don't normally use mailing lists and I forgot to send the
below message to the list.
---------- Forwarded message ----------
From: Cathy Fitzpatrick <cathy_at_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.martin_at_wandisco.com>
On Thu, Nov 21, 2013 at 4:19 AM, Philip Martin
<philip.martin_at_wandisco.com> wrote:
> Cathy Fitzpatrick <cathy_at_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
Received on 2013-11-21 20:24:54 CET