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

Reply via email to