You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Cathy Fitzpatrick <ca...@cathyjf.com> on 2013/11/21 02:07:25 UTC
[PATCH] Fix for `svn patch` changing permissions of patched files
Hello,
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.
Here is an example of the current behaviour (before my patch):
[Cathy@localhost subversion]$ ls -la INSTALL
-rw-rw-r--. 1 Cathy Cathy 63057 Nov 20 17:34 INSTALL
[Cathy@localhost subversion]$ ./svn patch patch.diff
U INSTALL
[Cathy@localhost subversion]$ ls -la INSTALL
-rw-------. 1 Cathy Cathy 63070 Nov 20 17:58 INSTALL
Notice that applying the patch has changed the permissions of INSTALL.
After my patch, we instead get the following:
[Cathy@localhost subversion]$ ls -la INSTALL
-rw-rw-r--. 1 Cathy Cathy 63057 Nov 20 17:59 INSTALL
[Cathy@localhost subversion]$ ./svn patch patch.diff
U INSTALL
[Cathy@localhost subversion]$ ls -la INSTALL
-rw-rw-r--. 1 Cathy Cathy 63070 Nov 20 17:59 INSTALL
As expected, patching the file no longer alters its permissions.
I have attached my patch to this email and it includes a log message.
Regards,
Cathy
Re: [PATCH] Fix for `svn patch` changing permissions of patched files
Posted by Cathy Fitzpatrick <ca...@cathyjf.com>.
On Thu, Nov 21, 2013 at 4:52 AM, Stefan Sperling <st...@elego.de> wrote:
> On Thu, Nov 21, 2013 at 11:19:34AM +0000, Philip Martin 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?
>
> I think the proposed patch is right.
>
> However, I think we should start thinking about tweaking the svn_io_
> APIs in a way that forces callers to consider what permissions
> copy and rename destinations should have.
>
> svn_subst_copy_and_translate4() actually does copy permission from
> the source to the target. However, since the source is a temporary file
> in /tmp with restrictive permissions, that doesn't really make sense.
> So the io APIs don't really offer what 'svn patch' (and possibly
> other commands) need.
>
> If svn_subst_copy_and_translate() had an additional argument describing
> file permissions to set on the destination, all callers would be forced
> to consider this question. That would make more sense than the current
> approach of calling svn_subst_copy_and_translate() and then also calling
> copy_perms(). When writing future code, we'll keep forgetting to check
> whether an additional copy_perms() call is needed.
>
> I believe this problem affects more functions that rename or copy
> things, not just svn_subst_copy_and_translate().
>
> Cathy, would you be willing to invest a bit more time into this?
> If so, can you help us by identifying which functions in include/svn_io.h
> copy files and are setting implied permissions at the destination?
>
> If you don't have time for this, I suggest we apply your patch, and
> I'll file my above idea as an issue and get to it later.
>
> Thank you for your contribution, in any case!
>
> Once you have identified the functions, we can add arguments (creating
> a new version of each function), and then over time go through all callers
> of these functions and upgrade them to the new version. That should
> fix such issues everywhere, and will prevent new instances of the
> problem from occurring.
Hi Stefan,
I agree with you that it would make sense to review the use of the io
functions in a more comprehensive manner. I probably won't be able to
do this in the near future though. However, it would be good to fix
`svn patch` for the reasons I mentioned in my other email to Philip.
Thanks,
Cathy
Re: [PATCH] Fix for `svn patch` changing permissions of patched files
Posted by Stefan Sperling <st...@elego.de>.
On Thu, Nov 21, 2013 at 11:19:34AM +0000, Philip Martin 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?
I think the proposed patch is right.
However, I think we should start thinking about tweaking the svn_io_
APIs in a way that forces callers to consider what permissions
copy and rename destinations should have.
svn_subst_copy_and_translate4() actually does copy permission from
the source to the target. However, since the source is a temporary file
in /tmp with restrictive permissions, that doesn't really make sense.
So the io APIs don't really offer what 'svn patch' (and possibly
other commands) need.
If svn_subst_copy_and_translate() had an additional argument describing
file permissions to set on the destination, all callers would be forced
to consider this question. That would make more sense than the current
approach of calling svn_subst_copy_and_translate() and then also calling
copy_perms(). When writing future code, we'll keep forgetting to check
whether an additional copy_perms() call is needed.
I believe this problem affects more functions that rename or copy
things, not just svn_subst_copy_and_translate().
Cathy, would you be willing to invest a bit more time into this?
If so, can you help us by identifying which functions in include/svn_io.h
copy files and are setting implied permissions at the destination?
If you don't have time for this, I suggest we apply your patch, and
I'll file my above idea as an issue and get to it later.
Thank you for your contribution, in any case!
Once you have identified the functions, we can add arguments (creating
a new version of each function), and then over time go through all callers
of these functions and upgrade them to the new version. That should
fix such issues everywhere, and will prevent new instances of the
problem from occurring.
Re: Fwd: [PATCH] Fix for `svn patch` changing permissions of patched
files
Posted by Branko Čibej <br...@wandisco.com>.
On 22.11.2013 15:23, Bert Huijben wrote:
>> Branko: On Windows we never alter permissions. We let the inherited folder
>> permission handle these things. A really nice case where the ACL's make our
>> code easier, by not letting user code worry about this.
>> (There is also a per user temporary directory instead of one for all users)
That's not quite the case. We often create temp files in the top-level
.svn/tmp directory; and the inherited permissions there can be
completely different than the ones on the target (likewise for the
user's temp dir). When we move these files into place, they may have
"wrong" permissions. We've had complaints on users@ about that.
(Not that I necessarily agree with the complaints, but they're there.)
> And even if we would try to do this, it is very likely to fail as
> managing permissions is commonly not something that the owner/creator
> of a file is even allowed to do.
Yup. That's why I said it's way too much "fun" on Windows.
-- Brane
--
Branko Čibej | Director of Subversion
WANdisco // Non-Stop Data
e. brane@wandisco.com
RE: Fwd: [PATCH] Fix for `svn patch` changing permissions of patched files
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Bert Huijben [mailto:bert@qqmail.nl]
> Sent: vrijdag 22 november 2013 15:21
> To: 'Philip Martin'; 'Branko Čibej'
> Cc: dev@subversion.apache.org
> Subject: RE: Fwd: [PATCH] Fix for `svn patch` changing permissions of
> patched files
>
> Branko: On Windows we never alter permissions. We let the inherited folder
> permission handle these things. A really nice case where the ACL's make our
> code easier, by not letting user code worry about this.
> (There is also a per user temporary directory instead of one for all users)
And even if we would try to do this, it is very likely to fail as managing permissions is commonly not something that the owner/creator of a file is even allowed to do.
Bert
Re: Fwd: [PATCH] Fix for `svn patch` changing permissions of patched
files
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 11/22/2013 10:26 AM, Philip Martin wrote:
> We want to limit use of the system temp dir for various reasons:
>
> - the system dir may be a small filesystem with much less space than
> the working copy filesystem
>
> - if Subversion is interrupted files left in the system dir can't
> easily be removed later by Subversion
- the system dir may be a ramdisk.
- the system dir may be on a different volume, forcing cross-
volume "renames".
Re: Fwd: [PATCH] Fix for `svn patch` changing permissions of patched files
Posted by Philip Martin <ph...@wandisco.com>.
"Bert Huijben" <be...@qqmail.nl> writes:
>> - svn_subst_copy_and_translate4 uses the parent directory of a
>> temporary file as the temporary directory for a call to
>> svn_io_open_unique_file3. That causes svn_io_open_unique_file3 to
>> set the permissions on the file according to umask.
>
> This sounds like a left over of the .svn/tmp directories in the old
> per directory working copies.
>
> It should probably use either the right .svn/tmp directory below the
> working copy root or the system temporary folder. But as a subst
> function it can't get the working copy information.
We want to limit use of the system temp dir for various reasons:
- the system dir may be a small filesystem with much less space than
the working copy filesystem
- if Subversion is interrupted files left in the system dir can't
easily be removed later by Subversion
A write operation, such as update, can simply use .svn/tmp and avoid
these problems but we want read operations, such as diff and status, to
work on read-only working copies. Perhaps read operations should use
.svn/tmp if possible and fall back on the system dir if .svn/tmp is not
writeable.
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*
RE: Fwd: [PATCH] Fix for `svn patch` changing permissions of patched files
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Philip Martin [mailto:philip.martin@wandisco.com]
> Sent: vrijdag 22 november 2013 14:31
> To: Branko Čibej
> Cc: dev@subversion.apache.org
> Subject: Re: Fwd: [PATCH] Fix for `svn patch` changing permissions of
> patched files
>
> Branko Čibej <br...@wandisco.com> writes:
>
> >
> > We're obviously making a lot of assumptions about which permissions are
> > correct. When we replace a file in the working copy, we should copy the
> > file's permissions, not invent new ones based on either the source
> > (which may be an arbitrary temporary file) or the umask.
> >
> > Of course, this is moderately easy on Unix (ignoring filesystems that
> > support ACLs) and quite a major pain on Windows.
>
> Yes, I think the aim of preserving the permissions when replacing files
> is good. We also want to avoid changing the permissions of files in the
> system temporary dir, those should remain 600. Running 'svn diff' on a
> working copy that has a file that requires translation reveals two
> different bugs that cause files in the system temp dir to have their
> permissions changed:
>
> - svn_subst_copy_and_translate4 uses the parent directory of a
> temporary file as the temporary directory for a call to
> svn_io_open_unique_file3. That causes svn_io_open_unique_file3 to
> set the permissions on the file according to umask.
This sounds like a left over of the .svn/tmp directories in the old per directory working copies.
It should probably use either the right .svn/tmp directory below the working copy root or the system temporary folder. But as a subst function it can't get the working copy information.
> - svn_subst_copy_and_translate4 copies the permissions from the
> working file to a file in the system temp dir.
Quite possible an artifact of converting this code, and adding permission changes in some code paths. (Originally we found that we created too readable files in global temp...) Only in 1.6/1.7 we started really using the global temp directory.
Branko: On Windows we never alter permissions. We let the inherited folder permission handle these things. A really nice case where the ACL's make our code easier, by not letting user code worry about this.
(There is also a per user temporary directory instead of one for all users)
Bert
Re: Fwd: [PATCH] Fix for `svn patch` changing permissions of patched files
Posted by Philip Martin <ph...@wandisco.com>.
Branko Čibej <br...@wandisco.com> writes:
>
> We're obviously making a lot of assumptions about which permissions are
> correct. When we replace a file in the working copy, we should copy the
> file's permissions, not invent new ones based on either the source
> (which may be an arbitrary temporary file) or the umask.
>
> Of course, this is moderately easy on Unix (ignoring filesystems that
> support ACLs) and quite a major pain on Windows.
Yes, I think the aim of preserving the permissions when replacing files
is good. We also want to avoid changing the permissions of files in the
system temporary dir, those should remain 600. Running 'svn diff' on a
working copy that has a file that requires translation reveals two
different bugs that cause files in the system temp dir to have their
permissions changed:
- svn_subst_copy_and_translate4 uses the parent directory of a
temporary file as the temporary directory for a call to
svn_io_open_unique_file3. That causes svn_io_open_unique_file3 to
set the permissions on the file according to umask.
- svn_subst_copy_and_translate4 copies the permissions from the
working file to a file in the system temp dir.
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*
Re: Fwd: [PATCH] Fix for `svn patch` changing permissions of patched
files
Posted by Branko Čibej <br...@wandisco.com>.
On 22.11.2013 12:45, Philip Martin wrote:
> Cathy Fitzpatrick <ca...@cathyjf.com> writes:
>
>> A close look at `svn_subst_copy_and_translate4` confirms that my guess
>> was correct. If the file does not have an EOL property, then
>> `svn_subst_copy_and_translate4` behaves as you say. If, however, it
>> does have an EOL property, then the last thing that
>> `svn_subst_copy_and_translate4` does is copy the file permissions from
>> the file under /tmp to the final destination file (line 1818 of
>> libsvn_subr/subst.c in r1544295). You are correct that it is the file
>> with the correct permissions that is copied to the final destination,
>> but then `svn_subst_copy_and_translate4` overwrites those permissions
>> (if and only if the file has an EOL property). So that's why we were
>> seeing different things.
> That's ugly. I think svn_subst_copy_and_translate4 should have the same
> effect on permissions irrespective of keywords, eol, etc. I'm not sure
> which of the two behaviours is correct.
We're obviously making a lot of assumptions about which permissions are
correct. When we replace a file in the working copy, we should copy the
file's permissions, not invent new ones based on either the source
(which may be an arbitrary temporary file) or the umask.
Of course, this is moderately easy on Unix (ignoring filesystems that
support ACLs) and quite a major pain on Windows.
-- Brane
--
Branko Čibej | Director of Subversion
WANdisco // Non-Stop Data
e. brane@wandisco.com
Re: Fwd: [PATCH] Fix for `svn patch` changing permissions of patched files
Posted by Philip Martin <ph...@wandisco.com>.
Cathy Fitzpatrick <ca...@cathyjf.com> writes:
> A close look at `svn_subst_copy_and_translate4` confirms that my guess
> was correct. If the file does not have an EOL property, then
> `svn_subst_copy_and_translate4` behaves as you say. If, however, it
> does have an EOL property, then the last thing that
> `svn_subst_copy_and_translate4` does is copy the file permissions from
> the file under /tmp to the final destination file (line 1818 of
> libsvn_subr/subst.c in r1544295). You are correct that it is the file
> with the correct permissions that is copied to the final destination,
> but then `svn_subst_copy_and_translate4` overwrites those permissions
> (if and only if the file has an EOL property). So that's why we were
> seeing different things.
That's ugly. I think svn_subst_copy_and_translate4 should have the same
effect on permissions irrespective of keywords, eol, etc. I'm not sure
which of the two behaviours is correct.
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*
Re: Fwd: [PATCH] Fix for `svn patch` changing permissions of patched files
Posted by Cathy Fitzpatrick <ca...@cathyjf.com>.
Hi again:
A close look at `svn_subst_copy_and_translate4` confirms that my guess
was correct. If the file does not have an EOL property, then
`svn_subst_copy_and_translate4` behaves as you say. If, however, it
does have an EOL property, then the last thing that
`svn_subst_copy_and_translate4` does is copy the file permissions from
the file under /tmp to the final destination file (line 1818 of
libsvn_subr/subst.c in r1544295). You are correct that it is the file
with the correct permissions that is copied to the final destination,
but then `svn_subst_copy_and_translate4` overwrites those permissions
(if and only if the file has an EOL property). So that's why we were
seeing different things.
See also my example set of commands in one of my other emails.
Thanks,
Cathy
On Fri, Nov 22, 2013 at 4:27 AM, Philip Martin
<ph...@wandisco.com> wrote:
> Cathy Fitzpatrick <ca...@cathyjf.com> writes:
>
>> As for why it's different on different machines -- it probably depends
>> on some details in APR. As I mentioned several times, the way the `svn
>> patch` code works is that it creates a file under /tmp and then copies
>> it to the final destination. The reason that gives 600 permissions on
>> the machines I've tried is that `apr_file_mktemp` creates files under
>> /tmp with 600 permissions.
>
> A new file is initially created in the system temp dir with permissions
> 600. However libsvn_client/patch.c:install_patched_target calls
> svn_subst_copy_and_translate4 and that creates a second temporary file
> in the working copy with permissions that respect umask. The content of
> the 600 file is copied into the umask file and it is the umask file that
> gets renamed to the working file. Why are you seeing something
> different?
>
> --
> Philip Martin | Subversion Committer
> WANdisco // *Non-Stop Data*
Re: Fwd: [PATCH] Fix for `svn patch` changing permissions of patched files
Posted by Philip Martin <ph...@wandisco.com>.
Cathy Fitzpatrick <ca...@cathyjf.com> writes:
> As for why it's different on different machines -- it probably depends
> on some details in APR. As I mentioned several times, the way the `svn
> patch` code works is that it creates a file under /tmp and then copies
> it to the final destination. The reason that gives 600 permissions on
> the machines I've tried is that `apr_file_mktemp` creates files under
> /tmp with 600 permissions.
A new file is initially created in the system temp dir with permissions
600. However libsvn_client/patch.c:install_patched_target calls
svn_subst_copy_and_translate4 and that creates a second temporary file
in the working copy with permissions that respect umask. The content of
the 600 file is copied into the umask file and it is the umask file that
gets renamed to the working file. Why are you seeing something
different?
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*
Re: Fwd: [PATCH] Fix for `svn patch` changing permissions of patched files
Posted by Julian Foad <ju...@btopenworld.com>.
Cathy Fitzpatrick wrote:
> Hi again Philip,
>
> Sorry, I was a bit too hasty in saying this was machine dependent.
> Your test case does respect the umask on all of the machines I am
> trying it on, but
[...]
> [Cathy@localhost subversion]$ svn revert INSTALL
> Reverted 'INSTALL'
> [Cathy@localhost subversion]$ ls -l INSTALL
> -rw-rw-r--. 1 Cathy Cathy 63057 Nov 22 03:29 INSTALL
> [Cathy@localhost subversion]$ umask 070
> [Cathy@localhost subversion]$ svn patch patch
> U INSTALL
> [Cathy@localhost subversion]$ ls -ls INSTALL
> 68 -rw-------. 1 Cathy Cathy 63062 Nov 22 03:29 INSTALL
Hi Cathy. I can confirm Philip's recipe behaves as he showed and yours behaves as you showed, also on my system (Ubuntu 12.04, svn trunk).
With your patch and your recipe above I get this result:
$ svn revert INSTALL && ls -l INSTALL && (umask 070 && svn patch patch) && ls -l INSTALL
Reverted 'INSTALL'
-rw-rw-r-- 1 julianfoad julianfoad 63057 Nov 22 10:49 INSTALL
U INSTALL
-rw----rw- 1 julianfoad julianfoad 63062 Nov 22 10:49 INSTALL
Is that what you get?
> This is just a guess to be verified later: it may be that the bug only
> occurs if a file has an EOL property specified.
That result is the same as in Philip's recipe (without keyword/eol-style). So that seems to me like a good fix.
I was a bit surprised at first because it *adds* world-writable permission, but maybe that's just fine given we specified a umask that allows this. At first I was expecting that it should never add permissions, but only take away any umasked-permissions and preserve the rest. But that's probably a question for another day.
One other thing. In your patch you use "svn_dirent_dirname(wcroot_abspath,)" for the temp-file directory. This puts the temp file *outside* the WC. I haven't checked what we do in other places but using 'wcroot_abspath' iteself would be less likely to fail, and probably even better is the directory where the to-be-patched filed itself is.
- Julian
[Philip's recipe results:]
>>> $ ls -l wc/f
>>> -rw-r--r-- 1 pm pm 2 Nov 22 10:03 wc/f
>>> $ (umask 070 && svn patch p wc)
>>> U wc/f
>>> $ ls -l wc/f
>>> -rw----rw- 1 pm pm 5 Nov 22 10:00 wc/f
>>>
>>> I get that behaviour with 1.7, 1.8 and trunk.
Re: Fwd: [PATCH] Fix for `svn patch` changing permissions of patched files
Posted by Cathy Fitzpatrick <ca...@cathyjf.com>.
Hi again Philip,
Sorry, I was a bit too hasty in saying this was machine dependent.
Your test case does respect the umask on all of the machines I am
trying it on, but on real life repositories the bug occurs as I
described (which is how I was reproducing it).
Here is a sequence of commands that should reproduce the bug for you:
[Cathy@localhost scratch]$ svn co
http://svn.apache.org/repos/asf/subversion/trunk subversion >
/dev/null
[Cathy@localhost scratch]$ cd subversion/
[Cathy@localhost subversion]$ echo blah >> INSTALL
[Cathy@localhost subversion]$ svn diff > patch
[Cathy@localhost subversion]$ svn revert INSTALL
Reverted 'INSTALL'
[Cathy@localhost subversion]$ ls -l INSTALL
-rw-rw-r--. 1 Cathy Cathy 63057 Nov 22 03:29 INSTALL
[Cathy@localhost subversion]$ umask 070
[Cathy@localhost subversion]$ svn patch patch
U INSTALL
[Cathy@localhost subversion]$ ls -ls INSTALL
68 -rw-------. 1 Cathy Cathy 63062 Nov 22 03:29 INSTALL
This is just a guess to be verified later: it may be that the bug only
occurs if a file has an EOL property specified.
Thanks,
Cathy
On Fri, Nov 22, 2013 at 3:12 AM, Cathy Fitzpatrick <ca...@cathyjf.com> wrote:
> Hi Philip:
>
> I get the behaviour I described on every version of subversion I've
> ever tried that has `svn patch`, on various different machines
> completely unrelated to each other.
>
> The patch I submitted was against trunk, which I confirmed to have the
> bug in my local testing (obviously; I wouldn't submit a patch to fix
> something that wasn't happening). I also just reproduced the bug on a
> completely unrelated installation of 1.8.0 on a different machine.
>
> As for why it's different on different machines -- it probably depends
> on some details in APR. As I mentioned several times, the way the `svn
> patch` code works is that it creates a file under /tmp and then copies
> it to the final destination. The reason that gives 600 permissions on
> the machines I've tried is that `apr_file_mktemp` creates files under
> /tmp with 600 permissions.
>
> There are several possible explanations of why it might be different
> on different machines, it's not something I have investigated because
> I did not expect it to be different on different machines.
>
> Regards,
>
> Cathy
>
> On Fri, Nov 22, 2013 at 3:06 AM, Philip Martin
> <ph...@wandisco.com> wrote:
>> Cathy Fitzpatrick <ca...@cathyjf.com> writes:
>>
>>> 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.
>>
>> I see patch setting permissions according to umask. Why are you seeing
>> something different? Which version of Subversion are you using?
>>
>> svnadmin create repo
>> svn import -mm repo/format file://`pwd`/repo/f
>> svn co file://`pwd`/repo wc
>> echo xx >> wc/f
>> (cd wc && svn diff) > p
>> svn revert -R wc
>>
>> Now
>>
>> $ ls -l wc/f
>> -rw-r--r-- 1 pm pm 2 Nov 22 10:03 wc/f
>> $ (umask 070 && svn patch p wc)
>> U wc/f
>> $ ls -l wc/f
>> -rw----rw- 1 pm pm 5 Nov 22 10:00 wc/f
>>
>> I get that behaviour with 1.7, 1.8 and trunk.
>>
>> --
>> Philip Martin | Subversion Committer
>> WANdisco // *Non-Stop Data*
Re: Fwd: [PATCH] Fix for `svn patch` changing permissions of patched files
Posted by Cathy Fitzpatrick <ca...@cathyjf.com>.
Hi Philip:
I get the behaviour I described on every version of subversion I've
ever tried that has `svn patch`, on various different machines
completely unrelated to each other.
The patch I submitted was against trunk, which I confirmed to have the
bug in my local testing (obviously; I wouldn't submit a patch to fix
something that wasn't happening). I also just reproduced the bug on a
completely unrelated installation of 1.8.0 on a different machine.
As for why it's different on different machines -- it probably depends
on some details in APR. As I mentioned several times, the way the `svn
patch` code works is that it creates a file under /tmp and then copies
it to the final destination. The reason that gives 600 permissions on
the machines I've tried is that `apr_file_mktemp` creates files under
/tmp with 600 permissions.
There are several possible explanations of why it might be different
on different machines, it's not something I have investigated because
I did not expect it to be different on different machines.
Regards,
Cathy
On Fri, Nov 22, 2013 at 3:06 AM, Philip Martin
<ph...@wandisco.com> wrote:
> Cathy Fitzpatrick <ca...@cathyjf.com> writes:
>
>> 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.
>
> I see patch setting permissions according to umask. Why are you seeing
> something different? Which version of Subversion are you using?
>
> svnadmin create repo
> svn import -mm repo/format file://`pwd`/repo/f
> svn co file://`pwd`/repo wc
> echo xx >> wc/f
> (cd wc && svn diff) > p
> svn revert -R wc
>
> Now
>
> $ ls -l wc/f
> -rw-r--r-- 1 pm pm 2 Nov 22 10:03 wc/f
> $ (umask 070 && svn patch p wc)
> U wc/f
> $ ls -l wc/f
> -rw----rw- 1 pm pm 5 Nov 22 10:00 wc/f
>
> I get that behaviour with 1.7, 1.8 and trunk.
>
> --
> Philip Martin | Subversion Committer
> WANdisco // *Non-Stop Data*
Re: Fwd: [PATCH] Fix for `svn patch` changing permissions of patched files
Posted by Philip Martin <ph...@wandisco.com>.
Cathy Fitzpatrick <ca...@cathyjf.com> writes:
> 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.
I see patch setting permissions according to umask. Why are you seeing
something different? Which version of Subversion are you using?
svnadmin create repo
svn import -mm repo/format file://`pwd`/repo/f
svn co file://`pwd`/repo wc
echo xx >> wc/f
(cd wc && svn diff) > p
svn revert -R wc
Now
$ ls -l wc/f
-rw-r--r-- 1 pm pm 2 Nov 22 10:03 wc/f
$ (umask 070 && svn patch p wc)
U wc/f
$ ls -l wc/f
-rw----rw- 1 pm pm 5 Nov 22 10:00 wc/f
I get that behaviour with 1.7, 1.8 and trunk.
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*
Fwd: [PATCH] Fix for `svn patch` changing permissions of patched files
Posted by Cathy Fitzpatrick <ca...@cathyjf.com>.
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 <ph...@wandisco.com>
On Thu, Nov 21, 2013 at 4:19 AM, Philip Martin
<ph...@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
Re: [PATCH] Fix for `svn patch` changing permissions of patched files
Posted by Philip Martin <ph...@wandisco.com>.
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*
Re: [PATCH] Fix for `svn patch` changing permissions of patched files
Posted by Cathy Fitzpatrick <ca...@cathyjf.com>.
Hi,
There was a potentially confusing typo in the comments in my patch, so
I've attached a version with the typo fixed. Note that the code itself
was fine, the typo was just in the comments.
Cheers,
Cathy
On Wed, Nov 20, 2013 at 6:07 PM, Cathy Fitzpatrick <ca...@cathyjf.com> wrote:
> Hello,
>
> 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.
>
> Here is an example of the current behaviour (before my patch):
>
> [Cathy@localhost subversion]$ ls -la INSTALL
> -rw-rw-r--. 1 Cathy Cathy 63057 Nov 20 17:34 INSTALL
> [Cathy@localhost subversion]$ ./svn patch patch.diff
> U INSTALL
> [Cathy@localhost subversion]$ ls -la INSTALL
> -rw-------. 1 Cathy Cathy 63070 Nov 20 17:58 INSTALL
>
> Notice that applying the patch has changed the permissions of INSTALL.
>
> After my patch, we instead get the following:
>
> [Cathy@localhost subversion]$ ls -la INSTALL
> -rw-rw-r--. 1 Cathy Cathy 63057 Nov 20 17:59 INSTALL
> [Cathy@localhost subversion]$ ./svn patch patch.diff
> U INSTALL
> [Cathy@localhost subversion]$ ls -la INSTALL
> -rw-rw-r--. 1 Cathy Cathy 63070 Nov 20 17:59 INSTALL
>
> As expected, patching the file no longer alters its permissions.
>
>
> I have attached my patch to this email and it includes a log message.
>
> Regards,
>
> Cathy