Re: Make 'svn patch' keep permissions of patched files
Ruediger Pluem wrote on Mon, 21 Feb 2022 12:23 +00:00: > On 2/21/22 11:54 AM, Daniel Shahaf wrote: >> Ruediger Pluem wrote on Mon, Feb 21, 2022 at 08:49:08 +0100: >> Even if we don't consense on what the behaviour should be, we could at >> least add a test that expects the current behaviour, in order to ensure >> we don't change it unintentionally. > > +1 Would you like to contribute such a test, Ruediger? It'd go in subversion/tests/cmdline/. We can have it SkipUnless is_posix_os() so it doesn't affect the Windows build. Cheers, Daniel
Re: Make 'svn patch' keep permissions of patched files
On 2/21/22 11:54 AM, Daniel Shahaf wrote: > Ruediger Pluem wrote on Mon, Feb 21, 2022 at 08:49:08 +0100: >> On 2/19/22 5:07 PM, Daniel Shahaf wrote: >>> I can't reproduce this. Using unpatched trunk: >> >> I checked again and I was able to reproduce it with 1.10. >> With 1.13 though it shows the behavior you describe for trunk below. > ⋮ >>> How do other patch tools behave in this case? >> >> I tried GNU patch 2.7.6 and it kept the permissions of the file to be >> patched. No matter what umask was set. >> git 2.27.0 on the other side behaves like svn trunk: The file permissons are >> affected by the umask. > > And «svn update» creates the new file with umask permissions too. E.g., > «chmod 0644 README; umask 077; svn up -r PREV README; stat README» gives > 0600 (with current trunk). I can confirm this behavior even with 1.10. > > I'm not sure what behaviour I would expect. On the one hand, umask only > kicks in because of an implementation detail of atomic renames; there's > no business logic reason to use the "default permissions for new files" > value when changing an existing file. On the other hand, there's an > argument that «svn patch» and «svn up» should be consistent with each > other, and changing the latter is more likely to break people's > proverbial spacebar heating. These are good points. Another possible option which is not covered by the proposed patch would be to offer options to svn patch and svn up that the permissions of existing files should be kept (possibly also or instead an option for .subversion/config). This would ensure that the behavior of svn up and svn patch is consistent by default and remains the same as currently but offers people who need to keep the permissions an option. If this looks like a way forward I would at least have a look into it. No promise yet that I can provide a patch though :-) > > Even if we don't consense on what the behaviour should be, we could at > least add a test that expects the current behaviour, in order to ensure > we don't change it unintentionally. +1 Regards Rüdiger
Re: Make 'svn patch' keep permissions of patched files
Ruediger Pluem wrote on Mon, Feb 21, 2022 at 08:49:08 +0100: > On 2/19/22 5:07 PM, Daniel Shahaf wrote: > > I can't reproduce this. Using unpatched trunk: > > I checked again and I was able to reproduce it with 1.10. > With 1.13 though it shows the behavior you describe for trunk below. ⋮ > > How do other patch tools behave in this case? > > I tried GNU patch 2.7.6 and it kept the permissions of the file to be > patched. No matter what umask was set. > git 2.27.0 on the other side behaves like svn trunk: The file permissons are > affected by the umask. And «svn update» creates the new file with umask permissions too. E.g., «chmod 0644 README; umask 077; svn up -r PREV README; stat README» gives 0600 (with current trunk). I'm not sure what behaviour I would expect. On the one hand, umask only kicks in because of an implementation detail of atomic renames; there's no business logic reason to use the "default permissions for new files" value when changing an existing file. On the other hand, there's an argument that «svn patch» and «svn up» should be consistent with each other, and changing the latter is more likely to break people's proverbial spacebar heating. Even if we don't consense on what the behaviour should be, we could at least add a test that expects the current behaviour, in order to ensure we don't change it unintentionally. Cheers, Daniel
Re: Make 'svn patch' keep permissions of patched files
On 2/19/22 5:07 PM, Daniel Shahaf wrote:
> Stefan Sperling wrote on Thu, Feb 10, 2022 at 09:29:12 +0100:
>> On Thu, Feb 10, 2022 at 12:10:08AM -0600, Karl Fogel wrote:
>>> On 09 Feb 2022, Ruediger Pluem wrote:
When rebuilding my own Subversion build I stumbled across the following
patch that I add to my build:
Index: subversion/libsvn_client/patch.c
===
--- subversion/libsvn_client/patch.c(revision 1897905)
+++ subversion/libsvn_client/patch.c(working copy)
@@ -3246,6 +3246,15 @@ install_patched_target(patch_target_t *target,
con
target->content->eol_style ==
svn_subst_eol_style_native);
+ /* Make sure the patched file has the same permissions the
+ * original file, but only if it does not get added.
+ */
+ if (!target->added)
+{
+ SVN_ERR(svn_io_copy_perms(
+target->local_abspath, target->patched_path,
pool));
+}
+
SVN_ERR(svn_subst_copy_and_translate4(
target->patched_path,
target->move_target_abspath
It ensures that files that get modified (not added) during svn patch
keep their permissions. Can this be added to trunk?
>>>
>>> I'm curious: what is the case that causes the patched target file to have
>>> different permissions than it had before the patch? (Or am I
>>> misunderstanding what you're saying?)
>>
>> Possibly related to the fact that the patched target is prepared inside
>> a temporary file, which has 0600 permissions by default since it gets
>> created by mkstemp(3) (see https://svn.apache.org/r880338).
>>
>> This temp file is then renamed on top of the file in the working copy and
>> file mode bits will be copied 1:1. We have had similar reports before, where
>> files ended up being readable only by the user who was using the working
>> copy, and where this interfered with some other use of such files.
>
> I can't reproduce this. Using unpatched trunk:
I checked again and I was able to reproduce it with 1.10.
With 1.13 though it shows the behavior you describe for trunk below.
>
> % echo >> iota
> % svn diff iota > diff
> % svn revert -q -R ./
> % zstat -or +mode iota
> 0100644 (-rw-r--r--)
> % svn patch diff
> U iota
> % zstat -or +mode iota
> 0100644 (-rw-r--r--)
> % svn st -q
> M iota
> %
>
> What I do see, however, is that the patched file's permissions are
> affected by the umask. I.e., if I change the umask before calling «svn
> patch», then iota's permissions are the permissions of a new file under
> the updated umask.
>
> How do other patch tools behave in this case?
I tried GNU patch 2.7.6 and it kept the permissions of the file to be patched.
No matter what umask was set.
git 2.27.0 on the other side behaves like svn trunk: The file permissons are
affected by the umask.
Regards
Rüdiger
Re: Make 'svn patch' keep permissions of patched files
Stefan Sperling wrote on Thu, Feb 10, 2022 at 09:29:12 +0100:
> On Thu, Feb 10, 2022 at 12:10:08AM -0600, Karl Fogel wrote:
> > On 09 Feb 2022, Ruediger Pluem wrote:
> > > When rebuilding my own Subversion build I stumbled across the following
> > > patch that I add to my build:
> > >
> > > Index: subversion/libsvn_client/patch.c
> > > ===
> > > --- subversion/libsvn_client/patch.c(revision 1897905)
> > > +++ subversion/libsvn_client/patch.c(working copy)
> > > @@ -3246,6 +3246,15 @@ install_patched_target(patch_target_t *target,
> > > con
> > > target->content->eol_style ==
> > > svn_subst_eol_style_native);
> > >
> > > + /* Make sure the patched file has the same permissions the
> > > + * original file, but only if it does not get added.
> > > + */
> > > + if (!target->added)
> > > +{
> > > + SVN_ERR(svn_io_copy_perms(
> > > +target->local_abspath, target->patched_path,
> > > pool));
> > > +}
> > > +
> > > SVN_ERR(svn_subst_copy_and_translate4(
> > > target->patched_path,
> > > target->move_target_abspath
> > >
> > > It ensures that files that get modified (not added) during svn patch
> > > keep their permissions. Can this be added to trunk?
> >
> > I'm curious: what is the case that causes the patched target file to have
> > different permissions than it had before the patch? (Or am I
> > misunderstanding what you're saying?)
>
> Possibly related to the fact that the patched target is prepared inside
> a temporary file, which has 0600 permissions by default since it gets
> created by mkstemp(3) (see https://svn.apache.org/r880338).
>
> This temp file is then renamed on top of the file in the working copy and
> file mode bits will be copied 1:1. We have had similar reports before, where
> files ended up being readable only by the user who was using the working
> copy, and where this interfered with some other use of such files.
I can't reproduce this. Using unpatched trunk:
% echo >> iota
% svn diff iota > diff
% svn revert -q -R ./
% zstat -or +mode iota
0100644 (-rw-r--r--)
% svn patch diff
U iota
% zstat -or +mode iota
0100644 (-rw-r--r--)
% svn st -q
M iota
%
What I do see, however, is that the patched file's permissions are
affected by the umask. I.e., if I change the umask before calling «svn
patch», then iota's permissions are the permissions of a new file under
the updated umask.
How do other patch tools behave in this case?
Cheers,
Daniel
> > If you feel like writing a regression test for this case, then I could
> > probably answer my question by looking at the test's code (but I realize you
> > might not have time to do that).
>
> I agree that having a test case to cover this behaviour would be good.
> A quick glance over subversion/tests/cmdline/patch.py suggests that
> this behaviour is not covered by our tests yet? That would be unfortunate.
>
> Ruediger, you may not be aware that our /subversion subtree of the ASF
> SVN repository is open for all ASF committers. You should already have
> write access. So once we are done discussing this change, you will be
> able to add this change to trunk yourself.
>
> Cheers,
> Stefan
Re: Make 'svn patch' keep permissions of patched files
On 10 Feb 2022, Stefan Sperling wrote: On Thu, Feb 10, 2022 at 12:10:08AM -0600, Karl Fogel wrote: I'm curious: what is the case that causes the patched target file to have different permissions than it had before the patch? (Or am I misunderstanding what you're saying?) Possibly related to the fact that the patched target is prepared inside a temporary file, which has 0600 permissions by default since it gets created by mkstemp(3) (see https://svn.apache.org/r880338). This temp file is then renamed on top of the file in the working copy and file mode bits will be copied 1:1. We have had similar reports before, where files ended up being readable only by the user who was using the working copy, and where this interfered with some other use of such files. Ah, thank you, Stefan. That was the key point I didn't know (until now). Best regards, -Karl
Re: Make 'svn patch' keep permissions of patched files
On Thu, Feb 10, 2022 at 12:10:08AM -0600, Karl Fogel wrote:
> On 09 Feb 2022, Ruediger Pluem wrote:
> > When rebuilding my own Subversion build I stumbled across the following
> > patch that I add to my build:
> >
> > Index: subversion/libsvn_client/patch.c
> > ===
> > --- subversion/libsvn_client/patch.c(revision 1897905)
> > +++ subversion/libsvn_client/patch.c(working copy)
> > @@ -3246,6 +3246,15 @@ install_patched_target(patch_target_t *target,
> > con
> > target->content->eol_style ==
> > svn_subst_eol_style_native);
> >
> > + /* Make sure the patched file has the same permissions
> > the
> > + * original file, but only if it does not get added.
> > + */
> > + if (!target->added)
> > +{
> > + SVN_ERR(svn_io_copy_perms(
> > +target->local_abspath,
> > target->patched_path, pool));
> > +}
> > +
> > SVN_ERR(svn_subst_copy_and_translate4(
> > target->patched_path,
> > target->move_target_abspath
> >
> > It ensures that files that get modified (not added) during svn patch
> > keep their permissions. Can this be added to trunk?
>
> I'm curious: what is the case that causes the patched target file to have
> different permissions than it had before the patch? (Or am I
> misunderstanding what you're saying?)
Possibly related to the fact that the patched target is prepared inside
a temporary file, which has 0600 permissions by default since it gets
created by mkstemp(3) (see https://svn.apache.org/r880338).
This temp file is then renamed on top of the file in the working copy and
file mode bits will be copied 1:1. We have had similar reports before, where
files ended up being readable only by the user who was using the working
copy, and where this interfered with some other use of such files.
> If you feel like writing a regression test for this case, then I could
> probably answer my question by looking at the test's code (but I realize you
> might not have time to do that).
I agree that having a test case to cover this behaviour would be good.
A quick glance over subversion/tests/cmdline/patch.py suggests that
this behaviour is not covered by our tests yet? That would be unfortunate.
Ruediger, you may not be aware that our /subversion subtree of the ASF
SVN repository is open for all ASF committers. You should already have
write access. So once we are done discussing this change, you will be
able to add this change to trunk yourself.
Cheers,
Stefan
Re: Make 'svn patch' keep permissions of patched files
On 09 Feb 2022, Ruediger Pluem wrote:
When rebuilding my own Subversion build I stumbled across the
following
patch that I add to my build:
Index: subversion/libsvn_client/patch.c
===
--- subversion/libsvn_client/patch.c(revision 1897905)
+++ subversion/libsvn_client/patch.c(working copy)
@@ -3246,6 +3246,15 @@ install_patched_target(patch_target_t
*target, con
target->content->eol_style ==
svn_subst_eol_style_native);
+ /* Make sure the patched file has the same
permissions the
+ * original file, but only if it does not get
added.
+ */
+ if (!target->added)
+{
+ SVN_ERR(svn_io_copy_perms(
+target->local_abspath,
target->patched_path, pool));
+}
+
SVN_ERR(svn_subst_copy_and_translate4(
target->patched_path,
target->move_target_abspath
It ensures that files that get modified (not added) during svn
patch
keep their permissions. Can this be added to trunk?
I'm curious: what is the case that causes the patched target file
to have different permissions than it had before the patch? (Or
am I misunderstanding what you're saying?)
If you feel like writing a regression test for this case, then I
could probably answer my question by looking at the test's code
(but I realize you might not have time to do that).
Best regards,
-Karl
Make 'svn patch' keep permissions of patched files
When rebuilding my own Subversion build I stumbled across the following patch
that I add to my build:
Index: subversion/libsvn_client/patch.c
===
--- subversion/libsvn_client/patch.c(revision 1897905)
+++ subversion/libsvn_client/patch.c(working copy)
@@ -3246,6 +3246,15 @@ install_patched_target(patch_target_t *target, con
target->content->eol_style ==
svn_subst_eol_style_native);
+ /* Make sure the patched file has the same permissions the
+ * original file, but only if it does not get added.
+ */
+ if (!target->added)
+{
+ SVN_ERR(svn_io_copy_perms(
+target->local_abspath, target->patched_path,
pool));
+}
+
SVN_ERR(svn_subst_copy_and_translate4(
target->patched_path,
target->move_target_abspath
It ensures that files that get modified (not added) during svn patch keep their
permissions.
Can this be added to trunk?
Regards
Rüdiger
