bug#7289: Tramp changes ownership of remote file when saving

bug#7289: Tramp changes ownership of remote file when saving

[Jo Vermeulen]

Package: emacs
Version: 23.1
Severity: important

When editing a remote file over SSH using tramp, emacs changes the ownership
of the file. I was logged in as root, and the file had ownership svn:svn.
After saving, however, emacs changed the ownership to root:root. The backup
file (filename~) did have the user/group settings however.

This behavior is quite annoying, as one often doesn't realize the ownership
has changed. This might result in services that are unable to read their
basic configuration files after they have been edited remotely with emacs.

P.S.: my apologies for double-posting, I first thought the package name
should be tramp instead of emacs.

Here's more information about my specific version of Emacs (running on
Ubuntu 10.10):

In GNU Emacs 23.1.1 (i486-pc-linux-gnu, GTK+ Version 2.20.0)
 of 2010-03-29 on rothera, modified by Debian
Windowing system distributor `The X.Org Foundation', version 11.0.10900000
configured using `configure  '--build=i486-linux-gnu'
'--host=i486-linux-gnu' '--prefix=/usr' '--sharedstatedir=/var/lib'
'--libexecdir=/usr/lib' '--localstatedir=/var/lib'
'--infodir=/usr/share/info' '--mandir=/usr/share/man' '--with-pop=yes'
'--enable-locallisppath=/etc/emacs23:/etc/emacs:/usr/local/share/emacs/23.1/site-lisp:/usr/local/share/emacs/site-lisp:/usr/share/emacs/23.1/site-lisp:/usr/share/emacs/site-lisp:/usr/share/emacs/23.1/leim'
'--with-x=yes' '--with-x-toolkit=gtk' '--with-toolkit-scroll-bars'
'build_alias=i486-linux-gnu' 'host_alias=i486-linux-gnu' 'CFLAGS=-DDEBIAN -g
-O2' 'LDFLAGS=-g' 'CPPFLAGS=''

Important settings:
  value of $LC_ALL: nil
  value of $LC_COLLATE: nil
  value of $LC_CTYPE: nil
  value of $LC_MESSAGES: nil
  value of $LC_MONETARY: nil
  value of $LC_NUMERIC: nil
  value of $LC_TIME: nil
  value of $LANG: en_US.utf8
  value of $XMODIFIERS: nil
  locale-coding-system: utf-8-unix
  default-enable-multibyte-characters: t

Major mode: Help

Minor modes in effect:
  tooltip-mode: t
  tool-bar-mode: t
  mouse-wheel-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  global-auto-composition-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t
  view-mode: t

Recent input:
C-x C-d <backspace> <backspace> / u s r <tab> s h <tab>
e m <tab> a <tab> / <tab> <tab> 2 3 <tab> e <tab> p
r <backspace> <S-backspace> P R <tab> <return> C-x
o C-x o C-x o C-x 1 <down> <return> <right> <return>
<return> <up> <down> M-x d i r e <tab> - <tab> m <tab>
<backspace> <tab> <backspace> o <tab> d <tab> <backspace>
<backspace> <backspace> <backspace> <backspace> <backspace>
<backspace> <backspace> <backspace> w d i r <tab> <backspace>
<backspace> <backspace> <backspace> <backspace> <backspace>
<backspace> <backspace> <backspace> <backspace> <backspace>
<backspace> <backspace> <backspace> <backspace> <backspace>
<backspace> <backspace> <backspace> <backspace> <backspace>
<backspace> <backspace> <backspace> <backspace> <backspace>
<backspace> <backspace> d i r e d - m <tab> <tab> <backspace>
<backspace> <backspace> <return> <return> <down> <down>
<down> <down> <down> <down> <down> <down> <down> <down>
<down> <down> <down> <down> <down> <down> <down> <down>
<down> <down> <down> <down> C-s P R O B L E M <return>
<return> <down> <down> <down> <down> <down> <down>
<down> <down> <down> <down> <down> <down> <down> <down>
<down> <down> <down> <up> <up> <up> <up> <up> <down>
<end> <down> <down> <down> <down> <down> <down> <down>
<down> <down> <down> <down> <down> C-s t r a m p C-s
<backspace> <backspace> <backspace> <backspace> <backspace>
<backspace> r e m o t e C-s C-s C-s s s <backspace>
<backspace> C-s C-s C-s C-s C-s C-s C-s C-s C-s C-s
C-s C-x k <return> C-x k <return> M-x r e p o r t -
e m a c s <tab> <return>

Recent messages:
Loading /etc/emacs/site-start.d/50org-mode.el (source)...done
Loading /etc/emacs/site-start.d/50psvn.el (source)...done
Loading /etc/emacs/site-start.d/50w3m-el.el (source)...done
For information about GNU Emacs and the GNU system, type C-h C-a.
Making completion list... [2 times]
help-follow: No cross-reference here [3 times]
Making completion list... [2 times]
Mark saved where search started
Note: file is write protected
Mark saved where search started

bug#7289: Tramp changes ownership of remote file when saving

[Stefan Monnier]

> When editing a remote file over SSH using tramp, emacs changes the ownership
> of the file. I was logged in as root, and the file had ownership svn:svn.
> After saving, however, emacs changed the ownership to root:root. The backup
> file (filename~) did have the user/group settings however.

What are your settings for backup-by-copying and
backup-by-copying-when-mismatch?


        Stefan

bug#7289: Tramp changes ownership of remote file when saving

[Jo Vermeulen]

On Wed, Oct 27, 2010 at 3:53 PM, Stefan Monnier
<monnier@iro.umontreal.ca> wrote:
>> When editing a remote file over SSH using tramp, emacs changes the ownership
>> of the file. I was logged in as root, and the file had ownership svn:svn.
>> After saving, however, emacs changed the ownership to root:root. The backup
>> file (filename~) did have the user/group settings however.
>
> What are your settings for backup-by-copying and
> backup-by-copying-when-mismatch?

C-h-v backup-by-coping gives me:

"backup-by-copying's value is nil"

Which, I assume, means that Emacs might use renaming for backups.

And C-h-v backup-by-coping-when-mismatch results in:

"backup-by-copying-when-mismatch's value is nil"

I guess if this value would be non-nil, my problem would be solved?

Additionally, C-h-v backup-by-copying-when-privileged-mismatch gives me:

"backup-by-copying-when-privileged-mismatch's value is 200"

I just toggled backup-by-copying-when-mismatch and tried it again, and
indeed, it does work now.

I'm just wondering if this means that the default behavior is broken?
It seems Emacs automatically sets backup-by-copying-when-mismatch to t
when it is run by the superuser (root) to overcome this problem
(source: http://www.delorie.com/gnu/docs/emacs/emacs_117.html).

I don't think many people will run Emacs as root in this day and age.
However, they might log in as root on a remote server, or edit a file
as root (using sudo/su). Maybe Emacs or Tramp should also
automatically set the backup-by-copying-when-mismatch variable to t in
those cases?

Thanks for your help!

Cheers,

-- Jo

bug#7289: Tramp changes ownership of remote file when saving

[Michael Albinus]

Jo Vermeulen <jo.vermeulen@gmail.com> writes:

> I don't think many people will run Emacs as root in this day and age.
> However, they might log in as root on a remote server, or edit a file
> as root (using sudo/su). Maybe Emacs or Tramp should also
> automatically set the backup-by-copying-when-mismatch variable to t in
> those cases?

Could you, please, try the following patch? It is towards the Emacs-23
branch, line numbers might differ with your tramp.el.

--8<---------------cut here---------------start------------->8---
*** /home/albinus/src/emacs-23/lisp/net/tramp.el.~100136~	2010-10-28 14:31:49.730285001 +0200
--- /home/albinus/src/emacs-23/lisp/net/tramp.el	2010-10-28 14:29:40.990285001 +0200
***************
*** 5008,5014 ****
  	    (setq buffer-file-name filename)
  	    (setq buffer-read-only (not (file-writable-p filename)))
  	    (set-visited-file-modtime)
! 	    (set-buffer-modified-p nil))
  	  (when (and (stringp local-copy)
  		     (or remote-copy (null tramp-temp-buffer-file-name)))
  	    (delete-file local-copy))
--- 5008,5018 ----
  	    (setq buffer-file-name filename)
  	    (setq buffer-read-only (not (file-writable-p filename)))
  	    (set-visited-file-modtime)
! 	    (set-buffer-modified-p nil)
! 	    ;; For root, preserve owner and group when editing files.
! 	    (when (string-equal (file-remote-p filename 'user) "root")
! 	      (set (make-local-variable 'backup-by-copying-when-mismatch) t)
! 	      (put 'backup-by-copying-when-mismatch 'permanent-local t)))
  	  (when (and (stringp local-copy)
  		     (or remote-copy (null tramp-temp-buffer-file-name)))
  	    (delete-file local-copy))
--8<---------------cut here---------------end--------------->8---

> Thanks for your help!
>
> Cheers,
>
> -- Jo

Best regards, Michael.

bug#7289: Tramp changes ownership of remote file when saving

[Jo Vermeulen]

On Thu, Oct 28, 2010 at 2:37 PM, Michael Albinus <michael.albinus@gmx.de> wrote:
> Jo Vermeulen <jo.vermeulen@gmail.com> writes:
>
>> I don't think many people will run Emacs as root in this day and age.
>> However, they might log in as root on a remote server, or edit a file
>> as root (using sudo/su). Maybe Emacs or Tramp should also
>> automatically set the backup-by-copying-when-mismatch variable to t in
>> those cases?
>
> Could you, please, try the following patch? It is towards the Emacs-23
> branch, line numbers might differ with your tramp.el.
>
> --8<---------------cut here---------------start------------->8---
> *** /home/albinus/src/emacs-23/lisp/net/tramp.el.~100136~       2010-10-28 14:31:49.730285001 +0200
> --- /home/albinus/src/emacs-23/lisp/net/tramp.el        2010-10-28 14:29:40.990285001 +0200
> ***************
> *** 5008,5014 ****
>            (setq buffer-file-name filename)
>            (setq buffer-read-only (not (file-writable-p filename)))
>            (set-visited-file-modtime)
> !           (set-buffer-modified-p nil))
>          (when (and (stringp local-copy)
>                     (or remote-copy (null tramp-temp-buffer-file-name)))
>            (delete-file local-copy))
> --- 5008,5018 ----
>            (setq buffer-file-name filename)
>            (setq buffer-read-only (not (file-writable-p filename)))
>            (set-visited-file-modtime)
> !           (set-buffer-modified-p nil)
> !           ;; For root, preserve owner and group when editing files.
> !           (when (string-equal (file-remote-p filename 'user) "root")
> !             (set (make-local-variable 'backup-by-copying-when-mismatch) t)
> !             (put 'backup-by-copying-when-mismatch 'permanent-local t)))
>          (when (and (stringp local-copy)
>                     (or remote-copy (null tramp-temp-buffer-file-name)))
>            (delete-file local-copy))
> --8<---------------cut here---------------end--------------->8---

To which function does this patch apply? Couldn't immediately find it,
but my code might be different from the current Emacs 23 branch ...

Thanks!

-- Jo

-- Jo

bug#7289: Tramp changes ownership of remote file when saving

[Michael Albinus]

Jo Vermeulen <jo.vermeulen@gmail.com> writes:

> To which function does this patch apply? Couldn't immediately find it,
> but my code might be different from the current Emacs 23 branch ...

tramp-handle-insert-file-contents

If you are using Emacs' trunk, it would be another file / another function.

> Thanks!
>
> -- Jo

Best regards, Michael.

bug#7289: Tramp changes ownership of remote file when saving

[Jo Vermeulen]

On Fri, Oct 29, 2010 at 1:25 PM, Michael Albinus <michael.albinus@gmx.de> wrote:
> Jo Vermeulen <jo.vermeulen@gmail.com> writes:
>
>> To which function does this patch apply? Couldn't immediately find it,
>> but my code might be different from the current Emacs 23 branch ...
>
> tramp-handle-insert-file-contents
>
> If you are using Emacs' trunk, it would be another file / another function.

I changed it, and it seems to work now. However, I get this error (in
*Messages*):

apply: Symbol's value as variable is void: local-copy

My tramp.el file can be found @
http://research.edm.uhasselt.be/~jvermeulen/tmp/emacs/tramp.el

Cheers,

-- Jo

bug#7289: Tramp changes ownership of remote file when saving

[Michael Albinus]

Jo Vermeulen <jo.vermeulen@gmail.com> writes:

> I changed it, and it seems to work now. However, I get this error (in
> *Messages*):
>
> apply: Symbol's value as variable is void: local-copy

I see. The lines

	  (when (and (stringp local-copy)
		     (or remote-copy (null tramp-temp-buffer-file-name)))
	    (delete-file local-copy))

were added later as with Emacs 23.1, you can discard them.

Thanks for testing, I'll commit the patch (and close the bug).

> Cheers,
>
> -- Jo

Best regards, Miochael.

bug#7289: Tramp changes ownership of remote file when saving

[Stefan Monnier]

> ! 	    ;; For root, preserve owner and group when editing files.
> ! 	    (when (string-equal (file-remote-p filename 'user) "root")
> ! 	      (set (make-local-variable 'backup-by-copying-when-mismatch) t)
> ! 	      (put 'backup-by-copying-when-mismatch 'permanent-local t)))

2 problems with this patch:
- (put <var> 'permanent-local t) should *never* be in such a function.
  It's a top-level operation that has global effects and belongs right
  next to the var's defvar.
- This makes it impossible to choose backup-by-copying-when-mismatch=nil
  when accessing such remote files, whereas for local files accessed by
  root a (setq backup-by-copying-when-mismatch nil) in the .emacs will
  do the trick.

A simpler solution would be to change the default value of
backup-by-copying-when-mismatch to t for everyone rather than just
for root.  I don't know what would the objections to this.


        Stefan

bug#7289: Tramp changes ownership of remote file when saving

[Michael Albinus]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> 2 problems with this patch:
> - (put <var> 'permanent-local t) should *never* be in such a function.
>   It's a top-level operation that has global effects and belongs right
>   next to the var's defvar.

I want to have `backup-by-copying-when-mismatch' buffer local. If I set
it in `insert-file-contents' (via `make-local-variable'), it is not kept
buffer local in that buffer. How could I achieve this otherwise?

> - This makes it impossible to choose backup-by-copying-when-mismatch=nil
>   when accessing such remote files, whereas for local files accessed by
>   root a (setq backup-by-copying-when-mismatch nil) in the .emacs will
>   do the trick.

D'accord. We would need another way for the user to express his
wish. Maybe there shall be also other values but only than nil and t.

> A simpler solution would be to change the default value of
> backup-by-copying-when-mismatch to t for everyone rather than just
> for root.  I don't know what would the objections to this.

From my pov it would be acceptable, but I don't know how people see
it. And we should check what it does mean for remote files, because they
tend to "mismatch" more often.

>         Stefan

Best regards, Michael.

backup-by-copying-when-mismatch (was: bug#7289: Tramp changes ownership of remote file when saving)

[Stefan Monnier]

[ Moving this to emacs-devel. ]

>> 2 problems with this patch:
>> - (put <var> 'permanent-local t) should *never* be in such a function.
>> It's a top-level operation that has global effects and belongs right
>> next to the var's defvar.

> I want to have `backup-by-copying-when-mismatch' buffer local. If I set
> it in `insert-file-contents' (via `make-local-variable'), it is not kept
> buffer local in that buffer.  How could I achieve this otherwise?

You could place the `put' in files.el at toplevel, right next to the
definition of backup-by-copying-when-mismatch.

>> - This makes it impossible to choose backup-by-copying-when-mismatch=nil
>> when accessing such remote files, whereas for local files accessed by
>> root a (setq backup-by-copying-when-mismatch nil) in the .emacs will
>> do the trick.
> D'accord.  We would need another way for the user to express his
> wish.  Maybe there shall be also other values but only than nil and t.

That would be an option, indeed.

>> A simpler solution would be to change the default value of
>> backup-by-copying-when-mismatch to t for everyone rather than just
>> for root.  I don't know what would the objections to this.

> From my pov it would be acceptable, but I don't know how people see it.

Neither do I.

> And we should check what it does mean for remote files, because they
> tend to "mismatch" more often.

That's a very good point.  


        Stefan

Re: backup-by-copying-when-mismatch

[Michael Albinus]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>> A simpler solution would be to change the default value of
>>> backup-by-copying-when-mismatch to t for everyone rather than just
>>> for root.  I don't know what would the objections to this.
>
>> From my pov it would be acceptable, but I don't know how people see it.
>
> Neither do I.

Let's change the default in the trunk. I'll deactivate the setting in
Tramp, and we'll see whether there are complaints. OK?

>> And we should check what it does mean for remote files, because they
>> tend to "mismatch" more often.
>
> That's a very good point.  

I'm a regular Tramp user, I'll keep it in mind to check whether there
are collateral damages.

>         Stefan

Best regards, Michael.

Re: backup-by-copying-when-mismatch

[Michael Albinus]

Michael Albinus <michael.albinus@gmx.de> writes:

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
>>>> A simpler solution would be to change the default value of
>>>> backup-by-copying-when-mismatch to t for everyone rather than just
>>>> for root.  I don't know what would the objections to this.
>>
>>> From my pov it would be acceptable, but I don't know how people see it.
>>
>> Neither do I.
>
> Let's change the default in the trunk. I'll deactivate the setting in
> Tramp, and we'll see whether there are complaints. OK?

Ping.

Stefan, shall I change it this way?

>>         Stefan

Best regards, Michael.

Re: backup-by-copying-when-mismatch

[Stefan Monnier]

>>>>> A simpler solution would be to change the default value of
>>>>> backup-by-copying-when-mismatch to t for everyone rather than just
>>>>> for root.  I don't know what would the objections to this.
>>>> From my pov it would be acceptable, but I don't know how people see it.
>>> Neither do I.
>> Let's change the default in the trunk. I'll deactivate the setting in
>> Tramp, and we'll see whether there are complaints. OK?
> Ping.
> Stefan, shall I change it this way?

OK, let's try it.


        Stefan

Re: backup-by-copying-when-mismatch

[Michael Albinus]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>>>>> A simpler solution would be to change the default value of
>>>>>> backup-by-copying-when-mismatch to t for everyone rather than just
>>>>>> for root.  I don't know what would the objections to this.
>>>>> From my pov it would be acceptable, but I don't know how people see it.
>>>> Neither do I.
>>> Let's change the default in the trunk. I'll deactivate the setting in
>>> Tramp, and we'll see whether there are complaints. OK?
>> Ping.
>> Stefan, shall I change it this way?
>
> OK, let's try it.

done.

>         Stefan

Best regards, Michael.