bug#37185: 24.5.1: vc--add-line, vc--remove-regexp are sub-optimal

bug#37185: 24.5.1: vc--add-line, vc--remove-regexp are sub-optimal

[Wolfgang Scherer]

In a *vc-dir* buffer for mercurial, ignoring a file with `vc-dir-ignore':

- If the file `.hgignore' is not found, an error is raised:

    vc--add-line: Opening input file: datei oder Verzeichnis nicht gefunden, /srv/install/linux/emacs/check/.hgignore

  This error is unnecesary and serves no real purpose.

- If the file `.hgignore' exists, `vc--add-line' reads the file
  from the filesysstem into a temporary buffer and saves the
  modified buffer back into the filesystem.

  If the file `.hgignore' was already open in a different buffer,
  a prompt is displayed, when trying to modify the contents of
  that buffer:

    File .hgignore changed on disk.  Reread from disk? (y or n) y

  This is at least annoying, if not seriously wrong.

- When adding a new entry to a `.hgignore' file that is already
  terminated with a newline, `vc--add-line' produces an empty
  line and the file ends without a newline, which seriously
  interferes with diffs.

  I fail to see any purpose for such a behavior.

Since `vc-cvs-append-to-ignore' uses `find-file-no-select', there
should be no problem applying the same semantics for mercurial.
The following patch agains the Savannah repository implements the
algorithm from `vc-cvs-append-to-ignore' in `vc--add-line' and
`vc--remove-regexp' as applicable.

diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index 4cac153..bd0b601 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -1449,20 +1449,21 @@ Argument BACKEND is the backend you are using."
 ;; Subroutine for `vc-git-ignore' and `vc-hg-ignore'.
 (defun vc--add-line (string file)
   "Add STRING as a line to FILE."
-  (with-temp-buffer
-    (insert-file-contents file)
+  (with-current-buffer (find-file-noselect file)
+    (goto-char (point-min))
     (unless (re-search-forward (concat "^" (regexp-quote string) "$") nil t)
       (goto-char (point-max))
-      (insert (concat "\n" string))
-      (write-region (point-min) (point-max) file))))
+      (unless (bolp) (insert "\n"))
+      (insert string "\n")
+      (save-buffer))))
 
 (defun vc--remove-regexp (regexp file)
   "Remove all matching for REGEXP in FILE."
-  (with-temp-buffer
-    (insert-file-contents file)
+  (with-current-buffer (find-file-noselect file)
+    (goto-char (point-min))
     (while (re-search-forward regexp nil t)
       (replace-match ""))
-    (write-region (point-min) (point-max) file)))
+    (save-buffer)))
 
 (defun vc-checkout (file &optional rev)
   "Retrieve a copy of the revision REV of FILE.

bug#37185: 24.5.1: vc--add-line, vc--remove-regexp are sub-optimal

[Eli Zaretskii]

> From: Wolfgang Scherer <Wolfgang.Scherer@gmx.de>
> Date: Sun, 25 Aug 2019 22:32:35 +0200
> 
> diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
> index 4cac153..bd0b601 100644
> --- a/lisp/vc/vc.el
> +++ b/lisp/vc/vc.el
> @@ -1449,20 +1449,21 @@ Argument BACKEND is the backend you are using."
>  ;; Subroutine for `vc-git-ignore' and `vc-hg-ignore'.
>  (defun vc--add-line (string file)
>    "Add STRING as a line to FILE."
> -  (with-temp-buffer
> -    (insert-file-contents file)

Thank you for your contributions.  I expect them to be reviewed soon,
but let me point out that the patches you are sending (in this and
other bug reports) have 2 problems which will make their application
harder than necessary:

  . the whitespace is converted to 0x00a0 NBSP characters, which do
    not exist in the original sources
  . the patches lack commit log messages, in the format described in
    the file CONTRIBUTE in the top-level directory of the Emacs
    sources

It is best to generate your patches using "git format-patch" and then
send them as an attachment, I think this will solve these problems,
assuming that you commit locally with the appropriate log message as
described in CONTRIBUTE.

Thanks again for your interest in Emacs.

bug#37185: *** GMX Spamverdacht *** Re: bug#37185: 24.5.1: vc--add-line, vc--remove-regexp are sub-optimal

[Wolfgang Scherer]

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

> It is best to generate your patches using "git format-patch" and then
> send them as an attachment, I think this will solve these problems,
OK. I gave it a shot. Patch is attached.
Since it is slightly different than the first submission, please use this new version for review.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-vc-add-line-vc-remove-regexp-enhanced.patch --]
[-- Type: text/x-patch; name="0001-vc-add-line-vc-remove-regexp-enhanced.patch", Size: 2019 bytes --]

From cf4a651a2ae799efd3c11ac2ccb83fbb1f2683b5 Mon Sep 17 00:00:00 2001
From: Wolfgang Scherer <wolfgang.scherer@gmx.de>
Date: Tue, 27 Aug 2019 00:45:48 +0200
Subject: [PATCH] vc--add-line, vc--remove-regexp enhanced

* lisp/vc/vc.el (vc--add-line): Create file, if it does not exist.
Use existing buffer to avoid discrepancies with filesytem.  Make sure,
the file ends with a newline.
(vc--remove-line): Do not Create file, if it does not exist.  Use
existing buffer to avoid discrepancies with filesytem. (bug#37185)

Copyright-paperwork-exempt: yes
---
 lisp/vc/vc.el | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index 4cac153..d97e7a2 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -1449,20 +1449,22 @@ Argument BACKEND is the backend you are using."
 ;; Subroutine for `vc-git-ignore' and `vc-hg-ignore'.
 (defun vc--add-line (string file)
   "Add STRING as a line to FILE."
-  (with-temp-buffer
-    (insert-file-contents file)
+  (with-current-buffer (find-file-noselect file)
+    (goto-char (point-min))
     (unless (re-search-forward (concat "^" (regexp-quote string) "$") nil t)
       (goto-char (point-max))
-      (insert (concat "\n" string))
-      (write-region (point-min) (point-max) file))))
+      (unless (bolp) (insert "\n"))
+      (insert string "\n")
+      (save-buffer))))

 (defun vc--remove-regexp (regexp file)
   "Remove all matching for REGEXP in FILE."
-  (with-temp-buffer
-    (insert-file-contents file)
-    (while (re-search-forward regexp nil t)
-      (replace-match ""))
-    (write-region (point-min) (point-max) file)))
+  (if (file-exists-p file)
+      (with-current-buffer (find-file-noselect file)
+        (goto-char (point-min))
+        (while (re-search-forward regexp nil t)
+          (replace-match ""))
+        (save-buffer))))

 (defun vc-checkout (file &optional rev)
   "Retrieve a copy of the revision REV of FILE.
--
2.7.4

bug#37185: *** GMX Spamverdacht *** Re: bug#37185: 24.5.1: vc--add-line, vc--remove-regexp are sub-optimal

[Eli Zaretskii]

> Cc: 37185@debbugs.gnu.org
> From: Wolfgang Scherer <Wolfgang.Scherer@gmx.de>
> Date: Tue, 27 Aug 2019 00:52:47 +0200
> 
> > It is best to generate your patches using "git format-patch" and then
> > send them as an attachment, I think this will solve these problems,
> OK. I gave it a shot. Patch is attached.

Thanks, it looks good, but please wait for our VC expert to review it.

> Copyright-paperwork-exempt: yes

I don't think you need this, since your copyright assignment from 2002
is still on file.  If it became invalid since then, I suggest to start
your renewal legal paperwork now, as your contributions are already
beyond the limit we can accept without an assignment.  But if it is
still valid, then just don't use the Copyright-paperwork-exempt
header in the future.

bug#37185: *** GMX Spamverdacht *** Re: *** GMX Spamverdacht *** Re: bug#37185: 24.5.1: vc--add-line, vc--remove-regexp are sub-optimal

[Wolfgang Scherer]

Am 27.08.19 um 09:37 schrieb Eli Zaretskii:
>>
>>> It is best to generate your patches using "git format-patch" and then
>>> send them as an attachment, I think this will solve these problems,
>> OK. I gave it a shot. Patch is attached.
> Thanks, it looks good, but please wait for our VC expert to review it.
Sure I managed for many years, so I am not in a hurry :-)
>> Copyright-paperwork-exempt: yes
> I don't think you need this, since your copyright assignment from 2002
> is still on file.  If it became invalid since then, I suggest to start
> your renewal legal paperwork now, as your contributions are already
> beyond the limit we can accept without an assignment.  But if it is
> still valid, then just don't use the Copyright-paperwork-exempt
> header in the future.

I wasn't sure, if it was still on file. That copyright assignment is
still perfectly valid. I will not use the header any more.

bug#37185: *** GMX Spamverdacht *** Re: bug#37185: 24.5.1: vc--add-line, vc--remove-regexp are sub-optimal

[Dmitry Gutov]

On 27.08.2019 10:37, Eli Zaretskii wrote:
> Thanks, it looks good, but please wait for our VC expert to review it.

Not really an expert on those particular functions, but the changes and 
the explanations look good to me as well.

Wolfgang, do you have commit access?

bug#37185: *** GMX Spamverdacht *** Re: bug#37185: 24.5.1: vc--add-line, vc--remove-regexp are sub-optimal

[Wolfgang Scherer]

Am 14.09.19 um 02:45 schrieb Dmitry Gutov:
> On 27.08.2019 10:37, Eli Zaretskii wrote:
>> Thanks, it looks good, but please wait for our VC expert to review it.
>
> Not really an expert on those particular functions, but the changes
> and the explanations look good to me as well.
>
> Wolfgang, do you have commit access?


No, I don't.

bug#37185: *** GMX Spamverdacht *** Re: bug#37185: 24.5.1: vc--add-line, vc--remove-regexp are sub-optimal

[Wolfgang Scherer]

Am 14.09.19 um 02:45 schrieb Dmitry Gutov:
> On 27.08.2019 10:37, Eli Zaretskii wrote:
>> Thanks, it looks good, but please wait for our VC expert to review it.
>
> Not really an expert on those particular functions, but the changes
> and the explanations look good to me as well.
>
> Wolfgang, do you have commit access?


No, I don't.

bug#37185: *** GMX Spamverdacht *** Re: bug#37185: 24.5.1: vc--add-line, vc--remove-regexp are sub-optimal

[Dmitry Gutov]

On 16.09.2019 20:15, Wolfgang Scherer wrote:
> Am 14.09.19 um 02:45 schrieb Dmitry Gutov:
>> On 27.08.2019 10:37, Eli Zaretskii wrote:
>>> Thanks, it looks good, but please wait for our VC expert to review it.
>>
>> Not really an expert on those particular functions, but the changes
>> and the explanations look good to me as well.
>>
>> Wolfgang, do you have commit access?
> 
> 
> No, I don't.

Installed with some changes in the commit message.

Thanks you.