all messages for Emacs-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
* bug#11490: vc-next-action overwrites changes in non-checked-out RCS file
@ 2012-05-16 19:29 Jonathan Kamens
  2012-05-16 20:12 ` Glenn Morris
  0 siblings, 1 reply; 12+ messages in thread
From: Jonathan Kamens @ 2012-05-16 19:29 UTC (permalink / raw)
  To: 11490

[-- Attachment #1: Type: text/plain, Size: 868 bytes --]

GNU Emacs 24.0.95.1 (x86_64-redhat-linux-gnu, GTK+ Version 2.24.10) of 
2012-04-06 on x86-13.phx2.fedoraproject.org

Make an RCS file writable with chmod +w without locking it.

Make changes to the file.

Type C-x v v.

The file will be locked and checked out and your changes will be 
overwritten.

This is Bad, Bad, Bad. It needs to check if there are non-checked-out 
changes and ask whether to preserve them.

It used to do this. I have no idea why it's behaving differently now or 
when it started behaving this way, but it's clearly wrong and dangerous, 
given the potential to lose work.

I lost a whole day of work recently as a result of this bug. Yeah, it 
was user error, but that's not really the point. It's easy to protect 
the user from losing work due to this editor, and it's something that 
Emacs used to do, so it should continue to do it.

   jik


[-- Attachment #2: Type: text/html, Size: 1204 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#11490: vc-next-action overwrites changes in non-checked-out RCS file
  2012-05-16 19:29 bug#11490: vc-next-action overwrites changes in non-checked-out RCS file Jonathan Kamens
@ 2012-05-16 20:12 ` Glenn Morris
  2012-05-17  1:17   ` Jonathan Kamens
  0 siblings, 1 reply; 12+ messages in thread
From: Glenn Morris @ 2012-05-16 20:12 UTC (permalink / raw)
  To: Jonathan Kamens; +Cc: 11490

Jonathan Kamens wrote:

> GNU Emacs 24.0.95.1 (x86_64-redhat-linux-gnu, GTK+ Version 2.24.10) of
> 2012-04-06 on x86-13.phx2.fedoraproject.org
>
> Make an RCS file writable with chmod +w without locking it.
>
> Make changes to the file.
>
> Type C-x v v.
>
> The file will be locked and checked out and your changes will be
> overwritten.

I cannot reproduce this. I did:


mkdir foo
cd foo
mkdir RCS
echo initial > file
ci -u -t-foo file
chmod +w file
emacs-24.0.96 -Q file
add some text to file
C-x v v

A log buffer appears. I enter some text and press C-c C-c.
I am told the buffer is modified and prompted to save it. I do so.

At this point, RCS returns an error:

  RCS/file,v  <--  file
  ci: RCS/file,v: no lock set by gmorris for revision 1.1

The contents of the file on disk and in the Emacs buffer are unchanged
(ie, the added text is still present).


This was with RCS 5.7. I never normally use RCS, maybe I am missing
something.

Do you have a recipe starting from emacs -Q that shows the problem?





^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#11490: vc-next-action overwrites changes in non-checked-out RCS file
  2012-05-16 20:12 ` Glenn Morris
@ 2012-05-17  1:17   ` Jonathan Kamens
  2012-05-18  0:45     ` Glenn Morris
  0 siblings, 1 reply; 12+ messages in thread
From: Jonathan Kamens @ 2012-05-17  1:17 UTC (permalink / raw)
  To: Glenn Morris; +Cc: 11490

[-- Attachment #1: Type: text/plain, Size: 696 bytes --]

Try it without making the file writable.

This happens, e.g., if a file is edited as root by a user using vim, 
which doesn't care whether it's writable or not, since it's root doing 
the editing, and then by a user using Emacs, which /does/ care because 
it detects VC and thus makes the file read-only in Emacs. The user's 
first impulse will be to type C-x v v, which will overwrite the previous 
editor's changes.

The problem goes away when vc-mistrust-permissions is set to true.

I would argue that C-x v v should always check the status of a file with 
vc-mistrust-permissions set to true (or the logical equivalent) before 
overwriting its contents and potentially losing data.

   jik


[-- Attachment #2: Type: text/html, Size: 975 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#11490: vc-next-action overwrites changes in non-checked-out RCS file
  2012-05-17  1:17   ` Jonathan Kamens
@ 2012-05-18  0:45     ` Glenn Morris
  2012-05-18 14:38       ` Jonathan Kamens
  0 siblings, 1 reply; 12+ messages in thread
From: Glenn Morris @ 2012-05-18  0:45 UTC (permalink / raw)
  To: Jonathan Kamens; +Cc: 11490

Jonathan Kamens wrote:

> Try it without making the file writable.

Well, ok, but your initial report began:

   Make an RCS file writable with chmod +w without locking it.

If I instead use M-x toggle-read-only, make changes, and use C-x v v,
then I see the problem.

> The problem goes away when vc-mistrust-permissions is set to true.

OK. It looks like that variable only affects RCS and SCCS?
In which case I am guessing that few people will care if the default
changes, so maybe we should just do that...





^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#11490: vc-next-action overwrites changes in non-checked-out RCS file
  2012-05-18  0:45     ` Glenn Morris
@ 2012-05-18 14:38       ` Jonathan Kamens
  2012-05-22  3:57         ` Glenn Morris
  0 siblings, 1 reply; 12+ messages in thread
From: Jonathan Kamens @ 2012-05-18 14:38 UTC (permalink / raw)
  To: Glenn Morris; +Cc: 11490

[-- Attachment #1: Type: text/plain, Size: 1094 bytes --]

On 05/17/2012 08:45 PM, Glenn Morris wrote:
> Well, ok, but your initial report began:
>
>    Make an RCS file writable with chmod +w without locking it.
>
> If I instead use M-x toggle-read-only, make changes, and use C-x v v,
> then I see the problem.
Yeah, well, you can never trust users to be accurate in bug reports. :-(
Sorry about that.

Seriously, I've encountered this problem twice on two different
computers recently, and the other time it happened might have been under
Cygwin on Windows, where permissions are screwy.
> OK. It looks like that variable only affects RCS and SCCS?
> In which case I am guessing that few people will care if the default
> changes, so maybe we should just do that...
I'm obviously not in any position of authority here, but as for my
personal opinion, I would have no objection whatsoever to making
vc-mistrust-permissions default to true for safety's sake. Might also
want to put a warning in the documentation of the variable about what
might happen if you set it to false and then use vc-next-action on a
file with non-checked-out changes.

  Jon


[-- Attachment #2: Type: text/html, Size: 1576 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#11490: vc-next-action overwrites changes in non-checked-out RCS file
  2012-05-18 14:38       ` Jonathan Kamens
@ 2012-05-22  3:57         ` Glenn Morris
  2013-01-04  2:57           ` Chong Yidong
  0 siblings, 1 reply; 12+ messages in thread
From: Glenn Morris @ 2012-05-22  3:57 UTC (permalink / raw)
  To: Jonathan Kamens; +Cc: 11490

Jonathan Kamens wrote:

> I'm obviously not in any position of authority here, but as for my
> personal opinion, I would have no objection whatsoever to making
> vc-mistrust-permissions default to true for safety's sake.

Me neither. It seems like the default should be the safest mode of
operation.

However, Emacs 22.3 does not have this issuse; 23.1 onwards does.
Something was lost with the removal of vc-next-action-on-file,
and now vc-buffer-sync does not get called.





^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#11490: vc-next-action overwrites changes in non-checked-out RCS file
  2012-05-22  3:57         ` Glenn Morris
@ 2013-01-04  2:57           ` Chong Yidong
  2013-01-04  3:11             ` Glenn Morris
  0 siblings, 1 reply; 12+ messages in thread
From: Chong Yidong @ 2013-01-04  2:57 UTC (permalink / raw)
  To: Glenn Morris; +Cc: Jonathan Kamens, 11490

>> I'm obviously not in any position of authority here, but as for my
>> personal opinion, I would have no objection whatsoever to making
>> vc-mistrust-permissions default to true for safety's sake.
>
> Me neither. It seems like the default should be the safest mode of
> operation.
>
> However, Emacs 22.3 does not have this issuse; 23.1 onwards does.
> Something was lost with the removal of vc-next-action-on-file,
> and now vc-buffer-sync does not get called.

Can someone summarize again a *correct* recipe to see the bug?  What
with the wrong initial bug report, and the imprecise follow-up, I can't
figure out where the problem lies.  If I do

mkdir foo
cd foo
mkdir RCS
echo initial > file
ci -u -t-foo file
[Edit file in root]
emacs-24.0.96 -Q file
C-x v v

Then I get a prompt saying

  File has unlocked changes.  Claim lock retaining changes? (yes or no)

If I type yes, the changes are still there.  What's the problem?





^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#11490: vc-next-action overwrites changes in non-checked-out RCS file
  2013-01-04  2:57           ` Chong Yidong
@ 2013-01-04  3:11             ` Glenn Morris
  2013-01-04  3:17               ` Jonathan Kamens
  0 siblings, 1 reply; 12+ messages in thread
From: Glenn Morris @ 2013-01-04  3:11 UTC (permalink / raw)
  To: Chong Yidong; +Cc: Jonathan Kamens, 11490

Chong Yidong wrote:

> Can someone summarize again a *correct* recipe to see the bug?

mkdir foo
cd foo
mkdir RCS
echo initial > file
ci -u -t-foo file
emacs-24.2 -Q file

M-x toggle-read-only

Enter some text in the buffer, eg now it looks like:

-----
initial
foobar
-----

Press C-x v v, and "foobar" is deleted with no prompting and no way to
get it back.

I changed vc-mistrust-permissions to t for 24.3 because of this.
But now that I check, it doesn't seem to help...





^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#11490: vc-next-action overwrites changes in non-checked-out RCS file
  2013-01-04  3:11             ` Glenn Morris
@ 2013-01-04  3:17               ` Jonathan Kamens
  2013-01-04  3:21                 ` Glenn Morris
  0 siblings, 1 reply; 12+ messages in thread
From: Jonathan Kamens @ 2013-01-04  3:17 UTC (permalink / raw)
  To: Glenn Morris; +Cc: 11490, Chong Yidong

[-- Attachment #1: Type: text/plain, Size: 1075 bytes --]

On 01/03/2013 10:11 PM, Glenn Morris wrote:
> Chong Yidong wrote:
>
>> Can someone summarize again a *correct* recipe to see the bug?
> mkdir foo
> cd foo
> mkdir RCS
> echo initial > file
> ci -u -t-foo file
> emacs-24.2 -Q file
>
> M-x toggle-read-only
>
> Enter some text in the buffer, eg now it looks like:
>
> -----
> initial
> foobar
> -----
>
> Press C-x v v, and "foobar" is deleted with no prompting and no way to
> get it back.
>
> I changed vc-mistrust-permissions to t for 24.3 because of this.
> But now that I check, it doesn't seem to help...
The problem described above may indeed be a problem, but it's not the 
problem I reported.

The problem I reported is:

mkdir foo
cd foo
mkdir RCS
echo initial > file
ci -u -t-foo file
chmod +w file
echo second >> file
chmod -w file
emacs -Q file
C-x v v - the changes are overwritten without prompting

I think the step missing from Chong Yidong's recipe was making sure the 
file is read-only before trying to edit it in emacs.

If vc-mistrust-permissions is true by default then this issue doesn't occur.

   jik

[-- Attachment #2: Type: text/html, Size: 1618 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#11490: vc-next-action overwrites changes in non-checked-out RCS file
  2013-01-04  3:17               ` Jonathan Kamens
@ 2013-01-04  3:21                 ` Glenn Morris
  2013-01-04  3:33                   ` Jonathan Kamens
  0 siblings, 1 reply; 12+ messages in thread
From: Glenn Morris @ 2013-01-04  3:21 UTC (permalink / raw)
  To: Jonathan Kamens; +Cc: 11490, Chong Yidong

Jonathan Kamens wrote:

> Make an RCS file writable with chmod +w

I wrote:

> I cannot reproduce this

Jonathan Kamens wrote:

> Try it without making the file writable.

I wrote:

> I see the problem.

(months pass)

Jonathan Kamens wrote:

> The problem described above may indeed be a problem, but it's not the
> problem I reported.
>
> The problem I reported is:
[...]
> chmod +w file


Welp, this sure has been crystal clear.





^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#11490: vc-next-action overwrites changes in non-checked-out RCS file
  2013-01-04  3:21                 ` Glenn Morris
@ 2013-01-04  3:33                   ` Jonathan Kamens
  2013-01-05  9:35                     ` Chong Yidong
  0 siblings, 1 reply; 12+ messages in thread
From: Jonathan Kamens @ 2013-01-04  3:33 UTC (permalink / raw)
  To: Glenn Morris; +Cc: 11490, Chong Yidong

[-- Attachment #1: Type: text/plain, Size: 435 bytes --]

Please read the WHOLE LIST OF COMMANDS I SENT in my test case.

I made the file writable in that list only to modify it. If you read the 
whole list of commands, you will see that I make it unwritable again 
before editing it with emacs. And that's exactly the problem... if the 
file isn't writable, emacs assumes that it's not checked out and not 
modified it and checks it out when you hit C-x v v, overwriting the 
changes in it.


[-- Attachment #2: Type: text/html, Size: 651 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* bug#11490: vc-next-action overwrites changes in non-checked-out RCS file
  2013-01-04  3:33                   ` Jonathan Kamens
@ 2013-01-05  9:35                     ` Chong Yidong
  0 siblings, 0 replies; 12+ messages in thread
From: Chong Yidong @ 2013-01-05  9:35 UTC (permalink / raw)
  To: Glenn Morris; +Cc: Jonathan Kamens, 11490

Jonathan Kamens <jik@kamens.us> writes:

> Please read the WHOLE LIST OF COMMANDS I SENT in my test case.
>
> I made the file writable in that list only to modify it. If you read
> the whole list of commands, you will see that I make it unwritable
> again before editing it with emacs. And that's exactly the problem...
> if the file isn't writable, emacs assumes that it's not checked out
> and not modified it and checks it out when you hit C-x v v,
> overwriting the changes in it.

OK, so the original bug is fixed by changing `vc-mistrust-permissions'
to t, indeed.

As for the problem Glenn just pointed out, it arises because VC caches
the state of the file when it is first visited (via vc-state-refresh).
The following patch (against trunk) should fix this, but I haven't had
time to give it much testing yet.


=== modified file 'lisp/vc/vc-hooks.el'
*** lisp/vc/vc-hooks.el	2013-01-02 16:13:04 +0000
--- lisp/vc/vc-hooks.el	2013-04-02 09:23:22 +0000
***************
*** 703,719 ****
    ;; the state to 'edited and redisplay the mode line.
    (let* ((file buffer-file-name)
           (backend (vc-backend file)))
!     (and backend
! 	 (or (and (equal (vc-file-getprop file 'vc-checkout-time)
! 			 (nth 5 (file-attributes file)))
! 		  ;; File has been saved in the same second in which
! 		  ;; it was checked out.  Clear the checkout-time
! 		  ;; to avoid confusion.
! 		  (vc-file-setprop file 'vc-checkout-time nil))
! 	     t)
!          (eq (vc-checkout-model backend (list file)) 'implicit)
!          (vc-state-refresh file backend)
! 	 (vc-mode-line file backend))
      ;; Try to avoid unnecessary work, a *vc-dir* buffer is
      ;; present if this is true.
      (when vc-dir-buffers
--- 703,723 ----
    ;; the state to 'edited and redisplay the mode line.
    (let* ((file buffer-file-name)
           (backend (vc-backend file)))
!     (when backend
!       (if (eq (vc-checkout-model backend (list file)) 'implicit)
! 	  (progn
! 	    ;; If the file was saved in the same second in which it
! 	    ;; was checked out, clear the checkout-time to avoid
! 	    ;; confusion.
! 	    (if (equal (vc-file-getprop file 'vc-checkout-time)
! 		       (nth 5 (file-attributes file)))
! 		(vc-file-setprop file 'vc-checkout-time nil))
! 	    (if (vc-state-refresh file backend)
! 		(vc-mode-line file backend)))
! 	;; If we saved an unlocked file on a locking based VCS, that
! 	;; file is not longer up-to-date.
! 	(if (eq (vc-file-getprop file 'vc-state) 'up-to-date)
! 	    (vc-file-setprop file 'vc-state nil))))
      ;; Try to avoid unnecessary work, a *vc-dir* buffer is
      ;; present if this is true.
      (when vc-dir-buffers

=== modified file 'lisp/vc/vc.el'
*** lisp/vc/vc.el	2013-01-02 16:13:04 +0000
--- lisp/vc/vc.el	2013-04-02 09:30:28 +0000
***************
*** 1072,1077 ****
--- 1072,1087 ----
           ;; among all the `files'.
  	 (model (nth 4 vc-fileset)))
  
+     ;; If a buffer has unsaved changes, a checkout would discard them.
+     (when (and (not (eq model 'implicit))
+ 	       (eq state 'up-to-date))
+       (let ((files files)
+ 	    buffer)
+ 	(while files
+ 	  (setq buffer (get-file-buffer (car files)))
+ 	  (and buffer (buffer-modified-p buffer)
+ 	       (setq state 'unlocked-changes files nil)))))
+ 
      ;; Do the right thing
      (cond
       ((eq state 'missing)






^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2013-01-05  9:35 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-05-16 19:29 bug#11490: vc-next-action overwrites changes in non-checked-out RCS file Jonathan Kamens
2012-05-16 20:12 ` Glenn Morris
2012-05-17  1:17   ` Jonathan Kamens
2012-05-18  0:45     ` Glenn Morris
2012-05-18 14:38       ` Jonathan Kamens
2012-05-22  3:57         ` Glenn Morris
2013-01-04  2:57           ` Chong Yidong
2013-01-04  3:11             ` Glenn Morris
2013-01-04  3:17               ` Jonathan Kamens
2013-01-04  3:21                 ` Glenn Morris
2013-01-04  3:33                   ` Jonathan Kamens
2013-01-05  9:35                     ` Chong Yidong

Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/emacs.git
	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.