Package: emacs;
Reported by: Juri Linkov <juri <at> linkov.net>
Date: Sat, 1 Dec 2018 22:13:02 UTC
Severity: wishlist
Tags: patch
Done: Juri Linkov <juri <at> linkov.net>
Bug is archived. No further changes may be made.
To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 33567 in the body.
You can then email your comments to 33567 AT debbugs.gnu.org in the normal way.
Toggle the display of automated, internal messages from the tracker.
View this report as an mbox folder, status mbox, maintainer mbox
bug-gnu-emacs <at> gnu.org:bug#33567; Package emacs.
(Sat, 01 Dec 2018 22:13:02 GMT) Full text and rfc822 format available.Juri Linkov <juri <at> linkov.net>:bug-gnu-emacs <at> gnu.org.
(Sat, 01 Dec 2018 22:13:02 GMT) Full text and rfc822 format available.Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):
From: Juri Linkov <juri <at> linkov.net> To: bug-gnu-emacs <at> gnu.org Subject: Syntactic fontification of diff hunks Date: Sat, 01 Dec 2018 23:55:40 +0200
[Message part 1 (text/plain, inline)]
Tags: patch Severity: wishlist For a long time after announcing this feature in https://lists.gnu.org/archive/html/emacs-devel/2018-08/msg00537.html I received requests in private mails asking when I'll submit a complete patch. I'm sorry, it took much time addressing all concerns raised in that thread, and testing in many possible scenarios. Based on the feedback, I rewrote it several times, and now finally it's optimized to be fast and reliable.
[diff-font-lock-syntax.patch (text/x-diff, inline)]
diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 4adef02984..68b2f9e522 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -103,6 +103,21 @@ diff-font-lock-prettify
:version "27.1"
:type 'boolean)
+(defcustom diff-font-lock-syntax 'vc
+ "If non-nil, diff hunk font-lock includes syntax highlighting.
+If `vc', highlight syntax only in Diff buffers created by a version control
+system that provides all necessary context for reliable highlighting.
+If t, additionally try to get more context from existing files, or when
+source files are not found, still try to highlight hunks without enough
+context that sometimes might result in wrong fontification.
+If `hunk-only', fontification is based on hunk alone, without full source.
+This is the fastest, but less reliable."
+ :version "27.1"
+ :type '(choice (const :tag "Don't highlight syntax" nil)
+ (const :tag "Only under version control" vc)
+ (const :tag "Hunk-based only" hunk-only)
+ (const :tag "Without full source or get it from files" t)))
+
(defvar diff-vc-backend nil
"The VC backend that created the current Diff buffer, if any.")
@@ -406,6 +421,7 @@ diff-font-lock-keywords
(1 font-lock-comment-delimiter-face)
(2 font-lock-comment-face))
("^[^-=+*!<>#].*\n" (0 'diff-context))
+ (,#'diff--font-lock-syntax)
(,#'diff--font-lock-prettify)
(,#'diff--font-lock-refined)))
@@ -2316,6 +2333,195 @@ diff--font-lock-prettify
'display "")))))
nil)
+;;; Syntax highlighting from font-lock
+
+(defun diff--font-lock-syntax (max)
+ "Syntax highlighting from font-lock."
+ (when diff-font-lock-syntax
+ (when (get-char-property (point) 'diff--font-lock-syntax)
+ (goto-char (next-single-char-property-change
+ (point) 'diff--font-lock-syntax nil max)))
+ (let* ((min (point))
+ (beg (or (ignore-errors (diff-beginning-of-hunk))
+ (ignore-errors (diff-hunk-next) (point))
+ max)))
+ (while (< beg max)
+ (let ((end
+ (save-excursion (goto-char beg) (diff-end-of-hunk) (point))))
+ (if (< end min) (setq beg min))
+ (unless (or (< end beg)
+ (get-char-property beg 'diff--font-lock-syntax))
+ (diff-syntax-fontify beg end)
+ (let ((ol (make-overlay beg end)))
+ (overlay-put ol 'diff--font-lock-syntax t)
+ (overlay-put ol 'diff-mode 'syntax)
+ (overlay-put ol 'evaporate t)
+ (overlay-put ol 'modification-hooks
+ '(diff--font-lock-syntax--refresh))))
+ (goto-char (max beg end))
+ (setq beg (or (ignore-errors (diff-hunk-next) (point)) max))))))
+ nil)
+
+(defun diff--font-lock-syntax--refresh (ol _after _beg _end &optional _len)
+ (delete-overlay ol))
+
+(defun diff-syntax-fontify (start end)
+ (save-excursion
+ (diff-syntax-fontify-hunk start end t)
+ (diff-syntax-fontify-hunk start end nil)))
+
+(defvar diff-syntax-fontify-revisions (make-hash-table :test 'equal))
+
+(defun diff-syntax-fontify-hunk (beg end old)
+ "Highlight language syntax in diff hunks."
+ (remove-overlays beg end 'diff-mode 'syntax)
+ (goto-char beg)
+ (let* ((hunk (buffer-substring-no-properties beg end))
+ (text (or (ignore-errors (diff-hunk-text hunk (not old) nil)) ""))
+ (line (if (looking-at "\\(?:\\*\\{15\\}.*\n\\)?[-@* ]*\\([0-9,]+\\)\\([ acd+]+\\([0-9,]+\\)\\)?")
+ (if old (match-string 1)
+ (if (match-end 3) (match-string 3) (match-string 1)))))
+ (line-nb (and line (string-match "\\([0-9]+\\),\\([0-9]+\\)" line)
+ (list (string-to-number (match-string 1 line))
+ (string-to-number (match-string 2 line)))))
+ props)
+ (cond
+ ((and diff-vc-backend (not (eq diff-font-lock-syntax 'hunk-only)))
+ (let* ((file (diff-find-file-name old t))
+ (revision (and file (if (not old) (nth 1 diff-vc-revisions)
+ (or (nth 0 diff-vc-revisions)
+ (vc-working-revision file))))))
+ (if file
+ (if (not revision)
+ ;; Get properties from the current working file
+ (when (and (not old) (file-exists-p file))
+ ;; Try to reuse an existing buffer
+ (if (get-file-buffer (expand-file-name file))
+ (with-current-buffer (get-file-buffer (expand-file-name file))
+ (setq props (diff-syntax-fontify-props nil text line-nb t)))
+ ;; Get properties from the file
+ (with-temp-buffer
+ (insert-file-contents file t)
+ (setq props (diff-syntax-fontify-props file text line-nb)))))
+ ;; Get properties from a cached revision
+ (let* ((buffer-name (format " diff-syntax:%s.~%s~"
+ (expand-file-name file) revision))
+ (buffer (gethash buffer-name diff-syntax-fontify-revisions))
+ (no-init t))
+ (unless (and buffer (buffer-live-p buffer))
+ (let* ((vc-find-revision-no-save t)
+ (vc-buffer (save-window-excursion
+ ;; Restore restore previous window configuration
+ ;; because when vc-find-revision can't find a revision
+ ;; (e.g. for /dev/null), it jumps to another window
+ ;; using pop-to-buffer in vc-do-command when
+ ;; the buffer name doesn't begin with a space char.
+ (ignore-errors
+ (vc-find-revision (expand-file-name file)
+ revision diff-vc-backend)))))
+ (when vc-buffer
+ (with-current-buffer (get-buffer-create buffer-name)
+ (insert-buffer-substring-no-properties vc-buffer)
+ (setq buffer (current-buffer) no-init nil))
+ (puthash buffer-name buffer diff-syntax-fontify-revisions)
+ (kill-buffer vc-buffer))))
+ (when buffer
+ (with-current-buffer buffer
+ (setq props (diff-syntax-fontify-props file text line-nb no-init))))))
+ ;; If file is unavailable, get properties from the hunk alone
+ (setq file (car (diff-hunk-file-names old)))
+ (with-temp-buffer
+ (insert text)
+ (setq props (diff-syntax-fontify-props file text line-nb nil t))))))
+ ((eq diff-font-lock-syntax 'hunk-only)
+ (setq file (car (diff-hunk-file-names old)))
+ (with-temp-buffer
+ (insert text)
+ (setq props (diff-syntax-fontify-props file text line-nb nil t))))
+ ((not (eq diff-font-lock-syntax 'vc))
+ (let ((file (car (diff-hunk-file-names old))))
+ (if (and file (file-exists-p file))
+ ;; Try to get full text from the file
+ (with-temp-buffer
+ (insert-file-contents file t)
+ (setq props (diff-syntax-fontify-props file text line-nb)))
+ ;; Otherwise, get properties from the hunk alone
+ (with-temp-buffer
+ (insert text)
+ (setq props (diff-syntax-fontify-props file text line-nb nil t)))))))
+
+ ;; Put properties over the hunk text
+ (when props
+ (goto-char beg)
+ (while (< (progn (forward-line 1) (point)) end)
+ (when (or (and (not old) (not (looking-at-p "[-<]")))
+ (and old (not (looking-at-p "[+>]"))))
+ (if (and old (not (looking-at-p "[-<]")))
+ ;; Fontify context lines only from new source,
+ ;; don't refontify context lines from old source.
+ (pop props)
+ (let ((line-props (pop props))
+ (bol (1+ (point))))
+ (dolist (prop line-props)
+ (let ((ol (make-overlay (+ bol (nth 0 prop))
+ (+ bol (nth 1 prop))
+ nil 'front-advance nil)))
+ (overlay-put ol 'evaporate t)
+ (overlay-put ol 'face (nth 2 prop)))))))))))
+
+(defun diff-syntax-fontify-props (file text line-nb &optional no-init hunk-only)
+ "Get font-lock properties from the source code."
+ (unless no-init
+ (buffer-disable-undo)
+ (font-lock-mode -1)
+ (let ((enable-local-variables :safe) ;; to find `mode:'
+ (buffer-file-name file))
+ (set-auto-mode)
+ (generic-mode-find-file-hook)))
+
+ (let ((font-lock-defaults (or font-lock-defaults '(nil t)))
+ props beg end)
+ (goto-char (point-min))
+ (if hunk-only
+ (setq beg (point-min) end (point-max))
+ (forward-line (1- (nth 0 line-nb)))
+ ;; non-regexp looking-at to compare hunk text for verification
+ (if (search-forward text (+ (point) (length text)) t)
+ (setq beg (- (point) (length text)) end (point))
+ (goto-char (point-min))
+ (if (search-forward text nil t)
+ (setq beg (- (point) (length text)) end (point)))))
+
+ (when (and beg end)
+ (goto-char beg)
+ (when (text-property-not-all beg end 'fontified t)
+ (if file
+ ;; In a temporary or cached buffer
+ (save-excursion
+ (font-lock-fontify-region beg end)
+ (put-text-property beg end 'fontified t))
+ ;; In an existing buffer
+ (font-lock-ensure beg end)))
+
+ (while (< (point) end)
+ (let* ((bol (point))
+ (eol (line-end-position))
+ line-props
+ (searching t)
+ (from (point)) to
+ (val (get-text-property from 'face)))
+ (while searching
+ (setq to (next-single-property-change from 'face nil eol))
+ (when val (push (list (- from bol) (- to bol) val) line-props))
+ (setq val (get-text-property to 'face) from to)
+ (unless (< to eol) (setq searching nil)))
+ (when val (push (list from eol val) line-props))
+ (push (nreverse line-props) props))
+ (forward-line 1)))
+ (set-buffer-modified-p nil)
+ (nreverse props)))
+
+
(defun diff--filter-substring (str)
(when diff-font-lock-prettify
;; Strip the `display' properties added by diff-font-lock-prettify,
bug-gnu-emacs <at> gnu.org:bug#33567; Package emacs.
(Sun, 02 Dec 2018 06:58:02 GMT) Full text and rfc822 format available.Message #8 received at 33567 <at> debbugs.gnu.org (full text, mbox):
From: Eli Zaretskii <eliz <at> gnu.org> To: Juri Linkov <juri <at> linkov.net> Cc: 33567 <at> debbugs.gnu.org Subject: Re: bug#33567: Syntactic fontification of diff hunks Date: Sun, 02 Dec 2018 08:56:44 +0200
> From: Juri Linkov <juri <at> linkov.net> > Date: Sat, 01 Dec 2018 23:55:40 +0200 > > For a long time after announcing this feature in > https://lists.gnu.org/archive/html/emacs-devel/2018-08/msg00537.html > I received requests in private mails asking when I'll submit a complete patch. > I'm sorry, it took much time addressing all concerns raised in that thread, > and testing in many possible scenarios. Based on the feedback, I rewrote it > several times, and now finally it's optimized to be fast and reliable. Thank you for working on this. A few comments below. > +(defcustom diff-font-lock-syntax 'vc > + "If non-nil, diff hunk font-lock includes syntax highlighting. > +If `vc', highlight syntax only in Diff buffers created by a version control > +system that provides all necessary context for reliable highlighting. > +If t, additionally try to get more context from existing files, or when > +source files are not found, still try to highlight hunks without enough > +context that sometimes might result in wrong fontification. > +If `hunk-only', fontification is based on hunk alone, without full source. > +This is the fastest, but less reliable." I don't think one can understand what the feature does just by reading the doc string. I think something very basic is missing here, without which the rest of the doc text cannot be unlocked. Perhaps just elaborating on what exactly "syntax highlighting" means in this context would be enough. Also, judging by my reading of the code, the description of what the various non-nil values do is not entirely accurate, and might not be what the user expects by reading the above description. > + :type '(choice (const :tag "Don't highlight syntax" nil) > + (const :tag "Only under version control" vc) The "under" part of "Under version control" seems to say something very different from what the doc string says about this value. > + (const :tag "Without full source or get it from files" t))) This description is backwards, I think: you should start with "with source files". (But maybe I misunderstand the whole issue, see above.) > + ;; Restore restore previous window configuration > + ;; because when vc-find-revision can't find a revision > + ;; (e.g. for /dev/null), it jumps to another window > + ;; using pop-to-buffer in vc-do-command when > + ;; the buffer name doesn't begin with a space char. Nitpicking: can this comment please be refilled to not exceed "normal" line width? > + ((not (eq diff-font-lock-syntax 'vc)) > + (let ((file (car (diff-hunk-file-names old)))) > + (if (and file (file-exists-p file)) This assumes that the file name is relative to the default-directory of the buffer with the diffs, right? How reasonable is such an assumption for when browsing diffs? Should we perhaps allow the user to specify the directory of the sources? Also, if the diffs are from Git, they begin with a/, b/, etc. dummy directories, which usually don't exist in the file system. Finally, please include the necessary documentation changes with the final changeset.
bug-gnu-emacs <at> gnu.org:bug#33567; Package emacs.
(Mon, 03 Dec 2018 01:29:03 GMT) Full text and rfc822 format available.Message #11 received at 33567 <at> debbugs.gnu.org (full text, mbox):
From: Juri Linkov <juri <at> linkov.net> To: Eli Zaretskii <eliz <at> gnu.org> Cc: 33567 <at> debbugs.gnu.org Subject: Re: bug#33567: Syntactic fontification of diff hunks Date: Mon, 03 Dec 2018 02:34:14 +0200
[Message part 1 (text/plain, inline)]
> I don't think one can understand what the feature does just by reading > the doc string. I think something very basic is missing here, without > which the rest of the doc text cannot be unlocked. Perhaps just > elaborating on what exactly "syntax highlighting" means in this > context would be enough. > > Also, judging by my reading of the code, the description of what the > various non-nil values do is not entirely accurate, and might not be > what the user expects by reading the above description. I tried to explain this more thoughtfully in a new version of the docstring. >> + (const :tag "Without full source or get it from files" t))) > > This description is backwards, I think: you should start with "with > source files". (But maybe I misunderstand the whole issue, see > above.) Fixed in a new version. Also please note why `t' is not the default. This is to avoid trying to read local files while a received patch is displayed in the mail attachment. I think to avoid such situations when it will try to read random files, better to use the same method as is used in diff buffers created by a VC command - it sets a special variable `diff-vc-backend' that guarantees that diff buffer is created with file paths relative to the process that created these buffers. I propose for commands that compare files like diff, diff-backup, dired-diff, dired-backup-diff also to set a similar variable e.g. `diff-files' that will guarantee that file paths are valid to read. >> + ;; Restore restore previous window configuration >> + ;; because when vc-find-revision can't find a revision >> + ;; (e.g. for /dev/null), it jumps to another window >> + ;; using pop-to-buffer in vc-do-command when >> + ;; the buffer name doesn't begin with a space char. > > Nitpicking: can this comment please be refilled to not exceed "normal" > line width? Fixing the described problem will remove this comment, but I have no idea how better to do this. The problem is that we need to provide own created buffer to the call to `vc-find-revision'. Currently it has the following function signature: (defun vc-find-revision (file revision &optional backend) But VC API in the comments in the beginning of vc.el is documented with a different function signature: ;; * find-revision (file rev buffer) ;; ;; Fetch revision REV of file FILE and put it into BUFFER. ;; If REV is the empty string, fetch the head of the trunk. ;; The implementation should pass the value of vc-checkout-switches ;; to the backend command. This means that to fix the problem we need the call as is documented with the argument BUFFER, but the current implementation without such argument doesn't correspond to the documentation. BTW, while deciding what to do with this, could you please confirm if I correctly fixed another problem in vc-find-revision-no-save. Recently in bug#33319 I added this function but now discovered a problem with encoding. A vc process outputs lines to the buffer with no-conversion, so in the patch below I added recode-region to convert output to the buffer's encoding. coding-system-for-write that I removed was copied from vc-find-revision-save where is was needed for write-region called from the macro with-temp-file, but vc-find-revision-no-save doesn't write output to the file. >> + ((not (eq diff-font-lock-syntax 'vc)) >> + (let ((file (car (diff-hunk-file-names old)))) >> + (if (and file (file-exists-p file)) > > This assumes that the file name is relative to the default-directory > of the buffer with the diffs, right? How reasonable is such an > assumption for when browsing diffs? Should we perhaps allow the user > to specify the directory of the sources? This assumption should be true for all cases when the diff buffer is created using commands like dired-diff, dired-backup-diff, diff, diff-backup. But when navigating a diff output saved to a file that was moved to another directory, currently diff-mode asks for a directory interactively, that is not possible to do for non-interactive fontification. As a general solution is should be possible to specify the default directory in the local variables at the first line of the diff files as currently already is used in compilation/grep buffers like -*- mode: diff-mode; default-directory: "..." -*- > Also, if the diffs are from Git, they begin with a/, b/, etc. dummy > directories, which usually don't exist in the file system. This is not a problem because diff-find-file-name used in the patch strips such a/, b/ prefixes to get the existing file name.
[diff-font-lock-syntax.2.patch (text/x-diff, inline)]
diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index de43544864..00ec1226d7 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -2008,8 +2008,7 @@ vc-find-revision-no-save
(with-current-buffer filebuf
(let ((failed t))
(unwind-protect
- (let ((coding-system-for-read 'no-conversion)
- (coding-system-for-write 'no-conversion))
+ (let ((coding-system-for-read 'no-conversion))
(with-current-buffer (create-file-buffer filename)
(setq buffer-file-name filename)
(let ((outbuf (current-buffer)))
@@ -2019,6 +2018,9 @@ vc-find-revision-no-save
(vc-call find-revision file revision outbuf))))
(goto-char (point-min))
(normal-mode)
+ (recode-region (point-min) (point-max)
+ (car (detect-coding-region (point-min) (point-max)))
+ 'no-conversion)
(set-buffer-modified-p nil)
(setq buffer-read-only t))
(setq failed nil))
diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 4adef02984..02421e2630 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -103,6 +103,31 @@ diff-font-lock-prettify
:version "27.1"
:type 'boolean)
+(defcustom diff-font-lock-syntax 'vc
+ "If non-nil, diff hunk's font-lock includes language syntax highlighting.
+This highlighting is the same as added by `font-lock-mode' when
+corresponding source files are visited from the diff buffer.
+In diff hunks syntax highlighting is added over diff own
+highlighted changes.
+
+If `vc', highlight syntax only in Diff buffers created by a version control
+system that provides all necessary context for reliable highlighting.
+For working revisions get highlighting according to the working
+copy of the file.
+
+If `hunk-only', fontification is based on hunk alone, without full source.
+It tries to highlight hunks without enough context that sometimes might result
+in wrong fontification. This is the fastest option, but less reliable.
+
+If t, additionally to trying to use a version control system to get
+old revisions for fontification, also try to get fontification based
+on existing files, and on failure get fontification from hunk alone."
+ :version "27.1"
+ :type '(choice (const :tag "Don't highlight syntax" nil)
+ (const :tag "Use version control" vc)
+ (const :tag "Hunk-based only" hunk-only)
+ (const :tag "Try everything including files" t)))
+
(defvar diff-vc-backend nil
"The VC backend that created the current Diff buffer, if any.")
@@ -406,6 +431,7 @@ diff-font-lock-keywords
(1 font-lock-comment-delimiter-face)
(2 font-lock-comment-face))
("^[^-=+*!<>#].*\n" (0 'diff-context))
+ (,#'diff--font-lock-syntax)
(,#'diff--font-lock-prettify)
(,#'diff--font-lock-refined)))
@@ -1348,6 +1374,7 @@ diff-next-error
(defun diff--font-lock-cleanup ()
(remove-overlays nil nil 'diff-mode 'fine)
+ (remove-overlays nil nil 'diff-mode 'syntax)
(when font-lock-mode
(make-local-variable 'font-lock-extra-managed-props)
;; Added when diff--font-lock-prettify is non-nil!
@@ -1748,7 +1775,7 @@ diff-find-source-location
(vc-working-revision file)))))
(buf (if revision
(let ((vc-find-revision-no-save t))
- (vc-find-revision file revision diff-vc-backend))
+ (vc-find-revision (expand-file-name file) revision diff-vc-backend))
(find-file-noselect file))))
;; Update the user preference if he so wished.
(when (> (prefix-numeric-value other-file) 8)
@@ -2316,6 +2343,197 @@ diff--font-lock-prettify
'display "")))))
nil)
+;;; Syntax highlighting from font-lock
+
+(defun diff--font-lock-syntax (max)
+ "Syntax highlighting from font-lock."
+ (when diff-font-lock-syntax
+ (when (get-char-property (point) 'diff--font-lock-syntax)
+ (goto-char (next-single-char-property-change
+ (point) 'diff--font-lock-syntax nil max)))
+ (let* ((min (point))
+ (beg (or (ignore-errors (diff-beginning-of-hunk))
+ (ignore-errors (diff-hunk-next) (point))
+ max)))
+ (while (< beg max)
+ (let ((end
+ (save-excursion (goto-char beg) (diff-end-of-hunk) (point))))
+ (if (< end min) (setq beg min))
+ (unless (or (< end beg)
+ (get-char-property beg 'diff--font-lock-syntax))
+ (diff-syntax-fontify beg end)
+ (let ((ol (make-overlay beg end)))
+ (overlay-put ol 'diff--font-lock-syntax t)
+ (overlay-put ol 'diff-mode 'syntax)
+ (overlay-put ol 'evaporate t)
+ (overlay-put ol 'modification-hooks
+ '(diff--font-lock-syntax--refresh))))
+ (goto-char (max beg end))
+ (setq beg (or (ignore-errors (diff-hunk-next) (point)) max))))))
+ nil)
+
+(defun diff--font-lock-syntax--refresh (ol _after _beg _end &optional _len)
+ (delete-overlay ol))
+
+(defun diff-syntax-fontify (start end)
+ (save-excursion
+ (diff-syntax-fontify-hunk start end t)
+ (diff-syntax-fontify-hunk start end nil)))
+
+(defvar diff-syntax-fontify-revisions (make-hash-table :test 'equal))
+
+(defun diff-syntax-fontify-hunk (beg end old)
+ "Highlight language syntax in diff hunks."
+ (remove-overlays beg end 'diff-mode 'syntax)
+ (goto-char beg)
+ (let* ((hunk (buffer-substring-no-properties beg end))
+ (text (or (ignore-errors (diff-hunk-text hunk (not old) nil)) ""))
+ (line (if (looking-at "\\(?:\\*\\{15\\}.*\n\\)?[-@* ]*\\([0-9,]+\\)\\([ acd+]+\\([0-9,]+\\)\\)?")
+ (if old (match-string 1)
+ (if (match-end 3) (match-string 3) (match-string 1)))))
+ (line-nb (and line (string-match "\\([0-9]+\\),\\([0-9]+\\)" line)
+ (list (string-to-number (match-string 1 line))
+ (string-to-number (match-string 2 line)))))
+ props)
+ (cond
+ ((and diff-vc-backend (not (eq diff-font-lock-syntax 'hunk-only)))
+ (let* ((file (diff-find-file-name old t))
+ (revision (and file (if (not old) (nth 1 diff-vc-revisions)
+ (or (nth 0 diff-vc-revisions)
+ (vc-working-revision file))))))
+ (if file
+ (if (not revision)
+ ;; Get properties from the current working revision
+ (when (and (not old) (file-exists-p file))
+ ;; Try to reuse an existing buffer
+ (if (get-file-buffer (expand-file-name file))
+ (with-current-buffer (get-file-buffer (expand-file-name file))
+ (setq props (diff-syntax-fontify-props nil text line-nb t)))
+ ;; Get properties from the file
+ (with-temp-buffer
+ (insert-file-contents file t)
+ (setq props (diff-syntax-fontify-props file text line-nb)))))
+ ;; Get properties from a cached revision
+ (let* ((buffer-name (format " diff-syntax:%s.~%s~"
+ (expand-file-name file) revision))
+ (buffer (gethash buffer-name diff-syntax-fontify-revisions))
+ (no-init t))
+ (unless (and buffer (buffer-live-p buffer))
+ (let* ((vc-find-revision-no-save t)
+ (vc-buffer (save-window-excursion
+ ;; Restore restore previous window configuration
+ ;; because when vc-find-revision can't find a revision
+ ;; (e.g. for /dev/null), it jumps to another window
+ ;; using pop-to-buffer in vc-do-command when
+ ;; the buffer name doesn't begin with a space char.
+ (ignore-errors
+ (vc-find-revision (expand-file-name file)
+ revision diff-vc-backend)))))
+ (when vc-buffer
+ (with-current-buffer (get-buffer-create buffer-name)
+ (insert-buffer-substring-no-properties vc-buffer)
+ (setq buffer (current-buffer) no-init nil))
+ (puthash buffer-name buffer diff-syntax-fontify-revisions)
+ (kill-buffer vc-buffer))))
+ (when buffer
+ (with-current-buffer buffer
+ (setq props (diff-syntax-fontify-props file text line-nb no-init))))))
+ ;; If file is unavailable, get properties from the hunk alone
+ (setq file (car (diff-hunk-file-names old)))
+ (with-temp-buffer
+ (insert text)
+ (setq props (diff-syntax-fontify-props file text line-nb nil t))))))
+ ((eq diff-font-lock-syntax 'hunk-only)
+ (let ((file (car (diff-hunk-file-names old))))
+ (with-temp-buffer
+ (insert text)
+ (setq props (diff-syntax-fontify-props file text line-nb nil t)))))
+ ((not (eq diff-font-lock-syntax 'vc))
+ (let ((file (car (diff-hunk-file-names old))))
+ (if (and file (file-exists-p file))
+ ;; Try to get full text from the file
+ (with-temp-buffer
+ (insert-file-contents file t)
+ (setq props (diff-syntax-fontify-props file text line-nb)))
+ ;; Otherwise, get properties from the hunk alone
+ (with-temp-buffer
+ (insert text)
+ (setq props (diff-syntax-fontify-props file text line-nb nil t)))))))
+
+ ;; Put properties over the hunk text
+ (when props
+ (goto-char beg)
+ (while (< (progn (forward-line 1) (point)) end)
+ (when (or (and (not old) (not (looking-at-p "[-<]")))
+ (and old (not (looking-at-p "[+>]"))))
+ (if (and old (not (looking-at-p "[-<]")))
+ ;; Fontify context lines only from new source,
+ ;; don't refontify context lines from old source.
+ (pop props)
+ (let ((line-props (pop props))
+ (bol (1+ (point))))
+ (dolist (prop line-props)
+ (let ((ol (make-overlay (+ bol (nth 0 prop))
+ (+ bol (nth 1 prop))
+ nil 'front-advance nil)))
+ (overlay-put ol 'evaporate t)
+ (overlay-put ol 'face (nth 2 prop)))))))))))
+
+(defun diff-syntax-fontify-props (file text line-nb &optional no-init hunk-only)
+ "Get font-lock properties from the source code."
+ (unless no-init
+ (buffer-disable-undo)
+ (font-lock-mode -1)
+ (let ((enable-local-variables :safe) ;; to find `mode:'
+ (buffer-file-name file))
+ (set-auto-mode)
+ (when (and (memq 'generic-mode-find-file-hook find-file-hook)
+ (fboundp 'generic-mode-find-file-hook))
+ (generic-mode-find-file-hook))))
+
+ (let ((font-lock-defaults (or font-lock-defaults '(nil t)))
+ props beg end)
+ (goto-char (point-min))
+ (if hunk-only
+ (setq beg (point-min) end (point-max))
+ (forward-line (1- (nth 0 line-nb)))
+ ;; non-regexp looking-at to compare hunk text for verification
+ (if (search-forward text (+ (point) (length text)) t)
+ (setq beg (- (point) (length text)) end (point))
+ (goto-char (point-min))
+ (if (search-forward text nil t)
+ (setq beg (- (point) (length text)) end (point)))))
+
+ (when (and beg end)
+ (goto-char beg)
+ (when (text-property-not-all beg end 'fontified t)
+ (if file
+ ;; In a temporary or cached buffer
+ (save-excursion
+ (font-lock-fontify-region beg end)
+ (put-text-property beg end 'fontified t))
+ ;; In an existing buffer
+ (font-lock-ensure beg end)))
+
+ (while (< (point) end)
+ (let* ((bol (point))
+ (eol (line-end-position))
+ line-props
+ (searching t)
+ (from (point)) to
+ (val (get-text-property from 'face)))
+ (while searching
+ (setq to (next-single-property-change from 'face nil eol))
+ (when val (push (list (- from bol) (- to bol) val) line-props))
+ (setq val (get-text-property to 'face) from to)
+ (unless (< to eol) (setq searching nil)))
+ (when val (push (list from eol val) line-props))
+ (push (nreverse line-props) props))
+ (forward-line 1)))
+ (set-buffer-modified-p nil)
+ (nreverse props)))
+
+
(defun diff--filter-substring (str)
(when diff-font-lock-prettify
;; Strip the `display' properties added by diff-font-lock-prettify,
bug-gnu-emacs <at> gnu.org:bug#33567; Package emacs.
(Mon, 03 Dec 2018 06:50:02 GMT) Full text and rfc822 format available.Message #14 received at 33567 <at> debbugs.gnu.org (full text, mbox):
From: Eli Zaretskii <eliz <at> gnu.org> To: Juri Linkov <juri <at> linkov.net> Cc: 33567 <at> debbugs.gnu.org Subject: Re: bug#33567: Syntactic fontification of diff hunks Date: Mon, 03 Dec 2018 08:49:15 +0200
> From: Juri Linkov <juri <at> linkov.net> > Cc: 33567 <at> debbugs.gnu.org > Date: Mon, 03 Dec 2018 02:34:14 +0200 > > > Nitpicking: can this comment please be refilled to not exceed "normal" > > line width? > > Fixing the described problem will remove this comment, > but I have no idea how better to do this. The problem is that > we need to provide own created buffer to the call to `vc-find-revision'. > Currently it has the following function signature: > > (defun vc-find-revision (file revision &optional backend) > > But VC API in the comments in the beginning of vc.el > is documented with a different function signature: > > ;; * find-revision (file rev buffer) > ;; > ;; Fetch revision REV of file FILE and put it into BUFFER. > ;; If REV is the empty string, fetch the head of the trunk. > ;; The implementation should pass the value of vc-checkout-switches > ;; to the backend command. > > This means that to fix the problem we need the call as is documented > with the argument BUFFER, but the current implementation without > such argument doesn't correspond to the documentation. I'm not an expert on those details, sorry. Perhaps someone else (Dmitry? Martin?) could help you. > BTW, while deciding what to do with this, could you please confirm > if I correctly fixed another problem in vc-find-revision-no-save. > Recently in bug#33319 I added this function but now discovered > a problem with encoding. A vc process outputs lines to the buffer > with no-conversion, so in the patch below I added recode-region > to convert output to the buffer's encoding. coding-system-for-write > that I removed was copied from vc-find-revision-save where is was > needed for write-region called from the macro with-temp-file, > but vc-find-revision-no-save doesn't write output to the file. vc-find-revision disables encoding/decoding because it wants to create an identical copy of the checked-out file, and doesn't want to be tripped by encoding/decoding issues. But in your case you don't write the buffer to a file, so why do you need to bind coding-system-for-read at all? I say leave it unbound, and let Emacs do its job decoding the text as usual. Does that not work? > >> + ((not (eq diff-font-lock-syntax 'vc)) > >> + (let ((file (car (diff-hunk-file-names old)))) > >> + (if (and file (file-exists-p file)) > > > > This assumes that the file name is relative to the default-directory > > of the buffer with the diffs, right? How reasonable is such an > > assumption for when browsing diffs? Should we perhaps allow the user > > to specify the directory of the sources? > > This assumption should be true for all cases when the diff buffer is created > using commands like dired-diff, dired-backup-diff, diff, diff-backup. > > But when navigating a diff output saved to a file that was moved to > another directory, currently diff-mode asks for a directory interactively, > that is not possible to do for non-interactive fontification. > > As a general solution is should be possible to specify the default > directory in the local variables at the first line of the diff files > as currently already is used in compilation/grep buffers like > > -*- mode: diff-mode; default-directory: "..." -*- This is all fine, but I think we should document that files are visited relative to default-directory of the buffer, so that users could invoke "cd" to change that as needed. > > Also, if the diffs are from Git, they begin with a/, b/, etc. dummy > > directories, which usually don't exist in the file system. > > This is not a problem because diff-find-file-name used in the patch > strips such a/, b/ prefixes to get the existing file name. Not in my testing, but maybe I tried in the wrong Emacs version. Is this feature new with Emacs 27? > @@ -2008,8 +2008,7 @@ vc-find-revision-no-save > (with-current-buffer filebuf > (let ((failed t)) > (unwind-protect > - (let ((coding-system-for-read 'no-conversion) > - (coding-system-for-write 'no-conversion)) > + (let ((coding-system-for-read 'no-conversion)) > (with-current-buffer (create-file-buffer filename) > (setq buffer-file-name filename) > (let ((outbuf (current-buffer))) > @@ -2019,6 +2018,9 @@ vc-find-revision-no-save > (vc-call find-revision file revision outbuf)))) > (goto-char (point-min)) > (normal-mode) > + (recode-region (point-min) (point-max) > + (car (detect-coding-region (point-min) (point-max))) > + 'no-conversion) See above: I think this manual decoding is unnecessary. > +(defcustom diff-font-lock-syntax 'vc > + "If non-nil, diff hunk's font-lock includes language syntax highlighting. > +This highlighting is the same as added by `font-lock-mode' when > +corresponding source files are visited from the diff buffer. Thanks, this is much better than the original text, but there are still unclear corners. One such corner is the "visited from the diff buffer" part -- what is its significance? Can we just say "when the corresponding source files are visited normally"? > +In diff hunks syntax highlighting is added over diff own > +highlighted changes. What is the significance of the "In diff hunks" part here? Apart of diff hunks, we have just headers, where this feature is irrelevant, right? > +If `vc', highlight syntax only in Diff buffers created by a version control > +system that provides all necessary context for reliable highlighting. I would use "in Diff buffers created by VC commands" instead. I would also add this text (assuming it is correct): This value requires support from a VC backend to find the files being compared. This should tell users that they could in principle set up things manually even for buffers that were not created by VC commands. Please also indicate that `vc' is the default. > +For working revisions get highlighting according to the working > +copy of the file. I don't understand the significance of this comment. If you want to say that the produced highlighting might be wrong if the working version has changed since it was compared, then let's say that explicitly. > +If t, additionally to trying to use a version control system to get > +old revisions for fontification, also try to get fontification based > +on existing files, and on failure get fontification from hunk alone." What is the difference between using a VCS to get old revisions, and using existing files? Also, does it mean `vc' will not fall back to `hunk-only'? Why not? Thanks.
bug-gnu-emacs <at> gnu.org:bug#33567; Package emacs.
(Mon, 03 Dec 2018 11:25:02 GMT) Full text and rfc822 format available.Message #17 received at 33567 <at> debbugs.gnu.org (full text, mbox):
From: Dmitry Gutov <dgutov <at> yandex.ru> To: Juri Linkov <juri <at> linkov.net>, Eli Zaretskii <eliz <at> gnu.org> Cc: 33567 <at> debbugs.gnu.org Subject: Re: bug#33567: Syntactic fontification of diff hunks Date: Mon, 3 Dec 2018 13:24:40 +0200
On 03.12.2018 2:34, Juri Linkov wrote: > Fixing the described problem will remove this comment, > but I have no idea how better to do this. The problem is that > we need to provide own created buffer to the call to `vc-find-revision'. > Currently it has the following function signature: > > (defun vc-find-revision (file revision &optional backend) > > But VC API in the comments in the beginning of vc.el > is documented with a different function signature: > > ;; * find-revision (file rev buffer) > ;; > ;; Fetch revision REV of file FILE and put it into BUFFER. > ;; If REV is the empty string, fetch the head of the trunk. > ;; The implementation should pass the value of vc-checkout-switches > ;; to the backend command. That is the VC backend API, not the signatures of public functions in vc.el (which would be kinda pointless). vc-git-find-revision and friends do indeed have this signature. Take a look at the uses of vc-call-backend in vc-find-revision[-no]-save. Maybe you need to write a new ???-find-revision function that does what you need.
bug-gnu-emacs <at> gnu.org:bug#33567; Package emacs.
(Tue, 04 Dec 2018 00:06:02 GMT) Full text and rfc822 format available.Message #20 received at 33567 <at> debbugs.gnu.org (full text, mbox):
From: Juri Linkov <juri <at> linkov.net> To: Dmitry Gutov <dgutov <at> yandex.ru> Cc: Eli Zaretskii <eliz <at> gnu.org>, 33567 <at> debbugs.gnu.org Subject: Re: bug#33567: Syntactic fontification of diff hunks Date: Tue, 04 Dec 2018 01:24:52 +0200
[Message part 1 (text/plain, inline)]
>> Fixing the described problem will remove this comment, >> but I have no idea how better to do this. The problem is that >> we need to provide own created buffer to the call to `vc-find-revision'. >> Currently it has the following function signature: >> >> (defun vc-find-revision (file revision &optional backend) >> >> But VC API in the comments in the beginning of vc.el >> is documented with a different function signature: >> >> ;; * find-revision (file rev buffer) >> ;; >> ;; Fetch revision REV of file FILE and put it into BUFFER. >> ;; If REV is the empty string, fetch the head of the trunk. >> ;; The implementation should pass the value of vc-checkout-switches >> ;; to the backend command. > > That is the VC backend API, not the signatures of public functions in vc.el > (which would be kinda pointless). > > vc-git-find-revision and friends do indeed have this signature. > > Take a look at the uses of vc-call-backend in > vc-find-revision[-no]-save. Maybe you need to write a new ???-find-revision > function that does what you need. Thanks, I see now that we can freely add a new optional arg:
[vc-find-revision-no-save-buffer.patch (text/x-diff, inline)]
diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index de43544864..4976deefef 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -1998,33 +1998,37 @@ vc-find-revision-save
(set (make-local-variable 'vc-parent-buffer) filebuf))
result-buf)))
-(defun vc-find-revision-no-save (file revision &optional backend)
- "Read REVISION of FILE into a buffer and return the buffer.
+(defun vc-find-revision-no-save (file revision &optional backend buffer)
+ "Read REVISION of FILE into a BUFFER and return the buffer.
Unlike `vc-find-revision-save', doesn't save the created buffer to file."
- (let ((filebuf (or (get-file-buffer file) (current-buffer)))
- (filename (vc-version-backup-file-name file revision 'manual)))
- (unless (or (get-file-buffer filename)
- (file-exists-p filename))
+ (let ((filebuf (or buffer (get-file-buffer file) (current-buffer)))
+ (filename (unless buffer (vc-version-backup-file-name file revision 'manual))))
+ (unless (and (not buffer)
+ (or (get-file-buffer filename)
+ (file-exists-p filename)))
(with-current-buffer filebuf
(let ((failed t))
(unwind-protect
(let ((coding-system-for-read 'no-conversion)
(coding-system-for-write 'no-conversion))
- (with-current-buffer (create-file-buffer filename)
- (setq buffer-file-name filename)
+ (with-current-buffer (or buffer (create-file-buffer filename))
+ (unless buffer (setq buffer-file-name filename))
(let ((outbuf (current-buffer)))
(with-current-buffer filebuf
(if backend
(vc-call-backend backend 'find-revision file revision outbuf)
(vc-call find-revision file revision outbuf))))
(goto-char (point-min))
- (normal-mode)
+ (if buffer (let ((buffer-file-name file)) (normal-mode)) (normal-mode))
(set-buffer-modified-p nil)
(setq buffer-read-only t))
(setq failed nil))
- (when (and failed (get-file-buffer filename))
+ (when (and failed (unless buffer (get-file-buffer filename)))
+ (with-current-buffer (get-file-buffer filename)
+ (set-buffer-modified-p nil))
(kill-buffer (get-file-buffer filename)))))))
- (let ((result-buf (or (get-file-buffer filename)
+ (let ((result-buf (or buffer
+ (get-file-buffer filename)
(find-file-noselect filename))))
(with-current-buffer result-buf
(set (make-local-variable 'vc-parent-buffer) filebuf))
bug-gnu-emacs <at> gnu.org:bug#33567; Package emacs.
(Tue, 04 Dec 2018 00:07:02 GMT) Full text and rfc822 format available.Message #23 received at 33567 <at> debbugs.gnu.org (full text, mbox):
From: Juri Linkov <juri <at> linkov.net> To: Eli Zaretskii <eliz <at> gnu.org> Cc: 33567 <at> debbugs.gnu.org Subject: Re: bug#33567: Syntactic fontification of diff hunks Date: Tue, 04 Dec 2018 01:36:52 +0200
>> BTW, while deciding what to do with this, could you please confirm >> if I correctly fixed another problem in vc-find-revision-no-save. >> Recently in bug#33319 I added this function but now discovered >> a problem with encoding. A vc process outputs lines to the buffer >> with no-conversion, so in the patch below I added recode-region >> to convert output to the buffer's encoding. coding-system-for-write >> that I removed was copied from vc-find-revision-save where is was >> needed for write-region called from the macro with-temp-file, >> but vc-find-revision-no-save doesn't write output to the file. > > vc-find-revision disables encoding/decoding because it wants to > create an identical copy of the checked-out file, and doesn't want to > be tripped by encoding/decoding issues. But in your case you don't > write the buffer to a file, so why do you need to bind > coding-system-for-read at all? I say leave it unbound, and let Emacs > do its job decoding the text as usual. Does that not work? I tried to remove coding-system-for-read binding from vc-find-revision-no-save, but it still fails to get the buffer in the right encoding. Then I discovered that vc-git-find-revision and also some other VC backend API implementations of find-revision bind coding-system-for-read too. It seems that removing coding-system-for-read from vc-git-find-revision will cause a lot of breakage.
bug-gnu-emacs <at> gnu.org:bug#33567; Package emacs.
(Tue, 04 Dec 2018 00:07:02 GMT) Full text and rfc822 format available.Message #26 received at 33567 <at> debbugs.gnu.org (full text, mbox):
From: Juri Linkov <juri <at> linkov.net> To: Eli Zaretskii <eliz <at> gnu.org> Cc: 33567 <at> debbugs.gnu.org Subject: Re: bug#33567: Syntactic fontification of diff hunks Date: Tue, 04 Dec 2018 01:59:07 +0200
>> As a general solution is should be possible to specify the default >> directory in the local variables at the first line of the diff files >> as currently already is used in compilation/grep buffers like >> >> -*- mode: diff-mode; default-directory: "..." -*- > > This is all fine, but I think we should document that files are > visited relative to default-directory of the buffer, so that users > could invoke "cd" to change that as needed. For more safety, I propose to set a new buffer-local variable `diff-default-directory' by such commands as diff, diff-backup, dired-diff, dired-backup-diff. The existence of such variable should guarantee that the referenced files really exist. This variable will be like `diff-vc-backend' that says that the diff-mode buffer is created by the VCS command. Then anyone who want to visit a diff file in another directory, could add it to the first line: -*- mode: diff-mode; diff-default-directory: "..." -*- >> > Also, if the diffs are from Git, they begin with a/, b/, etc. dummy >> > directories, which usually don't exist in the file system. >> >> This is not a problem because diff-find-file-name used in the patch >> strips such a/, b/ prefixes to get the existing file name. > > Not in my testing, but maybe I tried in the wrong Emacs version. Is > this feature new with Emacs 27? For testing better try to eval e.g. `(diff-find-file-name nil t)' on a hunk in a diff-mode buffer created by git. >> +(defcustom diff-font-lock-syntax 'vc >> + "If non-nil, diff hunk's font-lock includes language syntax highlighting. >> +This highlighting is the same as added by `font-lock-mode' when >> +corresponding source files are visited from the diff buffer. > > Thanks, this is much better than the original text, but there are > still unclear corners. One such corner is the "visited from the diff > buffer" part -- what is its significance? Can we just say "when the > corresponding source files are visited normally"? Changed, will send a patch with more changes later. >> +In diff hunks syntax highlighting is added over diff own >> +highlighted changes. > > What is the significance of the "In diff hunks" part here? Apart of > diff hunks, we have just headers, where this feature is irrelevant, > right? Removed "In diff hunks". >> +If `vc', highlight syntax only in Diff buffers created by a version control >> +system that provides all necessary context for reliable highlighting. > > I would use "in Diff buffers created by VC commands" instead. I would > also add this text (assuming it is correct): > > This value requires support from a VC backend to find the files > being compared. > > This should tell users that they could in principle set up things > manually even for buffers that were not created by VC commands. Changed. > Please also indicate that `vc' is the default. After adding another safe option that uses `diff-default-directory', we could combine it with `vc' for the safe default. >> +For working revisions get highlighting according to the working >> +copy of the file. > > I don't understand the significance of this comment. If you want to > say that the produced highlighting might be wrong if the working > version has changed since it was compared, then let's say that > explicitly. This means that working revisions can't be extracted from the repository. Until committed, they reside in files that are visited with find-file. >> +If t, additionally to trying to use a version control system to get >> +old revisions for fontification, also try to get fontification based >> +on existing files, and on failure get fontification from hunk alone." > > What is the difference between using a VCS to get old revisions, and > using existing files? This means that when a diff-mode buffer is not created by a VCS, then it tries to read files with find-file. > Also, does it mean `vc' will not fall back to `hunk-only'? Why not? Actually, it already falls back to `hunk-only', this is what "on failure get fontification from hunk alone." tries to say.
bug-gnu-emacs <at> gnu.org:bug#33567; Package emacs.
(Tue, 04 Dec 2018 00:21:01 GMT) Full text and rfc822 format available.Message #29 received at 33567 <at> debbugs.gnu.org (full text, mbox):
From: Dmitry Gutov <dgutov <at> yandex.ru> To: Juri Linkov <juri <at> linkov.net> Cc: 33567 <at> debbugs.gnu.org Subject: Re: bug#33567: Syntactic fontification of diff hunks Date: Tue, 4 Dec 2018 02:20:05 +0200
On 04.12.2018 1:24, Juri Linkov wrote: > Thanks, I see now that we can freely add a new optional arg: *shrug* or that. > Unlike `vc-find-revision-save', doesn't save the created buffer to file." This part of the docstring will probably need to be updated, too.
bug-gnu-emacs <at> gnu.org:bug#33567; Package emacs.
(Tue, 04 Dec 2018 06:48:02 GMT) Full text and rfc822 format available.Message #32 received at 33567 <at> debbugs.gnu.org (full text, mbox):
From: Eli Zaretskii <eliz <at> gnu.org> To: Juri Linkov <juri <at> linkov.net> Cc: 33567 <at> debbugs.gnu.org, dgutov <at> yandex.ru Subject: Re: bug#33567: Syntactic fontification of diff hunks Date: Tue, 04 Dec 2018 08:46:46 +0200
> From: Juri Linkov <juri <at> linkov.net>
> Cc: Eli Zaretskii <eliz <at> gnu.org>, 33567 <at> debbugs.gnu.org
> Date: Tue, 04 Dec 2018 01:24:52 +0200
>
> -(defun vc-find-revision-no-save (file revision &optional backend)
> - "Read REVISION of FILE into a buffer and return the buffer.
> +(defun vc-find-revision-no-save (file revision &optional backend buffer)
> + "Read REVISION of FILE into a BUFFER and return the buffer.
Since you use BUFFER, please drop the "a" part.
Also, the doc string should tell what BUFFER defaults to if omitted or
nil.
> + (let ((filebuf (or buffer (get-file-buffer file) (current-buffer)))
^^^^^^^^^
I would use bufferp here. What if someone calls the function with
something that is not a buffer? And, btw, are buffer names allowed?
Similarly with other places where you make the same test (why not make
it once and bind some local variable to the result, btw?).
Thanks.
bug-gnu-emacs <at> gnu.org:bug#33567; Package emacs.
(Tue, 04 Dec 2018 07:02:02 GMT) Full text and rfc822 format available.Message #35 received at 33567 <at> debbugs.gnu.org (full text, mbox):
From: Eli Zaretskii <eliz <at> gnu.org> To: Juri Linkov <juri <at> linkov.net> Cc: 33567 <at> debbugs.gnu.org Subject: Re: bug#33567: Syntactic fontification of diff hunks Date: Tue, 04 Dec 2018 09:01:24 +0200
> From: Juri Linkov <juri <at> linkov.net> > Cc: 33567 <at> debbugs.gnu.org > Date: Tue, 04 Dec 2018 01:36:52 +0200 > > > vc-find-revision disables encoding/decoding because it wants to > > create an identical copy of the checked-out file, and doesn't want to > > be tripped by encoding/decoding issues. But in your case you don't > > write the buffer to a file, so why do you need to bind > > coding-system-for-read at all? I say leave it unbound, and let Emacs > > do its job decoding the text as usual. Does that not work? > > I tried to remove coding-system-for-read binding from > vc-find-revision-no-save, but it still fails to get the buffer > in the right encoding. What is "the right encoding", and what did Emacs think the encoding was, when you didn't bind coding-system-for-read? These details are necessary to understand what exactly happens there and how to solve it. > Then I discovered that vc-git-find-revision and also some other VC > backend API implementations of find-revision bind > coding-system-for-read too. It seems that removing > coding-system-for-read from vc-git-find-revision will cause a lot of > breakage. How do you know vc-git-find-revision doesn't have a subtle bug as well, e.g. when file names in the repository are encoded in some non-trivial, non-UTF-8 encoding? And anyway, we are not talking about changing vc-git-find-revision or affecting it, we are talking about your vc-find-revision-no-save, which does a different job. For the latter, I'd prefer not to decode by hand, as that might have subtle issues and will require much more testing in all kinds of environments and OSes. I prefer to rely on the usual decoding machinery, which we know works well.
bug-gnu-emacs <at> gnu.org:bug#33567; Package emacs.
(Tue, 04 Dec 2018 07:38:02 GMT) Full text and rfc822 format available.Message #38 received at 33567 <at> debbugs.gnu.org (full text, mbox):
From: Eli Zaretskii <eliz <at> gnu.org> To: Juri Linkov <juri <at> linkov.net> Cc: 33567 <at> debbugs.gnu.org Subject: Re: bug#33567: Syntactic fontification of diff hunks Date: Tue, 04 Dec 2018 09:36:55 +0200
> From: Juri Linkov <juri <at> linkov.net> > Cc: 33567 <at> debbugs.gnu.org > Date: Tue, 04 Dec 2018 01:59:07 +0200 > > >> As a general solution is should be possible to specify the default > >> directory in the local variables at the first line of the diff files > >> as currently already is used in compilation/grep buffers like > >> > >> -*- mode: diff-mode; default-directory: "..." -*- > > > > This is all fine, but I think we should document that files are > > visited relative to default-directory of the buffer, so that users > > could invoke "cd" to change that as needed. > > For more safety, I propose to set a new buffer-local variable > `diff-default-directory' by such commands as diff, diff-backup, > dired-diff, dired-backup-diff. The existence of such variable > should guarantee that the referenced files really exist. > This variable will be like `diff-vc-backend' that says that > the diff-mode buffer is created by the VCS command. > Then anyone who want to visit a diff file in another directory, > could add it to the first line: > > -*- mode: diff-mode; diff-default-directory: "..." -*- I'm not sure this is a step in the right direction. What is the advantage of having a separate variable? How is it "safer"? The advantage of default-directory is that it is well-known, and the command to change it, 'cd', is easier to type and invoke than setting diff-default-directory manually. > > >> > Also, if the diffs are from Git, they begin with a/, b/, etc. dummy > >> > directories, which usually don't exist in the file system. > >> > >> This is not a problem because diff-find-file-name used in the patch > >> strips such a/, b/ prefixes to get the existing file name. > > > > Not in my testing, but maybe I tried in the wrong Emacs version. Is > > this feature new with Emacs 27? > > For testing better try to eval e.g. `(diff-find-file-name nil t)' > on a hunk in a diff-mode buffer created by git. I did, but I guess this must be done inside the repository to work, does it? If I put the output of "git diff" on a file in some arbitrary directory, then visit that file and evaluate (diff-find-file-name nil t), I get nil. > >> +For working revisions get highlighting according to the working > >> +copy of the file. > > > > I don't understand the significance of this comment. If you want to > > say that the produced highlighting might be wrong if the working > > version has changed since it was compared, then let's say that > > explicitly. > > This means that working revisions can't be extracted from the repository. > Until committed, they reside in files that are visited with find-file. We need to describe the implications of that to the users. Does the following text capture the issue? For diffs against the working-tree version of a file, the highlighting is based on the current file contents, which could be different from the contents when the diffs were taken. In such cases, the produced highlighting might be wrong. > >> +If t, additionally to trying to use a version control system to get > >> +old revisions for fontification, also try to get fontification based > >> +on existing files, and on failure get fontification from hunk alone." > > > > What is the difference between using a VCS to get old revisions, and > > using existing files? > > This means that when a diff-mode buffer is not created by a VCS, > then it tries to read files with find-file. If so, I suggest the following wording: If t, try to infer fontification from the compared files, if they exist in the filesystem, when accessing their contents via VC fails. > > Also, does it mean `vc' will not fall back to `hunk-only'? Why not? > > Actually, it already falls back to `hunk-only', this is what > "on failure get fontification from hunk alone." tries to say. There's no such text in the description of 'vc', only in the description of t, which is why I asked.
bug-gnu-emacs <at> gnu.org:bug#33567; Package emacs.
(Wed, 05 Dec 2018 00:15:02 GMT) Full text and rfc822 format available.Message #41 received at 33567 <at> debbugs.gnu.org (full text, mbox):
From: Juri Linkov <juri <at> linkov.net> To: Eli Zaretskii <eliz <at> gnu.org> Cc: 33567 <at> debbugs.gnu.org, dgutov <at> yandex.ru Subject: Re: bug#33567: Syntactic fontification of diff hunks Date: Wed, 05 Dec 2018 00:58:49 +0200
[Message part 1 (text/plain, inline)]
>> -(defun vc-find-revision-no-save (file revision &optional backend) >> - "Read REVISION of FILE into a buffer and return the buffer. >> +(defun vc-find-revision-no-save (file revision &optional backend buffer) >> + "Read REVISION of FILE into a BUFFER and return the buffer. > > Since you use BUFFER, please drop the "a" part. > > Also, the doc string should tell what BUFFER defaults to if omitted or > nil. Updated. >> + (let ((filebuf (or buffer (get-file-buffer file) (current-buffer))) > ^^^^^^^^^ > I would use bufferp here. What if someone calls the function with > something that is not a buffer? Added buffer-live-p. > And, btw, are buffer names allowed? I see that most VC functions work with buffer objects only. So buffer names could be allowed only when really necessary. > Similarly with other places where you make the same test (why not make > it once and bind some local variable to the result, btw?). Done:
[vc-find-revision-no-save-buffer.2.patch (text/x-diff, inline)]
diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index de43544864..dbbc3e2038 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -1998,33 +1998,41 @@ vc-find-revision-save
(set (make-local-variable 'vc-parent-buffer) filebuf))
result-buf)))
-(defun vc-find-revision-no-save (file revision &optional backend)
- "Read REVISION of FILE into a buffer and return the buffer.
-Unlike `vc-find-revision-save', doesn't save the created buffer to file."
- (let ((filebuf (or (get-file-buffer file) (current-buffer)))
- (filename (vc-version-backup-file-name file revision 'manual)))
- (unless (or (get-file-buffer filename)
- (file-exists-p filename))
+(defun vc-find-revision-no-save (file revision &optional backend buffer)
+ "Read REVISION of FILE into BUFFER and return the buffer.
+If BUFFER omitted or nil, this function creates a new buffer and sets
+`buffer-file-name' to the name constructed from the file name and the
+revision number.
+Unlike `vc-find-revision-save', doesn't save the buffer to the file."
+ (let* ((buffer (when (buffer-live-p buffer) buffer))
+ (filebuf (or buffer (get-file-buffer file) (current-buffer)))
+ (filename (unless buffer (vc-version-backup-file-name file revision 'manual))))
+ (unless (and (not buffer)
+ (or (get-file-buffer filename)
+ (file-exists-p filename)))
(with-current-buffer filebuf
(let ((failed t))
(unwind-protect
(let ((coding-system-for-read 'no-conversion)
- (coding-system-for-write 'no-conversion))
- (with-current-buffer (create-file-buffer filename)
- (setq buffer-file-name filename)
+ (coding-system-for-write 'no-conversion))
+ (with-current-buffer (or buffer (create-file-buffer filename))
+ (unless buffer (setq buffer-file-name filename))
(let ((outbuf (current-buffer)))
(with-current-buffer filebuf
(if backend
(vc-call-backend backend 'find-revision file revision outbuf)
(vc-call find-revision file revision outbuf))))
(goto-char (point-min))
- (normal-mode)
+ (if buffer (let ((buffer-file-name file)) (normal-mode)) (normal-mode))
(set-buffer-modified-p nil)
(setq buffer-read-only t))
(setq failed nil))
- (when (and failed (get-file-buffer filename))
+ (when (and failed (unless buffer (get-file-buffer filename)))
+ (with-current-buffer (get-file-buffer filename)
+ (set-buffer-modified-p nil))
(kill-buffer (get-file-buffer filename)))))))
- (let ((result-buf (or (get-file-buffer filename)
+ (let ((result-buf (or buffer
+ (get-file-buffer filename)
(find-file-noselect filename))))
(with-current-buffer result-buf
(set (make-local-variable 'vc-parent-buffer) filebuf))
bug-gnu-emacs <at> gnu.org:bug#33567; Package emacs.
(Wed, 05 Dec 2018 00:15:02 GMT) Full text and rfc822 format available.Message #44 received at 33567 <at> debbugs.gnu.org (full text, mbox):
From: Juri Linkov <juri <at> linkov.net> To: Eli Zaretskii <eliz <at> gnu.org> Cc: 33567 <at> debbugs.gnu.org Subject: Re: bug#33567: Syntactic fontification of diff hunks Date: Wed, 05 Dec 2018 01:16:48 +0200
>> > vc-find-revision disables encoding/decoding because it wants to >> > create an identical copy of the checked-out file, and doesn't want to >> > be tripped by encoding/decoding issues. But in your case you don't >> > write the buffer to a file, so why do you need to bind >> > coding-system-for-read at all? I say leave it unbound, and let Emacs >> > do its job decoding the text as usual. Does that not work? >> >> I tried to remove coding-system-for-read binding from >> vc-find-revision-no-save, but it still fails to get the buffer >> in the right encoding. > > What is "the right encoding", By the right encoding I meant the same encoding that is detected when write-region saves the file, e.g. when using the macro with-temp-file in vc-find-revision-save. I don't know how write-region detects the encoding for the saved file, but we need the same encoding for the buffer that is not saved to the file in vc-find-revision-no-save. > and what did Emacs think the encoding was, when you didn't bind > coding-system-for-read? These details are necessary to understand > what exactly happens there and how to solve it. vc-git-find-revision binds coding-system-for-read to `binary'. >> Then I discovered that vc-git-find-revision and also some other VC >> backend API implementations of find-revision bind >> coding-system-for-read too. It seems that removing >> coding-system-for-read from vc-git-find-revision will cause a lot of >> breakage. > > How do you know vc-git-find-revision doesn't have a subtle bug as > well, e.g. when file names in the repository are encoded in some > non-trivial, non-UTF-8 encoding? This is why vc-git-find-revision does nothing with its output when it binds coding-system-for-read to `binary', and doesn't try to encode/decode the git output. > And anyway, we are not talking about changing vc-git-find-revision or > affecting it, we are talking about your vc-find-revision-no-save, > which does a different job. For the latter, I'd prefer not to decode > by hand, as that might have subtle issues and will require much more > testing in all kinds of environments and OSes. vc-git-find-revision returns the output undecoded. I don't know other way to decode it to the default coding other than recode-region. > I prefer to rely on the usual decoding machinery, which we know > works well. Maybe I missed a separate function that can use the decoding machinery like is used in write-region, but without writing a buffer to the file?
bug-gnu-emacs <at> gnu.org:bug#33567; Package emacs.
(Wed, 05 Dec 2018 00:15:03 GMT) Full text and rfc822 format available.Message #47 received at 33567 <at> debbugs.gnu.org (full text, mbox):
From: Juri Linkov <juri <at> linkov.net> To: Eli Zaretskii <eliz <at> gnu.org> Cc: 33567 <at> debbugs.gnu.org Subject: Re: bug#33567: Syntactic fontification of diff hunks Date: Wed, 05 Dec 2018 01:28:45 +0200
>> For more safety, I propose to set a new buffer-local variable >> `diff-default-directory' by such commands as diff, diff-backup, >> dired-diff, dired-backup-diff. The existence of such variable >> should guarantee that the referenced files really exist. >> This variable will be like `diff-vc-backend' that says that >> the diff-mode buffer is created by the VCS command. >> Then anyone who want to visit a diff file in another directory, >> could add it to the first line: >> >> -*- mode: diff-mode; diff-default-directory: "..." -*- > > I'm not sure this is a step in the right direction. What is the > advantage of having a separate variable? How is it "safer"? When this special variable is set by a diff command, it's safe to assume that the files referenced from the diff buffer really exist, so it's safe to try reading them. I don't want a patch in a mail attachment to try reading files mentioned in the patch attachment. >> >> > Also, if the diffs are from Git, they begin with a/, b/, etc. dummy >> >> > directories, which usually don't exist in the file system. >> >> >> >> This is not a problem because diff-find-file-name used in the patch >> >> strips such a/, b/ prefixes to get the existing file name. >> > >> > Not in my testing, but maybe I tried in the wrong Emacs version. Is >> > this feature new with Emacs 27? >> >> For testing better try to eval e.g. `(diff-find-file-name nil t)' >> on a hunk in a diff-mode buffer created by git. > > I did, but I guess this must be done inside the repository to work, > does it? If I put the output of "git diff" on a file in some > arbitrary directory, then visit that file and evaluate > (diff-find-file-name nil t), I get nil. Yes, it finds only the existing files inside the repository. >> >> +For working revisions get highlighting according to the working >> >> +copy of the file. >> > >> > I don't understand the significance of this comment. If you want to >> > say that the produced highlighting might be wrong if the working >> > version has changed since it was compared, then let's say that >> > explicitly. >> >> This means that working revisions can't be extracted from the repository. >> Until committed, they reside in files that are visited with find-file. > > We need to describe the implications of that to the users. Does the > following text capture the issue? > > For diffs against the working-tree version of a file, the > highlighting is based on the current file contents, which could be > different from the contents when the diffs were taken. In such > cases, the produced highlighting might be wrong. Such problem is very rare because highlighting is added usually immediately after creating a diff. When the file contents changes, there is no highlighting at all - it verifies if text of the hunk exist in the file, so highlighting never is wrong. >> >> +If t, additionally to trying to use a version control system to get >> >> +old revisions for fontification, also try to get fontification based >> >> +on existing files, and on failure get fontification from hunk alone." >> > >> > What is the difference between using a VCS to get old revisions, and >> > using existing files? >> >> This means that when a diff-mode buffer is not created by a VCS, >> then it tries to read files with find-file. > > If so, I suggest the following wording: > > If t, try to infer fontification from the compared files, if they > exist in the filesystem, when accessing their contents via VC > fails. Will add in the final patch. >> > Also, does it mean `vc' will not fall back to `hunk-only'? Why not? >> >> Actually, it already falls back to `hunk-only', this is what >> "on failure get fontification from hunk alone." tries to say. > > There's no such text in the description of 'vc', only in the > description of t, which is why I asked. Maybe then better to add text common for all cases, e.g. "If some method fails, get fontification from hunk alone."
bug-gnu-emacs <at> gnu.org:bug#33567; Package emacs.
(Wed, 05 Dec 2018 07:21:02 GMT) Full text and rfc822 format available.Message #50 received at 33567 <at> debbugs.gnu.org (full text, mbox):
From: Eli Zaretskii <eliz <at> gnu.org> To: Juri Linkov <juri <at> linkov.net> Cc: 33567 <at> debbugs.gnu.org Subject: Re: bug#33567: Syntactic fontification of diff hunks Date: Wed, 05 Dec 2018 09:19:55 +0200
> From: Juri Linkov <juri <at> linkov.net> > Cc: 33567 <at> debbugs.gnu.org > Date: Wed, 05 Dec 2018 01:16:48 +0200 > > >> I tried to remove coding-system-for-read binding from > >> vc-find-revision-no-save, but it still fails to get the buffer > >> in the right encoding. > > > > What is "the right encoding", > > By the right encoding I meant the same encoding that is detected > when write-region saves the file, e.g. when using the macro > with-temp-file in vc-find-revision-save. I don't know how > write-region detects the encoding for the saved file, but we need > the same encoding for the buffer that is not saved to the file > in vc-find-revision-no-save. > > > and what did Emacs think the encoding was, when you didn't bind > > coding-system-for-read? These details are necessary to understand > > what exactly happens there and how to solve it. > > vc-git-find-revision binds coding-system-for-read to `binary'. I see that vc-hg-find-revision does the same. Sigh. I guess the find-revision API was never meant to process the resulting buffer normally. My advice would be to reimplement your vc-find-revision-no-save function differently, without trying to piggy-back the fact that vc-find-revision inserts the contents into a buffer. That is, let the code write the contents to a temporary file, like vc-find-revision does, then call insert-file-contents to re-read that file normally. It would be slightly less efficient, but I think the result will be much simpler, so a net win. If you still want to reuse the literal contents of the file, as inserted by vc-git-find-revision etc., then you will have to duplicate what insert-file-contents does internally. I suggest to look at how this is done in archive-set-buffer-as-visiting-file. > > How do you know vc-git-find-revision doesn't have a subtle bug as > > well, e.g. when file names in the repository are encoded in some > > non-trivial, non-UTF-8 encoding? > > This is why vc-git-find-revision does nothing with its output > when it binds coding-system-for-read to `binary', > and doesn't try to encode/decode the git output. vc-git-find-revision does _something_ with Git's output: it uses the file name returned by Git. That file name could have a non-trivial encoding. > vc-git-find-revision returns the output undecoded. I don't know > other way to decode it to the default coding other than recode-region. See above.
bug-gnu-emacs <at> gnu.org:bug#33567; Package emacs.
(Wed, 05 Dec 2018 07:26:02 GMT) Full text and rfc822 format available.Message #53 received at 33567 <at> debbugs.gnu.org (full text, mbox):
From: Eli Zaretskii <eliz <at> gnu.org> To: Juri Linkov <juri <at> linkov.net> Cc: 33567 <at> debbugs.gnu.org Subject: Re: bug#33567: Syntactic fontification of diff hunks Date: Wed, 05 Dec 2018 09:25:22 +0200
> From: Juri Linkov <juri <at> linkov.net> > Cc: 33567 <at> debbugs.gnu.org > Date: Wed, 05 Dec 2018 01:28:45 +0200 > > >> For more safety, I propose to set a new buffer-local variable > >> `diff-default-directory' by such commands as diff, diff-backup, > >> dired-diff, dired-backup-diff. The existence of such variable > >> should guarantee that the referenced files really exist. > >> This variable will be like `diff-vc-backend' that says that > >> the diff-mode buffer is created by the VCS command. > >> Then anyone who want to visit a diff file in another directory, > >> could add it to the first line: > >> > >> -*- mode: diff-mode; diff-default-directory: "..." -*- > > > > I'm not sure this is a step in the right direction. What is the > > advantage of having a separate variable? How is it "safer"? > > When this special variable is set by a diff command, it's safe to assume > that the files referenced from the diff buffer really exist, so it's > safe to try reading them. I don't want a patch in a mail attachment > to try reading files mentioned in the patch attachment. I think you lean heavily towards the use case where the diffs were produced by some Emacs command. Whereas what I have in mind is the use case where the diffs came from some other source, like manual invocation of shell commands. I'm saying that in the absence of diff-default-directory, using default-directory and 'cd' will be much easier for users. > >> This means that working revisions can't be extracted from the repository. > >> Until committed, they reside in files that are visited with find-file. > > > > We need to describe the implications of that to the users. Does the > > following text capture the issue? > > > > For diffs against the working-tree version of a file, the > > highlighting is based on the current file contents, which could be > > different from the contents when the diffs were taken. In such > > cases, the produced highlighting might be wrong. > > Such problem is very rare because highlighting is added usually > immediately after creating a diff. When the file contents changes, > there is no highlighting at all - it verifies if text of the hunk > exist in the file, so highlighting never is wrong. The use case I have in mind is that some time passes between the generation of the diffs and the time the diffs are visited and fontified. During that time, the working revision could have been changed. Isn't that what this issue is about? If so, we should explain why in that case, rare as it could be, the fontification might be wrong. > >> > Also, does it mean `vc' will not fall back to `hunk-only'? Why not? > >> > >> Actually, it already falls back to `hunk-only', this is what > >> "on failure get fontification from hunk alone." tries to say. > > > > There's no such text in the description of 'vc', only in the > > description of t, which is why I asked. > > Maybe then better to add text common for all cases, e.g. > > "If some method fails, get fontification from hunk alone." That'd be fine as well. Thanks.
bug-gnu-emacs <at> gnu.org:bug#33567; Package emacs.
(Thu, 06 Dec 2018 00:57:03 GMT) Full text and rfc822 format available.Message #56 received at 33567 <at> debbugs.gnu.org (full text, mbox):
From: Juri Linkov <juri <at> linkov.net> To: Eli Zaretskii <eliz <at> gnu.org> Cc: 33567 <at> debbugs.gnu.org Subject: Re: bug#33567: Syntactic fontification of diff hunks Date: Thu, 06 Dec 2018 01:25:46 +0200
>> vc-git-find-revision binds coding-system-for-read to `binary'. > > I see that vc-hg-find-revision does the same. Sigh. I guess the > find-revision API was never meant to process the resulting buffer > normally. My advice would be to reimplement your > vc-find-revision-no-save function differently, without trying to > piggy-back the fact that vc-find-revision inserts the contents into a > buffer. That is, let the code write the contents to a temporary file, > like vc-find-revision does, then call insert-file-contents to re-read > that file normally. It would be slightly less efficient, but I think > the result will be much simpler, so a net win. The whole purpose of creating vc-find-revision-no-save function was to improve the performance of vc-find-revision-save to avoid the need to write files. It would significantly degrade performance of diff syntax fontification if it will write files for every hunk. > If you still want to reuse the literal contents of the file, as > inserted by vc-git-find-revision etc., then you will have to duplicate > what insert-file-contents does internally. I suggest to look at how > this is done in archive-set-buffer-as-visiting-file. I see it does something like I was trying to do. I will use it when failing to use the third possible solution I proposed below. >> > How do you know vc-git-find-revision doesn't have a subtle bug as >> > well, e.g. when file names in the repository are encoded in some >> > non-trivial, non-UTF-8 encoding? >> >> This is why vc-git-find-revision does nothing with its output >> when it binds coding-system-for-read to `binary', >> and doesn't try to encode/decode the git output. > > vc-git-find-revision does _something_ with Git's output: it uses the > file name returned by Git. That file name could have a non-trivial > encoding. I'm thinking about overriding coding in vc-git-find-revision like diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el index f317400530..e5f44524df 100644 --- a/lisp/vc/vc-git.el +++ b/lisp/vc/vc-git.el @@ -838,8 +838,8 @@ vc-git-checkin (defun vc-git-find-revision (file rev buffer) (let* (process-file-side-effects - (coding-system-for-read 'binary) - (coding-system-for-write 'binary) + (coding-system-for-read (or coding-system-for-read 'binary)) + (coding-system-for-write (or coding-system-for-write 'binary)) (fullname (let ((fn (vc-git--run-command-string file "ls-files" "-z" "--full-name" "--"))) then a caller function could set its own value of this dynamic binding. But I haven't tested yet if it works with UTF-8 file names.
bug-gnu-emacs <at> gnu.org:bug#33567; Package emacs.
(Thu, 06 Dec 2018 00:58:02 GMT) Full text and rfc822 format available.Message #59 received at 33567 <at> debbugs.gnu.org (full text, mbox):
From: Juri Linkov <juri <at> linkov.net> To: Eli Zaretskii <eliz <at> gnu.org> Cc: 33567 <at> debbugs.gnu.org Subject: Re: bug#33567: Syntactic fontification of diff hunks Date: Thu, 06 Dec 2018 01:35:17 +0200
>> When this special variable is set by a diff command, it's safe to assume >> that the files referenced from the diff buffer really exist, so it's >> safe to try reading them. I don't want a patch in a mail attachment >> to try reading files mentioned in the patch attachment. > > I think you lean heavily towards the use case where the diffs were > produced by some Emacs command. Whereas what I have in mind is the > use case where the diffs came from some other source, like manual > invocation of shell commands. I'm saying that in the absence of > diff-default-directory, using default-directory and 'cd' will be much > easier for users. Users still can use all diff-mode commands to visit source files from them. Only language syntax fontification in diffs will fall back to hunk-only when a diff buffer is not created by a diff command. This is to avoid all possible dangers of automatically visiting files in arbitrary diff buffers. > The use case I have in mind is that some time passes between the > generation of the diffs and the time the diffs are visited and > fontified. During that time, the working revision could have been > changed. Isn't that what this issue is about? If so, we should > explain why in that case, rare as it could be, the fontification might > be wrong. Fontification can't be wrong because code verifies if hunks from diff still exist in the updated file, and doesn't highlight changed hunks.
bug-gnu-emacs <at> gnu.org:bug#33567; Package emacs.
(Thu, 06 Dec 2018 06:55:02 GMT) Full text and rfc822 format available.Message #62 received at 33567 <at> debbugs.gnu.org (full text, mbox):
From: Eli Zaretskii <eliz <at> gnu.org> To: Juri Linkov <juri <at> linkov.net> Cc: 33567 <at> debbugs.gnu.org Subject: Re: bug#33567: Syntactic fontification of diff hunks Date: Thu, 06 Dec 2018 08:53:46 +0200
> From: Juri Linkov <juri <at> linkov.net> > Cc: 33567 <at> debbugs.gnu.org > Date: Thu, 06 Dec 2018 01:25:46 +0200 > > I'm thinking about overriding coding in vc-git-find-revision like > > diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el > index f317400530..e5f44524df 100644 > --- a/lisp/vc/vc-git.el > +++ b/lisp/vc/vc-git.el > @@ -838,8 +838,8 @@ vc-git-checkin > > (defun vc-git-find-revision (file rev buffer) > (let* (process-file-side-effects > - (coding-system-for-read 'binary) > - (coding-system-for-write 'binary) > + (coding-system-for-read (or coding-system-for-read 'binary)) > + (coding-system-for-write (or coding-system-for-write 'binary)) > (fullname > (let ((fn (vc-git--run-command-string > file "ls-files" "-z" "--full-name" "--"))) > > then a caller function could set its own value of this dynamic binding. > But I haven't tested yet if it works with UTF-8 file names. This is something that we might want to do anyway (it is IMO rude to unconditionally bind these symbols), but I'm not sure it helps you, because the level you work on is not vc-git. The right value to bind coding-system-for-read here is vc-git-log-output-coding-system, but that is a Git specific value, whereas you are trying to provide a VCS-independent feature, which should work with any supported VCS.
bug-gnu-emacs <at> gnu.org:bug#33567; Package emacs.
(Tue, 11 Dec 2018 01:10:02 GMT) Full text and rfc822 format available.Message #65 received at 33567 <at> debbugs.gnu.org (full text, mbox):
From: Juri Linkov <juri <at> linkov.net> To: Eli Zaretskii <eliz <at> gnu.org> Cc: 33567 <at> debbugs.gnu.org Subject: Re: bug#33567: Syntactic fontification of diff hunks Date: Tue, 11 Dec 2018 02:38:18 +0200
> If you still want to reuse the literal contents of the file, as
> inserted by vc-git-find-revision etc., then you will have to duplicate
> what insert-file-contents does internally. I suggest to look at how
> this is done in archive-set-buffer-as-visiting-file.
I looked at archive-set-buffer-as-visiting-file, and it seems it could
be simplified based on the assumption that the backend inserts output
in the binary coding. I tried and this fixes the problem:
diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index 5ff9f4d5be..c980369fa9 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -2042,6 +2042,7 @@ vc-find-revision-no-save
(if backend
(vc-call-backend backend 'find-revision file revision outbuf)
(vc-call find-revision file revision outbuf))))
+ (recode-region (point-min) (point-max) buffer-file-coding-system 'binary)
(goto-char (point-min))
(if buffer (let ((buffer-file-name file)) (normal-mode)) (normal-mode))
(set-buffer-modified-p nil)
bug-gnu-emacs <at> gnu.org:bug#33567; Package emacs.
(Tue, 11 Dec 2018 06:25:01 GMT) Full text and rfc822 format available.Message #68 received at 33567 <at> debbugs.gnu.org (full text, mbox):
From: Eli Zaretskii <eliz <at> gnu.org> To: Juri Linkov <juri <at> linkov.net> Cc: 33567 <at> debbugs.gnu.org Subject: Re: bug#33567: Syntactic fontification of diff hunks Date: Tue, 11 Dec 2018 08:23:37 +0200
> From: Juri Linkov <juri <at> linkov.net>
> Cc: 33567 <at> debbugs.gnu.org
> Date: Tue, 11 Dec 2018 02:38:18 +0200
>
> > If you still want to reuse the literal contents of the file, as
> > inserted by vc-git-find-revision etc., then you will have to duplicate
> > what insert-file-contents does internally. I suggest to look at how
> > this is done in archive-set-buffer-as-visiting-file.
>
> I looked at archive-set-buffer-as-visiting-file, and it seems it could
> be simplified based on the assumption that the backend inserts output
> in the binary coding.
"Binary coding" means what we have in the buffer is exactly what we
had on disk. IOW, we have there the contents of the file in its
original encoding, and so decoding it correctly needs to use the same
facilities we normally use when visiting a file with the likes of
find-file. archive-set-buffer-as-visiting-file solves precisely the
same problem.
> I tried and this fixes the problem:
>
> diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
> index 5ff9f4d5be..c980369fa9 100644
> --- a/lisp/vc/vc.el
> +++ b/lisp/vc/vc.el
> @@ -2042,6 +2042,7 @@ vc-find-revision-no-save
> (if backend
> (vc-call-backend backend 'find-revision file revision outbuf)
> (vc-call find-revision file revision outbuf))))
> + (recode-region (point-min) (point-max) buffer-file-coding-system 'binary)
Where does the value of buffer-file-coding-system come from here?
Isn't that just (default-value 'buffer-file-coding-system)? If so,
you were just lucky that it worked for you; in general, if the
encoding of the file is different from your locale-derived defaults,
the above won't DTRT.
In any case, really don't think recode-region is TRT in this case, for
several reasons:
. recode-region is a command, so it wastes cycles checking the
argument coding-system, which is entirely unnecessary in this case
. it narrows the buffer, something that again is a waste of cycles
for your case
. it wastes even more cycles for "encoding" with 'binary', which in
this case is a no-op, since the text was not decoded on reading
. it doesn't use the following facilities for determining the right
encoding, where you use buffer-file-coding-system:
- auto-coding-function, which is where we detect the 'coding:'
cookies in the first line and in the local vars, and use the
data in auto-coding-alist and auto-coding-regexp-alist, and also
call auto-coding-functions if needed
- find-operation-coding-system by file name, which uses the data
in file-coding-system-alist to determine the appropriate
encoding given the file's name
The hard problem here is to determine what coding-system to use for
decoding a region that was inserted without any conversions; once the
encoding is determined, the rest boils down to calling
decode-coding-region with that encoding. The method used by
archive-set-buffer-as-visiting-file solves that very problem, whereas
recode-region does not, because it is a command that relies on the
caller to specify the encoding, and is otherwise nothing more than a
thin wrapper around decode-coding-region.
If you need further help understanding what
archive-set-buffer-as-visiting-file does, and which parts might not be
relevant to the function you are writing, please ask specific
questions and I will gladly help as much as I can. But recode-region
is IMO not the right tool for this job.
Thanks.
bug-gnu-emacs <at> gnu.org:bug#33567; Package emacs.
(Wed, 12 Dec 2018 00:43:03 GMT) Full text and rfc822 format available.Message #71 received at 33567 <at> debbugs.gnu.org (full text, mbox):
From: Juri Linkov <juri <at> linkov.net> To: Eli Zaretskii <eliz <at> gnu.org> Cc: 33567 <at> debbugs.gnu.org Subject: Re: bug#33567: Syntactic fontification of diff hunks Date: Wed, 12 Dec 2018 02:28:08 +0200
> . it doesn't use the following facilities for determining the right
> encoding, where you use buffer-file-coding-system:
> - auto-coding-function, which is where we detect the 'coding:'
> cookies in the first line and in the local vars, and use the
> data in auto-coding-alist and auto-coding-regexp-alist, and also
> call auto-coding-functions if needed
> - find-operation-coding-system by file name, which uses the data
> in file-coding-system-alist to determine the appropriate
> encoding given the file's name
>
> The hard problem here is to determine what coding-system to use for
> decoding a region that was inserted without any conversions; once the
> encoding is determined, the rest boils down to calling
> decode-coding-region with that encoding. The method used by
> archive-set-buffer-as-visiting-file solves that very problem, whereas
> recode-region does not, because it is a command that relies on the
> caller to specify the encoding, and is otherwise nothing more than a
> thin wrapper around decode-coding-region.
Thanks for the explanation. I explored more on this subject, and found
the most suitable existing function: `decode-coding-inserted-region'.
I tested it with different codings and everything works well:
diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index 5ff9f4d5be..127661a039 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -2042,6 +2042,8 @@ vc-find-revision-no-save
(if backend
(vc-call-backend backend 'find-revision file revision outbuf)
(vc-call find-revision file revision outbuf))))
+ (decode-coding-inserted-region (point-min) (point-max) file)
+ (after-insert-file-set-coding (- (point-max) (point-min)))
(goto-char (point-min))
(if buffer (let ((buffer-file-name file)) (normal-mode)) (normal-mode))
(set-buffer-modified-p nil)
bug-gnu-emacs <at> gnu.org:bug#33567; Package emacs.
(Wed, 12 Dec 2018 17:12:02 GMT) Full text and rfc822 format available.Message #74 received at 33567 <at> debbugs.gnu.org (full text, mbox):
From: Eli Zaretskii <eliz <at> gnu.org> To: Juri Linkov <juri <at> linkov.net> Cc: 33567 <at> debbugs.gnu.org Subject: Re: bug#33567: Syntactic fontification of diff hunks Date: Wed, 12 Dec 2018 19:11:28 +0200
> From: Juri Linkov <juri <at> linkov.net> > Cc: 33567 <at> debbugs.gnu.org > Date: Wed, 12 Dec 2018 02:28:08 +0200 > > Thanks for the explanation. I explored more on this subject, and found > the most suitable existing function: `decode-coding-inserted-region'. > I tested it with different codings and everything works well: Thanks, this LGTM.
bug-gnu-emacs <at> gnu.org:bug#33567; Package emacs.
(Wed, 12 Dec 2018 23:21:02 GMT) Full text and rfc822 format available.Message #77 received at 33567 <at> debbugs.gnu.org (full text, mbox):
From: Juri Linkov <juri <at> linkov.net> To: Eli Zaretskii <eliz <at> gnu.org> Cc: 33567 <at> debbugs.gnu.org Subject: Re: bug#33567: Syntactic fontification of diff hunks Date: Thu, 13 Dec 2018 01:17:52 +0200
[Message part 1 (text/plain, inline)]
> Users still can use all diff-mode commands to visit source files from them. > Only language syntax fontification in diffs will fall back to hunk-only > when a diff buffer is not created by a diff command. This is to avoid all > possible dangers of automatically visiting files in arbitrary diff buffers. Due to the new variable `diff-default-directory', we can simplify the customization options of `diff-font-lock-syntax'. The default `t' is the safest and the most reliable: it extracts revisions from VC when the Diff buffer is created by a VC command, and extracts files from the file system only in case the Diff buffer is created by a file-based Diff command that sets the new variable `diff-default-directory'. Here is a more less final version that I tested on many different diffs:
[diff-font-lock-syntax.3.patch (text/x-diff, inline)]
diff --git a/etc/NEWS b/etc/NEWS
index decc5e3954..1024e9e9f5 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -273,6 +273,8 @@ To disable it, set the new defcustom 'diff-font-lock-refine' to nil.
*** File headers can be shortened, mimicking Magit's diff format
To enable it, set the new defcustom 'diff-font-lock-prettify to t.
+*** Source language syntax is highlighted in diff hunks when 'diff-font-lock-syntax' is t.
+
** Browse-url
*** The function 'browse-url-emacs' can now visit a URL in selected window.
diff --git a/doc/emacs/files.texi b/doc/emacs/files.texi
index b47be51e24..07f311c24e 100644
--- a/doc/emacs/files.texi
+++ b/doc/emacs/files.texi
@@ -1617,6 +1617,9 @@ Diff Mode
modify the original (``old'') source files rather than the patched
(``new'') source files.
+@vindex diff-font-lock-syntax
+ If non-nil, diff hunk font-lock includes source language syntax highlighting.
+
@node Copying and Naming
@section Copying, Naming and Renaming Files
diff --git a/lisp/vc/diff.el b/lisp/vc/diff.el
index ac94586cac..93fa54b1fb 100644
--- a/lisp/vc/diff.el
+++ b/lisp/vc/diff.el
@@ -165,6 +167,7 @@ diff-no-select
(lambda (_ignore-auto _noconfirm)
(diff-no-select old new switches no-async (current-buffer))))
(setq default-directory thisdir)
+ (set (make-local-variable 'diff-default-directory) default-directory)
(let ((inhibit-read-only t))
(insert command "\n"))
(if (and (not no-async) (fboundp 'make-process))
diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 4adef02984..99ddb19f87 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -103,12 +104,41 @@ diff-font-lock-prettify
:version "27.1"
:type 'boolean)
+(defcustom diff-font-lock-syntax t
+ "If non-nil, diff hunk font-lock includes source language syntax highlighting.
+This highlighting is the same as added by `font-lock-mode'
+when corresponding source files are visited normally.
+Syntax highlighting is added over diff own highlighted changes.
+
+If t, the default, highlight syntax only in Diff buffers created by Diff
+commands that compare files or by VC commands that compare revisions.
+These provide all necessary context for reliable highlighting. This value
+requires support from a VC backend to find the files being compared.
+For diffs against the working-tree version of a file, the highlighting is
+based on the current file contents. File-based fontification tries to
+infer fontification from the compared files.
+
+If some method fails, get fontification from hunk alone if the value is
+`hunk-also'.
+
+If `hunk-only', fontification is based on hunk alone, without full source.
+It tries to highlight hunks without enough context that sometimes might result
+in wrong fontification. This is the fastest option, but less reliable."
+ :version "27.1"
+ :type '(choice (const :tag "Don't highlight syntax" nil)
+ (const :tag "Hunk-based also" hunk-also)
+ (const :tag "Hunk-based only" hunk-only)
+ (const :tag "Highlight syntax" t)))
+
(defvar diff-vc-backend nil
"The VC backend that created the current Diff buffer, if any.")
(defvar diff-vc-revisions nil
"The VC revisions compared in the current Diff buffer, if any.")
+(defvar diff-default-directory nil
+ "The default directory where the current Diff buffer was created.")
+
(defvar diff-outline-regexp
"\\([*+][*+][*+] [^0-9]\\|@@ ...\\|\\*\\*\\* [0-9].\\|--- [0-9]..\\)")
@@ -406,6 +436,7 @@ diff-font-lock-keywords
(1 font-lock-comment-delimiter-face)
(2 font-lock-comment-face))
("^[^-=+*!<>#].*\n" (0 'diff-context))
+ (,#'diff--font-lock-syntax)
(,#'diff--font-lock-prettify)
(,#'diff--font-lock-refined)))
@@ -2316,6 +2348,189 @@ diff--font-lock-prettify
'display "")))))
nil)
+;;; Syntax highlighting from font-lock
+
+(defun diff--font-lock-syntax (max)
+ "Syntax highlighting from font-lock."
+ (when diff-font-lock-syntax
+ (when (get-char-property (point) 'diff--font-lock-syntax)
+ (goto-char (next-single-char-property-change
+ (point) 'diff--font-lock-syntax nil max)))
+ (let* ((min (point))
+ (beg (or (ignore-errors (diff-beginning-of-hunk))
+ (ignore-errors (diff-hunk-next) (point))
+ max)))
+ (while (< beg max)
+ (let ((end
+ (save-excursion (goto-char beg) (diff-end-of-hunk) (point))))
+ (if (< end min) (setq beg min))
+ (unless (or (< end beg)
+ (get-char-property beg 'diff--font-lock-syntax))
+ (diff-syntax-fontify beg end)
+ (let ((ol (make-overlay beg end)))
+ (overlay-put ol 'diff--font-lock-syntax t)
+ (overlay-put ol 'diff-mode 'syntax)
+ (overlay-put ol 'evaporate t)
+ (overlay-put ol 'modification-hooks
+ '(diff--font-lock-syntax--refresh))))
+ (goto-char (max beg end))
+ (setq beg (or (ignore-errors (diff-hunk-next) (point)) max))))))
+ nil)
+
+(defun diff--font-lock-syntax--refresh (ol _after _beg _end &optional _len)
+ (delete-overlay ol))
+
+(defun diff-syntax-fontify (start end)
+ (save-excursion
+ (diff-syntax-fontify-hunk start end t)
+ (diff-syntax-fontify-hunk start end nil)))
+
+(defvar diff-syntax-fontify-revisions (make-hash-table :test 'equal))
+
+(defun diff-syntax-fontify-hunk (beg end old)
+ "Highlight source language syntax in diff hunks."
+ (remove-overlays beg end 'diff-mode 'syntax)
+ (goto-char beg)
+ (let* ((hunk (buffer-substring-no-properties beg end))
+ (text (or (ignore-errors (diff-hunk-text hunk (not old) nil)) ""))
+ (line (if (looking-at "\\(?:\\*\\{15\\}.*\n\\)?[-@* ]*\\([0-9,]+\\)\\([ acd+]+\\([0-9,]+\\)\\)?")
+ (if old (match-string 1)
+ (if (match-end 3) (match-string 3) (match-string 1)))))
+ (line-nb (and line (string-match "\\([0-9]+\\),\\([0-9]+\\)" line)
+ (list (string-to-number (match-string 1 line))
+ (string-to-number (match-string 2 line)))))
+ props)
+ (cond
+ ((and diff-vc-backend (not (eq diff-font-lock-syntax 'hunk-only)))
+ (let* ((file (diff-find-file-name old t))
+ (revision (and file (if (not old) (nth 1 diff-vc-revisions)
+ (or (nth 0 diff-vc-revisions)
+ (vc-working-revision file))))))
+ (if file
+ (if (not revision)
+ ;; Get properties from the current working revision
+ (when (and (not old) (file-exists-p file) (file-regular-p file))
+ ;; Try to reuse an existing buffer
+ (if (get-file-buffer (expand-file-name file))
+ (with-current-buffer (get-file-buffer (expand-file-name file))
+ (setq props (diff-syntax-fontify-props nil text line-nb t)))
+ ;; Get properties from the file
+ (with-temp-buffer
+ (insert-file-contents file t)
+ (setq props (diff-syntax-fontify-props file text line-nb)))))
+ ;; Get properties from a cached revision
+ (let* ((buffer-name (format " diff-syntax:%s.~%s~"
+ (expand-file-name file) revision))
+ (buffer (gethash buffer-name diff-syntax-fontify-revisions)))
+ (unless (and buffer (buffer-live-p buffer))
+ (let* ((vc-buffer (ignore-errors
+ (vc-find-revision-no-save
+ (expand-file-name file) revision
+ diff-vc-backend
+ (get-buffer-create buffer-name)))))
+ (when vc-buffer
+ (setq buffer vc-buffer)
+ (puthash buffer-name buffer diff-syntax-fontify-revisions))))
+ (when buffer
+ (with-current-buffer buffer
+ (setq props (diff-syntax-fontify-props file text line-nb t))))))
+ ;; If file is unavailable, get properties from the hunk alone
+ (setq file (car (diff-hunk-file-names old)))
+ (with-temp-buffer
+ (insert text)
+ (setq props (diff-syntax-fontify-props file text line-nb nil t))))))
+ ((and diff-default-directory (not (eq diff-font-lock-syntax 'hunk-only)))
+ (let ((file (car (diff-hunk-file-names old))))
+ (if (and file (file-exists-p file) (file-regular-p file))
+ ;; Try to get full text from the file
+ (with-temp-buffer
+ (insert-file-contents file t)
+ (setq props (diff-syntax-fontify-props file text line-nb)))
+ ;; Otherwise, get properties from the hunk alone
+ (with-temp-buffer
+ (insert text)
+ (setq props (diff-syntax-fontify-props file text line-nb nil t))))))
+ ((memq diff-font-lock-syntax '(hunk-also hunk-only))
+ (let ((file (car (diff-hunk-file-names old))))
+ (with-temp-buffer
+ (insert text)
+ (setq props (diff-syntax-fontify-props file text line-nb nil t))))))
+
+ ;; Put properties over the hunk text
+ (goto-char beg)
+ (when (and props (eq (diff-hunk-style) 'unified))
+ (while (< (progn (forward-line 1) (point)) end)
+ (when (or (and (not old) (not (looking-at-p "[-<]")))
+ (and old (not (looking-at-p "[+>]"))))
+ (if (and old (not (looking-at-p "[-<]")))
+ ;; Fontify context lines only from new source,
+ ;; don't refontify context lines from old source.
+ (pop props)
+ (let ((line-props (pop props))
+ (bol (1+ (point))))
+ (dolist (prop line-props)
+ (let ((ol (make-overlay (+ bol (nth 0 prop))
+ (+ bol (nth 1 prop))
+ nil 'front-advance nil)))
+ (overlay-put ol 'evaporate t)
+ (overlay-put ol 'face (nth 2 prop)))))))))))
+
+(defun diff-syntax-fontify-props (file text line-nb &optional no-init hunk-only)
+ "Get font-lock properties from the source code."
+ (unless no-init
+ (buffer-disable-undo)
+ (font-lock-mode -1)
+ (let ((enable-local-variables :safe) ;; to find `mode:'
+ (buffer-file-name file))
+ (set-auto-mode)
+ (when (and (memq 'generic-mode-find-file-hook find-file-hook)
+ (fboundp 'generic-mode-find-file-hook))
+ (generic-mode-find-file-hook))))
+
+ (let ((font-lock-defaults (or font-lock-defaults '(nil t)))
+ (inhibit-read-only t)
+ props beg end)
+ (goto-char (point-min))
+ (if hunk-only
+ (setq beg (point-min) end (point-max))
+ (forward-line (1- (nth 0 line-nb)))
+ ;; non-regexp looking-at to compare hunk text for verification
+ (if (search-forward text (+ (point) (length text)) t)
+ (setq beg (- (point) (length text)) end (point))
+ (goto-char (point-min))
+ (if (search-forward text nil t)
+ (setq beg (- (point) (length text)) end (point)))))
+
+ (when (and beg end)
+ (goto-char beg)
+ (when (text-property-not-all beg end 'fontified t)
+ (if file
+ ;; In a temporary or cached buffer
+ (save-excursion
+ (font-lock-fontify-region beg end)
+ (put-text-property beg end 'fontified t))
+ ;; In an existing buffer
+ (font-lock-ensure beg end)))
+
+ (while (< (point) end)
+ (let* ((bol (point))
+ (eol (line-end-position))
+ line-props
+ (searching t)
+ (from (point)) to
+ (val (get-text-property from 'face)))
+ (while searching
+ (setq to (next-single-property-change from 'face nil eol))
+ (when val (push (list (- from bol) (- to bol) val) line-props))
+ (setq val (get-text-property to 'face) from to)
+ (unless (< to eol) (setq searching nil)))
+ (when val (push (list from eol val) line-props))
+ (push (nreverse line-props) props))
+ (forward-line 1)))
+ (set-buffer-modified-p nil)
+ (nreverse props)))
+
+
(defun diff--filter-substring (str)
(when diff-font-lock-prettify
;; Strip the `display' properties added by diff-font-lock-prettify,
bug-gnu-emacs <at> gnu.org:bug#33567; Package emacs.
(Fri, 14 Dec 2018 09:15:02 GMT) Full text and rfc822 format available.Message #80 received at 33567 <at> debbugs.gnu.org (full text, mbox):
From: Eli Zaretskii <eliz <at> gnu.org> To: Juri Linkov <juri <at> linkov.net> Cc: 33567 <at> debbugs.gnu.org Subject: Re: bug#33567: Syntactic fontification of diff hunks Date: Fri, 14 Dec 2018 11:13:48 +0200
> From: Juri Linkov <juri <at> linkov.net> > Cc: 33567 <at> debbugs.gnu.org > Date: Thu, 13 Dec 2018 01:17:52 +0200 > > Here is a more less final version that I tested on many different diffs: Thanks, I have a few minor comments, mainly to the documentation parts. > diff --git a/etc/NEWS b/etc/NEWS > index decc5e3954..1024e9e9f5 100644 > --- a/etc/NEWS > +++ b/etc/NEWS > @@ -273,6 +273,8 @@ To disable it, set the new defcustom 'diff-font-lock-refine' to nil. > *** File headers can be shortened, mimicking Magit's diff format > To enable it, set the new defcustom 'diff-font-lock-prettify to t. > > +*** Source language syntax is highlighted in diff hunks when 'diff-font-lock-syntax' is t. This line is too long, suggest to rephrase: *** Better syntax highlighting of Diff hunks. Fragments of source in Diff hunks are now by default highlighted according to the appropriate major mode. Customize the new option 'diff-font-lock-syntax' to nil to disable this. > +@vindex diff-font-lock-syntax > + If non-nil, diff hunk font-lock includes source language syntax highlighting. "nil" should be in @code. > diff --git a/lisp/vc/diff.el b/lisp/vc/diff.el > index ac94586cac..93fa54b1fb 100644 > --- a/lisp/vc/diff.el > +++ b/lisp/vc/diff.el > @@ -165,6 +167,7 @@ diff-no-select > (lambda (_ignore-auto _noconfirm) > (diff-no-select old new switches no-async (current-buffer)))) > (setq default-directory thisdir) > + (set (make-local-variable 'diff-default-directory) default-directory) Any reason not to use setq-local? > +(defun diff--font-lock-syntax (max) > + "Syntax highlighting from font-lock." Although an internal function, could it have a slightly more detailed do string, please? > +(defun diff-syntax-fontify-hunk (beg end old) > + "Highlight source language syntax in diff hunks." This is for a single hunk, not "hunks", right? Also, please mention the arguments in the doc string. > +(defun diff-syntax-fontify-props (file text line-nb &optional no-init hunk-only) > + "Get font-lock properties from the source code." Please mention the arguments ion the doc string.
bug-gnu-emacs <at> gnu.org:bug#33567; Package emacs.
(Mon, 17 Dec 2018 00:34:02 GMT) Full text and rfc822 format available.Message #83 received at 33567 <at> debbugs.gnu.org (full text, mbox):
From: Juri Linkov <juri <at> linkov.net> To: Eli Zaretskii <eliz <at> gnu.org> Cc: 33567 <at> debbugs.gnu.org Subject: Re: bug#33567: Syntactic fontification of diff hunks Date: Mon, 17 Dec 2018 01:27:35 +0200
[Message part 1 (text/plain, inline)]
>> Here is a more less final version that I tested on many different diffs: > > Thanks, I have a few minor comments, mainly to the documentation > parts. > >> diff --git a/etc/NEWS b/etc/NEWS >> index decc5e3954..1024e9e9f5 100644 >> --- a/etc/NEWS >> +++ b/etc/NEWS >> @@ -273,6 +273,8 @@ To disable it, set the new defcustom 'diff-font-lock-refine' to nil. >> *** File headers can be shortened, mimicking Magit's diff format >> To enable it, set the new defcustom 'diff-font-lock-prettify to t. >> >> +*** Source language syntax is highlighted in diff hunks when 'diff-font-lock-syntax' is t. > > This line is too long, suggest to rephrase: > > *** Better syntax highlighting of Diff hunks. > Fragments of source in Diff hunks are now by default highlighted > according to the appropriate major mode. Customize the new option > 'diff-font-lock-syntax' to nil to disable this. Changed. >> +@vindex diff-font-lock-syntax >> + If non-nil, diff hunk font-lock includes source language syntax highlighting. > > "nil" should be in @code. Changed. >> diff --git a/lisp/vc/diff.el b/lisp/vc/diff.el >> index ac94586cac..93fa54b1fb 100644 >> --- a/lisp/vc/diff.el >> +++ b/lisp/vc/diff.el >> @@ -165,6 +167,7 @@ diff-no-select >> (lambda (_ignore-auto _noconfirm) >> (diff-no-select old new switches no-async (current-buffer)))) >> (setq default-directory thisdir) >> + (set (make-local-variable 'diff-default-directory) default-directory) > > Any reason not to use setq-local? Only for consistency with other variables in the same file that still don't use make-local-variable. But now I added automatically buffer-local (make-variable-buffer-local 'diff-default-directory) for consistency with `default-directory'. >> +(defun diff--font-lock-syntax (max) >> + "Syntax highlighting from font-lock." > > Although an internal function, could it have a slightly more detailed > do string, please? Done. >> +(defun diff-syntax-fontify-hunk (beg end old) >> + "Highlight source language syntax in diff hunks." > > This is for a single hunk, not "hunks", right? Also, please mention > the arguments in the doc string. Done. >> +(defun diff-syntax-fontify-props (file text line-nb &optional no-init hunk-only) >> + "Get font-lock properties from the source code." > > Please mention the arguments ion the doc string. Done. Also with more highlighting in diff hunks now diff indicators need more distinctive colors that I also tested.
[diff-font-lock-syntax.4.patch (text/x-diff, inline)]
diff --git a/etc/NEWS b/etc/NEWS
index c88f6ef5ca..7351f5cdd2 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -412,6 +412,12 @@ and compares their entire trees.
*** Hunks are now automatically refined by default.
To disable it, set the new defcustom 'diff-font-lock-refine' to nil.
++++
+*** Better syntax highlighting of Diff hunks.
+Fragments of source in Diff hunks are now by default highlighted
+according to the appropriate major mode. Customize the new option
+'diff-font-lock-syntax' to nil to disable this.
+
*** File headers can be shortened, mimicking Magit's diff format.
To enable it, set the new defcustom 'diff-font-lock-prettify' to t.
diff --git a/doc/emacs/files.texi b/doc/emacs/files.texi
index b47be51e24..6e1faf84dc 100644
--- a/doc/emacs/files.texi
+++ b/doc/emacs/files.texi
@@ -1617,6 +1617,10 @@ Diff Mode
modify the original (``old'') source files rather than the patched
(``new'') source files.
+@vindex diff-font-lock-syntax
+ If non-@code{nil}, fragments of source in hunks are highlighted
+according to the appropriate major mode.
+
@node Copying and Naming
@section Copying, Naming and Renaming Files
diff --git a/lisp/vc/diff.el b/lisp/vc/diff.el
index ac94586cac..ed5b49d3bf 100644
--- a/lisp/vc/diff.el
+++ b/lisp/vc/diff.el
@@ -165,6 +167,7 @@ diff-no-select
(lambda (_ignore-auto _noconfirm)
(diff-no-select old new switches no-async (current-buffer))))
(setq default-directory thisdir)
+ (setq diff-default-directory default-directory)
(let ((inhibit-read-only t))
(insert command "\n"))
(if (and (not no-async) (fboundp 'make-process))
diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index 4adef02984..7116d74db1 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -103,12 +104,42 @@ diff-font-lock-prettify
:version "27.1"
:type 'boolean)
+(defcustom diff-font-lock-syntax t
+ "If non-nil, diff hunk font-lock includes source language syntax highlighting.
+This highlighting is the same as added by `font-lock-mode'
+when corresponding source files are visited normally.
+Syntax highlighting is added over diff own highlighted changes.
+
+If t, the default, highlight syntax only in Diff buffers created by Diff
+commands that compare files or by VC commands that compare revisions.
+These provide all necessary context for reliable highlighting. This value
+requires support from a VC backend to find the files being compared.
+For diffs against the working-tree version of a file, the highlighting is
+based on the current file contents. File-based fontification tries to
+infer fontification from the compared files.
+
+If revision-based or file-based method fails, use hunk-based method to get
+fontification from hunk alone if the value is `hunk-also'.
+
+If `hunk-only', fontification is based on hunk alone, without full source.
+It tries to highlight hunks without enough context that sometimes might result
+in wrong fontification. This is the fastest option, but less reliable."
+ :version "27.1"
+ :type '(choice (const :tag "Don't highlight syntax" nil)
+ (const :tag "Hunk-based also" hunk-also)
+ (const :tag "Hunk-based only" hunk-only)
+ (const :tag "Highlight syntax" t)))
+
(defvar diff-vc-backend nil
"The VC backend that created the current Diff buffer, if any.")
(defvar diff-vc-revisions nil
"The VC revisions compared in the current Diff buffer, if any.")
+(defvar diff-default-directory nil
+ "The default directory where the current Diff buffer was created.")
+(make-variable-buffer-local 'diff-default-directory)
+
(defvar diff-outline-regexp
"\\([*+][*+][*+] [^0-9]\\|@@ ...\\|\\*\\*\\* [0-9].\\|--- [0-9]..\\)")
@@ -295,19 +326,25 @@ diff-changed
:version "25.1")
(defface diff-indicator-removed
- '((t :inherit diff-removed))
+ '((default :inherit diff-removed)
+ (((class color) (min-colors 88))
+ :foreground "#aa2222"))
"`diff-mode' face used to highlight indicator of removed lines (-, <)."
:version "22.1")
(defvar diff-indicator-removed-face 'diff-indicator-removed)
(defface diff-indicator-added
- '((t :inherit diff-added))
+ '((default :inherit diff-added)
+ (((class color) (min-colors 88))
+ :foreground "#22aa22"))
"`diff-mode' face used to highlight indicator of added lines (+, >)."
:version "22.1")
(defvar diff-indicator-added-face 'diff-indicator-added)
(defface diff-indicator-changed
- '((t :inherit diff-changed))
+ '((default :inherit diff-changed)
+ (((class color) (min-colors 88))
+ :foreground "#aaaa22"))
"`diff-mode' face used to highlight indicator of changed lines."
:version "22.1")
(defvar diff-indicator-changed-face 'diff-indicator-changed)
@@ -317,10 +354,7 @@ diff-function
"`diff-mode' face used to highlight function names produced by \"diff -p\".")
(defface diff-context
- '((((class color grayscale) (min-colors 88) (background light))
- :foreground "#333333")
- (((class color grayscale) (min-colors 88) (background dark))
- :foreground "#dddddd"))
+ '((t nil))
"`diff-mode' face used to highlight context and other side-information."
:version "25.1")
@@ -406,6 +440,7 @@ diff-font-lock-keywords
(1 font-lock-comment-delimiter-face)
(2 font-lock-comment-face))
("^[^-=+*!<>#].*\n" (0 'diff-context))
+ (,#'diff--font-lock-syntax)
(,#'diff--font-lock-prettify)
(,#'diff--font-lock-refined)))
@@ -2316,6 +2352,199 @@ diff--font-lock-prettify
'display "")))))
nil)
+;;; Syntax highlighting from font-lock
+
+(defun diff--font-lock-syntax (max)
+ "Apply source language syntax highlighting from font-lock.
+Calls `diff-syntax-fontify' on every hunk found between point
+and the position in MAX."
+ (when diff-font-lock-syntax
+ (when (get-char-property (point) 'diff--font-lock-syntax)
+ (goto-char (next-single-char-property-change
+ (point) 'diff--font-lock-syntax nil max)))
+ (let* ((min (point))
+ (beg (or (ignore-errors (diff-beginning-of-hunk))
+ (ignore-errors (diff-hunk-next) (point))
+ max)))
+ (while (< beg max)
+ (let ((end
+ (save-excursion (goto-char beg) (diff-end-of-hunk) (point))))
+ (if (< end min) (setq beg min))
+ (unless (or (< end beg)
+ (get-char-property beg 'diff--font-lock-syntax))
+ (diff-syntax-fontify beg end)
+ (let ((ol (make-overlay beg end)))
+ (overlay-put ol 'diff--font-lock-syntax t)
+ (overlay-put ol 'diff-mode 'syntax)
+ (overlay-put ol 'evaporate t)
+ (overlay-put ol 'modification-hooks
+ '(diff--font-lock-syntax--refresh))))
+ (goto-char (max beg end))
+ (setq beg (or (ignore-errors (diff-hunk-next) (point)) max))))))
+ nil)
+
+(defun diff--font-lock-syntax--refresh (ol _after _beg _end &optional _len)
+ (delete-overlay ol))
+
+(defun diff-syntax-fontify (beg end)
+ "Highlight source language syntax in diff hunk between BEG and END."
+ (save-excursion
+ (diff-syntax-fontify-hunk beg end t)
+ (diff-syntax-fontify-hunk beg end nil)))
+
+(defvar diff-syntax-fontify-revisions (make-hash-table :test 'equal))
+
+(defun diff-syntax-fontify-hunk (beg end old)
+ "Highlight source language syntax in diff hunk between BEG and END.
+When OLD is non-nil, highlight the hunk from the old source."
+ (remove-overlays beg end 'diff-mode 'syntax)
+ (goto-char beg)
+ (let* ((hunk (buffer-substring-no-properties beg end))
+ (text (or (ignore-errors (diff-hunk-text hunk (not old) nil)) ""))
+ (line (if (looking-at "\\(?:\\*\\{15\\}.*\n\\)?[-@* ]*\\([0-9,]+\\)\\([ acd+]+\\([0-9,]+\\)\\)?")
+ (if old (match-string 1)
+ (if (match-end 3) (match-string 3) (match-string 1)))))
+ (line-nb (and line (string-match "\\([0-9]+\\),\\([0-9]+\\)" line)
+ (list (string-to-number (match-string 1 line))
+ (string-to-number (match-string 2 line)))))
+ props)
+ (cond
+ ((and diff-vc-backend (not (eq diff-font-lock-syntax 'hunk-only)))
+ (let* ((file (diff-find-file-name old t))
+ (revision (and file (if (not old) (nth 1 diff-vc-revisions)
+ (or (nth 0 diff-vc-revisions)
+ (vc-working-revision file))))))
+ (if file
+ (if (not revision)
+ ;; Get properties from the current working revision
+ (when (and (not old) (file-exists-p file) (file-regular-p file))
+ ;; Try to reuse an existing buffer
+ (if (get-file-buffer (expand-file-name file))
+ (with-current-buffer (get-file-buffer (expand-file-name file))
+ (setq props (diff-syntax-fontify-props nil text line-nb t)))
+ ;; Get properties from the file
+ (with-temp-buffer
+ (insert-file-contents file t)
+ (setq props (diff-syntax-fontify-props file text line-nb)))))
+ ;; Get properties from a cached revision
+ (let* ((buffer-name (format " diff-syntax:%s.~%s~"
+ (expand-file-name file) revision))
+ (buffer (gethash buffer-name diff-syntax-fontify-revisions)))
+ (unless (and buffer (buffer-live-p buffer))
+ (let* ((vc-buffer (ignore-errors
+ (vc-find-revision-no-save
+ (expand-file-name file) revision
+ diff-vc-backend
+ (get-buffer-create buffer-name)))))
+ (when vc-buffer
+ (setq buffer vc-buffer)
+ (puthash buffer-name buffer diff-syntax-fontify-revisions))))
+ (when buffer
+ (with-current-buffer buffer
+ (setq props (diff-syntax-fontify-props file text line-nb t))))))
+ ;; If file is unavailable, get properties from the hunk alone
+ (setq file (car (diff-hunk-file-names old)))
+ (with-temp-buffer
+ (insert text)
+ (setq props (diff-syntax-fontify-props file text line-nb nil t))))))
+ ((and diff-default-directory (not (eq diff-font-lock-syntax 'hunk-only)))
+ (let ((file (car (diff-hunk-file-names old))))
+ (if (and file (file-exists-p file) (file-regular-p file))
+ ;; Try to get full text from the file
+ (with-temp-buffer
+ (insert-file-contents file t)
+ (setq props (diff-syntax-fontify-props file text line-nb)))
+ ;; Otherwise, get properties from the hunk alone
+ (with-temp-buffer
+ (insert text)
+ (setq props (diff-syntax-fontify-props file text line-nb nil t))))))
+ ((memq diff-font-lock-syntax '(hunk-also hunk-only))
+ (let ((file (car (diff-hunk-file-names old))))
+ (with-temp-buffer
+ (insert text)
+ (setq props (diff-syntax-fontify-props file text line-nb nil t))))))
+
+ ;; Put properties over the hunk text
+ (goto-char beg)
+ (when (and props (eq (diff-hunk-style) 'unified))
+ (while (< (progn (forward-line 1) (point)) end)
+ (when (or (and (not old) (not (looking-at-p "[-<]")))
+ (and old (not (looking-at-p "[+>]"))))
+ (if (and old (not (looking-at-p "[-<]")))
+ ;; Fontify context lines only from new source,
+ ;; don't refontify context lines from old source.
+ (pop props)
+ (let ((line-props (pop props))
+ (bol (1+ (point))))
+ (dolist (prop line-props)
+ (let ((ol (make-overlay (+ bol (nth 0 prop))
+ (+ bol (nth 1 prop))
+ nil 'front-advance nil)))
+ (overlay-put ol 'evaporate t)
+ (overlay-put ol 'face (nth 2 prop)))))))))))
+
+(defun diff-syntax-fontify-props (file text line-nb &optional no-init hunk-only)
+ "Get font-lock properties from the source code.
+FILE is the name of the source file. TEXT is the literal source text from
+hunk. LINE-NB is a pair of numbers: start line number and the number of
+lines in the hunk. NO-INIT means no initialization is needed to set major
+mode. When HUNK-ONLY is non-nil, then don't verify the existence of the
+hunk text in the source file. Otherwise, don't highlight the hunk if the
+hunk text is not found in the source file."
+ (unless no-init
+ (buffer-disable-undo)
+ (font-lock-mode -1)
+ (let ((enable-local-variables :safe) ;; to find `mode:'
+ (buffer-file-name file))
+ (set-auto-mode)
+ (when (and (memq 'generic-mode-find-file-hook find-file-hook)
+ (fboundp 'generic-mode-find-file-hook))
+ (generic-mode-find-file-hook))))
+
+ (let ((font-lock-defaults (or font-lock-defaults '(nil t)))
+ (inhibit-read-only t)
+ props beg end)
+ (goto-char (point-min))
+ (if hunk-only
+ (setq beg (point-min) end (point-max))
+ (forward-line (1- (nth 0 line-nb)))
+ ;; non-regexp looking-at to compare hunk text for verification
+ (if (search-forward text (+ (point) (length text)) t)
+ (setq beg (- (point) (length text)) end (point))
+ (goto-char (point-min))
+ (if (search-forward text nil t)
+ (setq beg (- (point) (length text)) end (point)))))
+
+ (when (and beg end)
+ (goto-char beg)
+ (when (text-property-not-all beg end 'fontified t)
+ (if file
+ ;; In a temporary or cached buffer
+ (save-excursion
+ (font-lock-fontify-region beg end)
+ (put-text-property beg end 'fontified t))
+ ;; In an existing buffer
+ (font-lock-ensure beg end)))
+
+ (while (< (point) end)
+ (let* ((bol (point))
+ (eol (line-end-position))
+ line-props
+ (searching t)
+ (from (point)) to
+ (val (get-text-property from 'face)))
+ (while searching
+ (setq to (next-single-property-change from 'face nil eol))
+ (when val (push (list (- from bol) (- to bol) val) line-props))
+ (setq val (get-text-property to 'face) from to)
+ (unless (< to eol) (setq searching nil)))
+ (when val (push (list from eol val) line-props))
+ (push (nreverse line-props) props))
+ (forward-line 1)))
+ (set-buffer-modified-p nil)
+ (nreverse props)))
+
+
(defun diff--filter-substring (str)
(when diff-font-lock-prettify
;; Strip the `display' properties added by diff-font-lock-prettify,
bug-gnu-emacs <at> gnu.org:bug#33567; Package emacs.
(Mon, 17 Dec 2018 16:14:01 GMT) Full text and rfc822 format available.Message #86 received at 33567 <at> debbugs.gnu.org (full text, mbox):
From: Eli Zaretskii <eliz <at> gnu.org> To: Juri Linkov <juri <at> linkov.net> Cc: 33567 <at> debbugs.gnu.org Subject: Re: bug#33567: Syntactic fontification of diff hunks Date: Mon, 17 Dec 2018 18:13:51 +0200
> From: Juri Linkov <juri <at> linkov.net> > Cc: 33567 <at> debbugs.gnu.org > Date: Mon, 17 Dec 2018 01:27:35 +0200 > > Also with more highlighting in diff hunks now diff indicators need more > distinctive colors that I also tested. Thanks, this LGTM.
Juri Linkov <juri <at> linkov.net>:Juri Linkov <juri <at> linkov.net>:Message #91 received at 33567-done <at> debbugs.gnu.org (full text, mbox):
From: Juri Linkov <juri <at> linkov.net> To: Eli Zaretskii <eliz <at> gnu.org> Cc: 33567-done <at> debbugs.gnu.org Subject: Re: bug#33567: Syntactic fontification of diff hunks Date: Tue, 18 Dec 2018 01:11:49 +0200
>> Also with more highlighting in diff hunks now diff indicators need more >> distinctive colors that I also tested. > > Thanks, this LGTM. Pushed to master and closed.
bug-gnu-emacs <at> gnu.org:bug#33567; Package emacs.
(Tue, 18 Dec 2018 00:34:02 GMT) Full text and rfc822 format available.Message #94 received at 33567 <at> debbugs.gnu.org (full text, mbox):
From: Juri Linkov <juri <at> linkov.net> To: 33567 <at> debbugs.gnu.org Subject: Re: bug#33567: Syntactic fontification of diff hunks Date: Tue, 18 Dec 2018 02:14:48 +0200
[Message part 1 (text/plain, inline)]
>> Thanks, this LGTM. > > Pushed to master and closed. Here's an additional patch to support diffs with files that contain just one line as documented in (info "(diffutils) Detailed Unified") and also without the final newline. Such diffs have the text: \ No newline at end of file
[diff-mode_no_newline.el.patch (text/x-diff, inline)]
diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index ed953deb21..8c18f69e8c 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -1697,10 +1697,11 @@ diff-hunk-text
(delete-region divider-pos (point-max)))
(delete-region (point-min) keep))
;; Remove line-prefix characters, and unneeded lines (unified diffs).
- (let ((kill-char (if destp ?- ?+)))
+ ;; Also skip lines like "\ No newline at end of file"
+ (let ((kill-chars (list (if destp ?- ?+) ?\\)))
(goto-char (point-min))
(while (not (eobp))
- (if (eq (char-after) kill-char)
+ (if (memq (char-after) kill-chars)
(delete-region (point) (progn (forward-line 1) (point)))
(delete-char num-pfx-chars)
(forward-line 1)))))
@@ -2394,19 +2395,23 @@ diff-syntax-fontify
(defvar diff-syntax-fontify-revisions (make-hash-table :test 'equal))
+(eval-when-compile (require 'subr-x)) ; for string-trim-right
+
(defun diff-syntax-fontify-hunk (beg end old)
"Highlight source language syntax in diff hunk between BEG and END.
When OLD is non-nil, highlight the hunk from the old source."
(remove-overlays beg end 'diff-mode 'syntax)
(goto-char beg)
(let* ((hunk (buffer-substring-no-properties beg end))
- (text (or (ignore-errors (diff-hunk-text hunk (not old) nil)) ""))
+ (text (string-trim-right (or (ignore-errors (diff-hunk-text hunk (not old) nil)) "")))
(line (if (looking-at "\\(?:\\*\\{15\\}.*\n\\)?[-@* ]*\\([0-9,]+\\)\\([ acd+]+\\([0-9,]+\\)\\)?")
(if old (match-string 1)
(if (match-end 3) (match-string 3) (match-string 1)))))
- (line-nb (and line (string-match "\\([0-9]+\\),\\([0-9]+\\)" line)
+ (line-nb (when line
+ (if (string-match "\\([0-9]+\\),\\([0-9]+\\)" line)
(list (string-to-number (match-string 1 line))
- (string-to-number (match-string 2 line)))))
+ (string-to-number (match-string 2 line)))
+ (list (string-to-number line) 1)))) ; One-line diffs
props)
(cond
((and diff-vc-backend (not (eq diff-font-lock-syntax 'hunk-only)))
@@ -2470,6 +2475,7 @@ diff-syntax-fontify-hunk
(while (< (progn (forward-line 1) (point)) end)
(when (or (and (not old) (not (looking-at-p "[-<]")))
(and old (not (looking-at-p "[+>]"))))
+ (unless (looking-at-p "\\\\") ; skip "\ No newline at end of file"
(if (and old (not (looking-at-p "[-<]")))
;; Fontify context lines only from new source,
;; don't refontify context lines from old source.
@@ -2481,7 +2487,7 @@ diff-syntax-fontify-hunk
(+ bol (nth 1 prop))
nil 'front-advance nil)))
(overlay-put ol 'evaporate t)
- (overlay-put ol 'face (nth 2 prop)))))))))))
+ (overlay-put ol 'face (nth 2 prop))))))))))))
(defun diff-syntax-fontify-props (file text line-nb &optional no-init hunk-only)
"Get font-lock properties from the source code.
bug-gnu-emacs <at> gnu.org:bug#33567; Package emacs.
(Tue, 18 Dec 2018 15:56:01 GMT) Full text and rfc822 format available.Message #97 received at 33567 <at> debbugs.gnu.org (full text, mbox):
From: Dmitry Gutov <dgutov <at> yandex.ru> To: 33567 <at> debbugs.gnu.org, juri <at> linkov.net Subject: Re: bug#33567: Syntactic fontification of diff hunks Date: Tue, 18 Dec 2018 17:55:15 +0200
On 18.12.2018 1:11, Juri Linkov wrote: >>> Also with more highlighting in diff hunks now diff indicators need more >>> distinctive colors that I also tested. >> >> Thanks, this LGTM. > > Pushed to master and closed. Juri, thank you. Looks like an helpful feature. Have you considered bringing over the "default" face foregrounds as well, though, not just the special highlighting? My brain sees syntax highlighting in some places and keeps wanting to interpret the green and red text and something syntactically meaningful as well. Maybe we could add extra ancestors for diff-added and diff-removed faces, with default foregrounds, and use them for the base highlighting of diff chunks, aside from +'s and -'s. Another thing that messes up the display a bit, is that comments and docstrings also have customized backgrounds in my theme, but that's less essential and seems harder to fix.
bug-gnu-emacs <at> gnu.org:bug#33567; Package emacs.
(Tue, 18 Dec 2018 22:40:01 GMT) Full text and rfc822 format available.Message #100 received at 33567 <at> debbugs.gnu.org (full text, mbox):
From: Juri Linkov <juri <at> linkov.net> To: Dmitry Gutov <dgutov <at> yandex.ru> Cc: 33567 <at> debbugs.gnu.org Subject: Re: bug#33567: Syntactic fontification of diff hunks Date: Wed, 19 Dec 2018 00:35:36 +0200
> Have you considered bringing over the "default" face foregrounds as well, > though, not just the special highlighting? > > My brain sees syntax highlighting in some places and keeps wanting to > interpret the green and red text and something syntactically meaningful > as well. > > Maybe we could add extra ancestors for diff-added and diff-removed faces, > with default foregrounds, and use them for the base highlighting of diff > chunks, aside from +'s and -'s. > > Another thing that messes up the display a bit, is that comments and > docstrings also have customized backgrounds in my theme, but that's less > essential and seems harder to fix. I tried to replicate your settings: in emacs -Q I change the default foreground with e.g. M-x set-foreground-color RET A side note: M-x set-foreground-color RET TAB shows a list of colors with different backgrounds, not foregrounds as it would be natural to expect, so it is difficult to decide what foreground color to select by looking at background colors. This needs a separate bug#. Ok, let's say I chose M-x set-foreground-color RET LightSeaGreen Now in diff-mode I see exactly the same fontification with the same foreground colors and as seen in the original buffer visited from diff-mode by `C-c C-c', plus red/green background added by diff-mode.
bug-gnu-emacs <at> gnu.org:bug#33567; Package emacs.
(Tue, 18 Dec 2018 23:34:01 GMT) Full text and rfc822 format available.Message #103 received at 33567 <at> debbugs.gnu.org (full text, mbox):
From: Dmitry Gutov <dgutov <at> yandex.ru> To: Juri Linkov <juri <at> linkov.net> Cc: 33567 <at> debbugs.gnu.org Subject: Re: bug#33567: Syntactic fontification of diff hunks Date: Wed, 19 Dec 2018 01:33:30 +0200
[Message part 1 (text/plain, inline)]
On 19.12.2018 0:35, Juri Linkov wrote: > I tried to replicate your settings: in emacs -Q I change the default > foreground with e.g. M-x set-foreground-color RET That only seems to change the default's foreground color. Which affects how the context looks, but not lines with changes. > A side note: M-x set-foreground-color RET TAB shows a list > of colors with different backgrounds, not foregrounds > as it would be natural to expect, so it is difficult to decide > what foreground color to select by looking at background colors. > This needs a separate bug#. Indeed, it only gives a rough idea. > Ok, let's say I chose M-x set-foreground-color RET LightSeaGreen > > Now in diff-mode I see exactly the same fontification with the same > foreground colors and as seen in the original buffer visited from > diff-mode by `C-c C-c', plus red/green background added by diff-mode. Let me show you a screenshot. I've set some purple-ish foreground using 'M-x set-foreground-color', and you can see it in certain places. But where there was the "default" face in the original buffers, in the changes lines there is now green4 or red4 in the foreground, coming from the respective faces diff-added and diff-removed. (You can also see the white from the docstring and comment faces, but that's a smaller issue, and I don't mind changing the theme if it comes to that).
[Screenshot from 2018-12-19 01-25-11.png (image/png, attachment)]
bug-gnu-emacs <at> gnu.org:bug#33567; Package emacs.
(Wed, 19 Dec 2018 00:19:01 GMT) Full text and rfc822 format available.Message #106 received at 33567 <at> debbugs.gnu.org (full text, mbox):
From: Juri Linkov <juri <at> linkov.net> To: Dmitry Gutov <dgutov <at> yandex.ru> Cc: 33567 <at> debbugs.gnu.org Subject: Re: bug#33567: Syntactic fontification of diff hunks Date: Wed, 19 Dec 2018 02:11:19 +0200
>> I tried to replicate your settings: in emacs -Q I change the default >> foreground with e.g. M-x set-foreground-color RET > > That only seems to change the default's foreground color. Which affects how > the context looks, but not lines with changes. > >> A side note: M-x set-foreground-color RET TAB shows a list >> of colors with different backgrounds, not foregrounds >> as it would be natural to expect, so it is difficult to decide >> what foreground color to select by looking at background colors. >> This needs a separate bug#. > > Indeed, it only gives a rough idea. Reported to bug#33799. >> Ok, let's say I chose M-x set-foreground-color RET LightSeaGreen >> >> Now in diff-mode I see exactly the same fontification with the same >> foreground colors and as seen in the original buffer visited from >> diff-mode by `C-c C-c', plus red/green background added by diff-mode. > > Let me show you a screenshot. I've set some purple-ish foreground using > 'M-x set-foreground-color', and you can see it in certain places. But where > there was the "default" face in the original buffers, in the changes lines > there is now green4 or red4 in the foreground, coming from the respective > faces diff-added and diff-removed. Thanks, I see now. Does it help to remove foreground colors from diff-added and diff-removed? Neither GitHub nor GitLab have foreground green/red colors, only background green/red colors.
bug-gnu-emacs <at> gnu.org:bug#33567; Package emacs.
(Wed, 19 Dec 2018 00:50:01 GMT) Full text and rfc822 format available.Message #109 received at 33567 <at> debbugs.gnu.org (full text, mbox):
From: Dmitry Gutov <dgutov <at> yandex.ru> To: Juri Linkov <juri <at> linkov.net> Cc: 33567 <at> debbugs.gnu.org Subject: Re: bug#33567: Syntactic fontification of diff hunks Date: Wed, 19 Dec 2018 02:48:58 +0200
On 19.12.2018 2:11, Juri Linkov wrote: > Does it help to remove foreground colors from diff-added and diff-removed? Yup. Except if I customize those and restart, I think the +'s and -'s on the left will become black as well, which is somewhat of a loss. So the colors might need to be moved to other definitions, e.g. diff-indicator-added. Further, I'm not sure if we should do the same to the look of the diff-mode buffers when diff-font-lock-syntax is nil. Just something to consider. Third, third-party code might rely on those faces looking as they do now. E.g. in diff-hl, diff-hl-insert inherits from diff-added, although this package likely won't be affected by this change (the faces define their own :foreground, for reasons lost to history). > Neither GitHub nor GitLab have foreground green/red colors, only background > green/red colors. Those are the interfaces that built up my expectations as well.
bug-gnu-emacs <at> gnu.org:bug#33567; Package emacs.
(Wed, 19 Dec 2018 01:36:02 GMT) Full text and rfc822 format available.Message #112 received at 33567 <at> debbugs.gnu.org (full text, mbox):
From: Dmitry Gutov <dgutov <at> yandex.ru> To: Juri Linkov <juri <at> linkov.net> Cc: 33567 <at> debbugs.gnu.org Subject: Re: bug#33567: Syntactic fontification of diff hunks Date: Wed, 19 Dec 2018 03:35:39 +0200
On 19.12.2018 2:48, Dmitry Gutov wrote:
> On 19.12.2018 2:11, Juri Linkov wrote:
>
>> Does it help to remove foreground colors from diff-added and
>> diff-removed?
>
> Yup. Except if I customize those and restart, I think the +'s and -'s on
> the left will become black as well, which is somewhat of a loss.
(Maybe not on colorful enough displays, though).
Anyway, to be clear, and other considerations aside, this works:
diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index ed953deb21..8a41e365da 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -304,9 +304,7 @@ diff-removed
(((class color) (min-colors 88) (background light))
:background "#ffdddd")
(((class color) (min-colors 88) (background dark))
- :background "#553333")
- (((class color))
- :foreground "red"))
+ :background "#553333"))
"`diff-mode' face used to highlight removed lines.")
(defface diff-added
@@ -315,9 +313,7 @@ diff-added
(((class color) (min-colors 88) (background light))
:background "#ddffdd")
(((class color) (min-colors 88) (background dark))
- :background "#335533")
- (((class color))
- :foreground "green"))
+ :background "#335533"))
"`diff-mode' face used to highlight added lines.")
(defface diff-changed
@@ -328,7 +324,9 @@ diff-changed
(defface diff-indicator-removed
'((default :inherit diff-removed)
(((class color) (min-colors 88))
- :foreground "#aa2222"))
+ :foreground "#aa2222")
+ (((class color))
+ :foreground "red"))
"`diff-mode' face used to highlight indicator of removed lines (-, <)."
:version "22.1")
(defvar diff-indicator-removed-face 'diff-indicator-removed)
@@ -336,7 +334,9 @@ diff-indicator-removed-face
(defface diff-indicator-added
'((default :inherit diff-added)
(((class color) (min-colors 88))
- :foreground "#22aa22"))
+ :foreground "#22aa22")
+ (((class color))
+ :foreground "green"))
"`diff-mode' face used to highlight indicator of added lines (+, >)."
:version "22.1")
(defvar diff-indicator-added-face 'diff-indicator-added)
bug-gnu-emacs <at> gnu.org:bug#33567; Package emacs.
(Wed, 19 Dec 2018 22:01:02 GMT) Full text and rfc822 format available.Message #115 received at 33567 <at> debbugs.gnu.org (full text, mbox):
From: Juri Linkov <juri <at> linkov.net> To: Dmitry Gutov <dgutov <at> yandex.ru> Cc: 33567 <at> debbugs.gnu.org Subject: Re: bug#33567: Syntactic fontification of diff hunks Date: Wed, 19 Dec 2018 23:51:24 +0200
>> Does it help to remove foreground colors from diff-added and diff-removed? > > Yup. Except if I customize those and restart, I think the +'s and -'s on > the left will become black as well, which is somewhat of a loss. So the > colors might need to be moved to other definitions, > e.g. diff-indicator-added. > > Further, I'm not sure if we should do the same to the look of the diff-mode > buffers when diff-font-lock-syntax is nil. Just something to consider. > > Third, third-party code might rely on those faces looking as they do > now. E.g. in diff-hl, diff-hl-insert inherits from diff-added, although > this package likely won't be affected by this change (the faces define > their own :foreground, for reasons lost to history). A worse situation with Magit - I was asked to update Magit to use this feature, but I don't know where to start: Magit uses green/red colors both for background and foreground. >> Neither GitHub nor GitLab have foreground green/red colors, only background >> green/red colors. > > Those are the interfaces that built up my expectations as well.
bug-gnu-emacs <at> gnu.org:bug#33567; Package emacs.
(Wed, 19 Dec 2018 22:01:02 GMT) Full text and rfc822 format available.Message #118 received at 33567 <at> debbugs.gnu.org (full text, mbox):
From: Juri Linkov <juri <at> linkov.net> To: Dmitry Gutov <dgutov <at> yandex.ru> Cc: 33567 <at> debbugs.gnu.org Subject: Re: bug#33567: Syntactic fontification of diff hunks Date: Wed, 19 Dec 2018 23:49:16 +0200
> Anyway, to be clear, and other considerations aside, this works: >[...] > (defface diff-added > @@ -315,9 +313,7 @@ diff-added > (((class color) (min-colors 88) (background light)) > :background "#ddffdd") > (((class color) (min-colors 88) (background dark)) > - :background "#335533") > - (((class color)) > - :foreground "green")) > + :background "#335533")) > "`diff-mode' face used to highlight added lines.") >[...] > (defface diff-indicator-added > '((default :inherit diff-added) > (((class color) (min-colors 88)) > - :foreground "#22aa22")) > + :foreground "#22aa22") > + (((class color)) > + :foreground "green")) > "`diff-mode' face used to highlight indicator of added lines (+, >)." > :version "22.1") > (defvar diff-indicator-added-face 'diff-indicator-added) This looks good. For the same reason we have the face font-lock-comment-delimiter-face separate from font-lock-comment-face to use colors only on the former, but not on the latter on tty with 8 colors to make easier to read comments.
bug-gnu-emacs <at> gnu.org:bug#33567; Package emacs.
(Wed, 19 Dec 2018 22:51:02 GMT) Full text and rfc822 format available.Message #121 received at 33567 <at> debbugs.gnu.org (full text, mbox):
From: Dmitry Gutov <dgutov <at> yandex.ru> To: Juri Linkov <juri <at> linkov.net> Cc: 33567 <at> debbugs.gnu.org Subject: Re: bug#33567: Syntactic fontification of diff hunks Date: Thu, 20 Dec 2018 00:50:15 +0200
On 19.12.2018 23:49, Juri Linkov wrote: >> Anyway, to be clear, and other considerations aside, this works: >> [...] >> (defface diff-added >> @@ -315,9 +313,7 @@ diff-added >> (((class color) (min-colors 88) (background light)) >> :background "#ddffdd") >> (((class color) (min-colors 88) (background dark)) >> - :background "#335533") >> - (((class color)) >> - :foreground "green")) >> + :background "#335533")) >> "`diff-mode' face used to highlight added lines.") >> [...] >> (defface diff-indicator-added >> '((default :inherit diff-added) >> (((class color) (min-colors 88)) >> - :foreground "#22aa22")) >> + :foreground "#22aa22") >> + (((class color)) >> + :foreground "green")) >> "`diff-mode' face used to highlight indicator of added lines (+, >)." >> :version "22.1") >> (defvar diff-indicator-added-face 'diff-indicator-added) > > This looks good. Should I install it? Nobody has commented on my earlier stated concerns, but maybe we should just push it and see how it plays out. > For the same reason we have the face font-lock-comment-delimiter-face > separate from font-lock-comment-face to use colors only on the former, > but not on the latter on tty with 8 colors to make easier to read comments. Yeah, it's totally fine to use separate faces. And I was happy to see diff-indicator-* were already defined and in use. My concerns were different, though: 1. Is it okay to use the black foreground inside diff hunks even when diff-font-lock-syntax is nil? It's an incompatible change. 2. Even if we change the default in diff-added and diff-removed, some themes might have their foregrounds customized, so those users won't notice the change. It will trickle down to the themes eventually, I think, but it's unclear how the theme authors will choose to deal with this change while keeping compatibility with previous Emacs releases.
bug-gnu-emacs <at> gnu.org:bug#33567; Package emacs.
(Thu, 20 Dec 2018 00:12:02 GMT) Full text and rfc822 format available.Message #124 received at 33567 <at> debbugs.gnu.org (full text, mbox):
From: Dmitry Gutov <dgutov <at> yandex.ru> To: Juri Linkov <juri <at> linkov.net> Cc: 33567 <at> debbugs.gnu.org Subject: Re: bug#33567: Syntactic fontification of diff hunks Date: Thu, 20 Dec 2018 02:11:20 +0200
On 19.12.2018 23:51, Juri Linkov wrote: > A worse situation with Magit - I was asked to update Magit > to use this feature, but I don't know where to start: > Magit uses green/red colors both for background and foreground. Are there cases where it uses only red/green foreground to show a hunk? I would honestly just skip those cases, if Magit insists on doing things in several different ways. Not sure why you need to port the new feature yourself, though.
bug-gnu-emacs <at> gnu.org:bug#33567; Package emacs.
(Thu, 20 Dec 2018 01:16:01 GMT) Full text and rfc822 format available.Message #127 received at 33567 <at> debbugs.gnu.org (full text, mbox):
From: Dmitry Gutov <dgutov <at> yandex.ru> To: Juri Linkov <juri <at> linkov.net> Cc: 33567 <at> debbugs.gnu.org Subject: Re: bug#33567: Syntactic fontification of diff hunks Date: Thu, 20 Dec 2018 03:15:06 +0200
On 19.12.2018 2:11, Juri Linkov wrote:
> Neither GitHub nor GitLab have foreground green/red colors, only background
> green/red colors.
Speaking of GitHub and GitLab, I wonder if we should take another cue
from them and tone down our background colors as well.
This makes the new diffs more readable for me:
(eval-after-load 'diff-mode
'(progn
(set-face-background 'diff-added "#eeffee")
(set-face-background 'diff-removed "#ffeeee")
(set-face-background 'diff-refine-added "#d0ffd0")
(set-face-background 'diff-refine-removed "#ffd0d0")
))
bug-gnu-emacs <at> gnu.org:bug#33567; Package emacs.
(Thu, 20 Dec 2018 22:32:01 GMT) Full text and rfc822 format available.Message #130 received at 33567 <at> debbugs.gnu.org (full text, mbox):
From: Juri Linkov <juri <at> linkov.net> To: Dmitry Gutov <dgutov <at> yandex.ru> Cc: 33567 <at> debbugs.gnu.org Subject: Re: bug#33567: Syntactic fontification of diff hunks Date: Thu, 20 Dec 2018 23:50:11 +0200
>> A worse situation with Magit - I was asked to update Magit >> to use this feature, but I don't know where to start: >> Magit uses green/red colors both for background and foreground. > > Are there cases where it uses only red/green foreground to show a hunk? > I would honestly just skip those cases, if Magit insists on doing things in > several different ways. > > Not sure why you need to port the new feature yourself, though. I see no such cases with only red/green foreground, so it's safe to remove red/green foreground, but probably I'll let Magit developers port it themselves because I don't understand many their design decisions.
bug-gnu-emacs <at> gnu.org:bug#33567; Package emacs.
(Thu, 20 Dec 2018 22:32:02 GMT) Full text and rfc822 format available.Message #133 received at 33567 <at> debbugs.gnu.org (full text, mbox):
From: Juri Linkov <juri <at> linkov.net> To: Dmitry Gutov <dgutov <at> yandex.ru> Cc: 33567 <at> debbugs.gnu.org Subject: Re: bug#33567: Syntactic fontification of diff hunks Date: Fri, 21 Dec 2018 00:00:18 +0200
>>> Anyway, to be clear, and other considerations aside, this works:
>>> [...]
>>> (defface diff-added
>>> @@ -315,9 +313,7 @@ diff-added
>>> (((class color) (min-colors 88) (background light))
>>> :background "#ddffdd")
>>> (((class color) (min-colors 88) (background dark))
>>> - :background "#335533")
>>> - (((class color))
>>> - :foreground "green"))
>>> + :background "#335533"))
>>> "`diff-mode' face used to highlight added lines.")
>>> [...]
>>> (defface diff-indicator-added
>>> '((default :inherit diff-added)
>>> (((class color) (min-colors 88))
>>> - :foreground "#22aa22"))
>>> + :foreground "#22aa22")
>>> + (((class color))
>>> + :foreground "green"))
>>> "`diff-mode' face used to highlight indicator of added lines (+, >)."
>>> :version "22.1")
>>> (defvar diff-indicator-added-face 'diff-indicator-added)
>>
>> This looks good.
>
> Should I install it? Nobody has commented on my earlier stated concerns,
> but maybe we should just push it and see how it plays out.
I don't know. We have two options for tty: highlight indicators only
or use red/green foreground without syntax highlighting.
>> For the same reason we have the face font-lock-comment-delimiter-face
>> separate from font-lock-comment-face to use colors only on the former,
>> but not on the latter on tty with 8 colors to make easier to read comments.
>
> Yeah, it's totally fine to use separate faces. And I was happy to see
> diff-indicator-* were already defined and in use.
>
> My concerns were different, though:
>
> 1. Is it okay to use the black foreground inside diff hunks even when
> diff-font-lock-syntax is nil? It's an incompatible change.
By default it used the black foreground. Only Magit uses red/green foreground.
> 2. Even if we change the default in diff-added and diff-removed, some
> themes might have their foregrounds customized, so those users won't notice
> the change. It will trickle down to the themes eventually, I think, but
> it's unclear how the theme authors will choose to deal with this change
> while keeping compatibility with previous Emacs releases.
Maybe with conditional face definitions like
(if (boundp 'diff-font-lock-syntax)
(defface blabla))
bug-gnu-emacs <at> gnu.org:bug#33567; Package emacs.
(Thu, 20 Dec 2018 22:32:02 GMT) Full text and rfc822 format available.Message #136 received at 33567 <at> debbugs.gnu.org (full text, mbox):
From: Juri Linkov <juri <at> linkov.net> To: Dmitry Gutov <dgutov <at> yandex.ru> Cc: 33567 <at> debbugs.gnu.org Subject: Re: bug#33567: Syntactic fontification of diff hunks Date: Fri, 21 Dec 2018 00:17:04 +0200
>> Neither GitHub nor GitLab have foreground green/red colors, only background
>> green/red colors.
>
> Speaking of GitHub and GitLab, I wonder if we should take another cue from
> them and tone down our background colors as well.
>
> This makes the new diffs more readable for me:
>
> (eval-after-load 'diff-mode
> '(progn
> (set-face-background 'diff-added "#eeffee")
> (set-face-background 'diff-removed "#ffeeee")
> (set-face-background 'diff-refine-added "#d0ffd0")
> (set-face-background 'diff-refine-removed "#ffd0d0")
> ))
Good idea.
But please see in https://debbugs.gnu.org/10181#68
why red and green colors are not symmetrical and
how is was wrong on GitHub.
After my post GitHub realized their mistake and
now they use #fbb instead of #faa.
So instead of #cfc a better color for refine-added is #bfb.
Here's a table with additional columns labeled "appr" (approximated)
where colors are rounded to the closest shorthand hex triplet:
GitHub appr GitLab appr current new better
removed #ffeef0 #fee #fbe9eb #fee #ffdddd #ffeeee
added #e6ffed #efe #ecfdf0 #efe #ddffdd #eeffee
refine-removed #fdb8c0 #fbb #fac5cd #fcc #ffbbbb #ffd0d0 #ffcccc
refine-added #acf2bd #aeb #c7f0d2 #cec #aaffaa #d0ffd0 #bbffbb
Your proposed new colors for added/removed are the same that are used
GitHub/GitLab, so this should be a good change. For refine-removed
better to use GitLab's color #ffcccc that is very close to the
color you proposed. But for refine-added GitLab made the same mistake
that GitHub already fixed. So the best color for refine-added is #bbffbb.
While at it, please also tone down file/hunk header colors:
diff-header from grey80 to grey90 and
diff-file-header from grey70 to grey80
This will produce nice-looking colors.
bug-gnu-emacs <at> gnu.org:bug#33567; Package emacs.
(Mon, 24 Dec 2018 02:30:02 GMT) Full text and rfc822 format available.Message #139 received at 33567 <at> debbugs.gnu.org (full text, mbox):
From: Dmitry Gutov <dgutov <at> yandex.ru> To: Juri Linkov <juri <at> linkov.net> Cc: 33567 <at> debbugs.gnu.org Subject: Re: bug#33567: Syntactic fontification of diff hunks Date: Mon, 24 Dec 2018 04:29:39 +0200
On 21.12.2018 0:00, Juri Linkov wrote: > I don't know. We have two options for tty: highlight indicators only > or use red/green foreground without syntax highlighting. Ouch. All this time I've been talking about a personal customization that's too old for me to remember about. Sorry about that, I'm dropping that part of the proposal. Speaking of ttys, green and red look foreground looks kind of okay for me in that context. However, diff-refine-* faces are simply defined as :inverse-video. And now, with the new feature, when e.g. there is a face that spans the whole line (like a comment), both diff-refine-added and diff-refine-removed faces look the same. Which is a problem. I don't have easy suggestions except defining specific background for them instead of inverse-video. And at that point we can drop the green/red foregrounds if people prefer. > By default it used the black foreground. Only Magit uses red/green foreground. So I'd propose Magit to follow the core here. And, like we discussed, other popular tools. > Maybe with conditional face definitions like > > (if (boundp 'diff-font-lock-syntax) > (defface blabla)) Fair enough.
bug-gnu-emacs <at> gnu.org:bug#33567; Package emacs.
(Tue, 25 Dec 2018 20:53:02 GMT) Full text and rfc822 format available.Message #142 received at 33567 <at> debbugs.gnu.org (full text, mbox):
From: Juri Linkov <juri <at> linkov.net> To: Dmitry Gutov <dgutov <at> yandex.ru> Cc: 33567 <at> debbugs.gnu.org Subject: Re: bug#33567: Syntactic fontification of diff hunks Date: Tue, 25 Dec 2018 22:35:23 +0200
> Speaking of ttys, green and red look foreground looks kind of okay for me > in that context. However, diff-refine-* faces are simply defined > as :inverse-video. And now, with the new feature, when e.g. there is a face > that spans the whole line (like a comment), both diff-refine-added and > diff-refine-removed faces look the same. Which is a problem. I don't have > easy suggestions except defining specific background for them instead of > inverse-video. And at that point we can drop the green/red foregrounds if > people prefer. What do you think about this solution that combines both green/red foreground colors with syntax colors: https://gist.github.com/skanev/0eeb943e3111a1df55fd
bug-gnu-emacs <at> gnu.org:bug#33567; Package emacs.
(Tue, 25 Dec 2018 20:53:03 GMT) Full text and rfc822 format available.Message #145 received at 33567 <at> debbugs.gnu.org (full text, mbox):
From: Juri Linkov <juri <at> linkov.net> To: Dmitry Gutov <dgutov <at> yandex.ru> Cc: 33567 <at> debbugs.gnu.org Subject: Re: bug#33567: Syntactic fontification of diff hunks Date: Tue, 25 Dec 2018 22:39:41 +0200
> Your proposed new colors for added/removed are the same that are used > GitHub/GitLab, so this should be a good change. For refine-removed > better to use GitLab's color #ffcccc that is very close to the > color you proposed. But for refine-added GitLab made the same mistake > that GitHub already fixed. So the best color for refine-added is #bbffbb. After trying to use there colors, I see that their shade is too subtle. They might look better on large hunks, and I'm not sure why they look ok in the browser, but in Emacs refined colors for small changes are almost not noticeable. However, please change them if majority agrees.
bug-gnu-emacs <at> gnu.org:bug#33567; Package emacs.
(Tue, 25 Dec 2018 21:16:02 GMT) Full text and rfc822 format available.Message #148 received at 33567 <at> debbugs.gnu.org (full text, mbox):
From: Dmitry Gutov <dgutov <at> yandex.ru> To: Juri Linkov <juri <at> linkov.net> Cc: 33567 <at> debbugs.gnu.org Subject: Re: bug#33567: Syntactic fontification of diff hunks Date: Tue, 25 Dec 2018 23:15:19 +0200
On 25.12.2018 22:35, Juri Linkov wrote: > What do you think about this solution that combines both > green/red foreground colors with syntax colors: > > https://gist.github.com/skanev/0eeb943e3111a1df55fd This is almost the look I complained about previously, but if the green and red are not as sharp, I suppose it's not too terrible. At least the lighter red doesn't scream "errors" to me. But note that this example does not show any "refined" functionality.
bug-gnu-emacs <at> gnu.org:bug#33567; Package emacs.
(Wed, 26 Dec 2018 00:44:01 GMT) Full text and rfc822 format available.Message #151 received at 33567 <at> debbugs.gnu.org (full text, mbox):
From: Dmitry Gutov <dgutov <at> yandex.ru> To: Juri Linkov <juri <at> linkov.net> Cc: 33567 <at> debbugs.gnu.org Subject: Re: bug#33567: Syntactic fontification of diff hunks Date: Wed, 26 Dec 2018 02:43:08 +0200
On 21.12.2018 0:17, Juri Linkov wrote: > But please see in https://debbugs.gnu.org/10181#68 > why red and green colors are not symmetrical and > how is was wrong on GitHub. Thanks! (TBC in another email). > Your proposed new colors for added/removed are the same that are used > GitHub/GitLab, so this should be a good change. Now pushed, thank you. > While at it, please also tone down file/hunk header colors: > diff-header from grey80 to grey90 and > diff-file-header from grey70 to grey80 I see you have done that yourself, looks good.
bug-gnu-emacs <at> gnu.org:bug#33567; Package emacs.
(Wed, 26 Dec 2018 02:49:01 GMT) Full text and rfc822 format available.Message #154 received at 33567 <at> debbugs.gnu.org (full text, mbox):
From: Dmitry Gutov <dgutov <at> yandex.ru> To: Juri Linkov <juri <at> linkov.net> Cc: 33567 <at> debbugs.gnu.org Subject: Re: bug#33567: Syntactic fontification of diff hunks Date: Wed, 26 Dec 2018 03:40:46 +0200
[Message part 1 (text/plain, inline)]
On 25.12.2018 22:39, Juri Linkov wrote: >> Your proposed new colors for added/removed are the same that are used >> GitHub/GitLab, so this should be a good change. For refine-removed >> better to use GitLab's color #ffcccc that is very close to the >> color you proposed. But for refine-added GitLab made the same mistake >> that GitHub already fixed. So the best color for refine-added is #bbffbb. > > After trying to use there colors, I see that their shade is too subtle. > They might look better on large hunks, and I'm not sure why they look ok > in the browser, but in Emacs refined colors for small changes are almost > not noticeable. However, please change them if majority agrees. To my eyes, that's a surprising conclusion. I wasn't going to argue with your correction to refine-added, even though I might prefer a slightly lighter variation (because I end up looking at larger refined regions often). Are you now saying that #ffcccc for refine-removed (or #d0ffd0, the difference is visible only on large regions) and #bbffbb for refine-added are hard for you to notice on smaller regions? It most likely depends on the choice of the font, the contrast levels of the monitor and similar stuff, but during the same several days I have not met the same experience. Before we get into deeper discussion (as well as discussing how one finds out majority's opinion), I have to ask: did you make sure to use the new refined colors with the new diff-added and diff-removed background colors? Attached are the screenshots of the default theme with my preferred font (Inconsolata LGC) and refined colors from your counter-proposal. The default Ubuntu's monospaced font is even more prominent and arguably readable, I can make screenshots with it as well if you like. This is commit a94ac604d8. We can also note that GitHub only refines smaller chunks: https://github.com/emacs-mirror/emacs/commit/a94ac604d8c9848b0414ade80a1920b345161656, so its use of darker backgrounds is more justifiable. What do you think of the screenshots? Are the small refined regions hard for you to see? Or do they look very different in your Emacs?
[Screenshot from 2018-12-26 03-28-06.png (image/png, attachment)]
[Screenshot from 2018-12-26 03-28-44.png (image/png, attachment)]
[Screenshot from 2018-12-26 03-29-20.png (image/png, attachment)]
bug-gnu-emacs <at> gnu.org:bug#33567; Package emacs.
(Wed, 26 Dec 2018 23:07:01 GMT) Full text and rfc822 format available.Message #157 received at 33567 <at> debbugs.gnu.org (full text, mbox):
From: Juri Linkov <juri <at> linkov.net> To: Dmitry Gutov <dgutov <at> yandex.ru> Cc: 33567 <at> debbugs.gnu.org Subject: Re: bug#33567: Syntactic fontification of diff hunks Date: Thu, 27 Dec 2018 00:49:44 +0200
>> What do you think about this solution that combines both >> green/red foreground colors with syntax colors: >> >> https://gist.github.com/skanev/0eeb943e3111a1df55fd > > This is almost the look I complained about previously, but if the green and > red are not as sharp, I suppose it's not too terrible. At least the lighter > red doesn't scream "errors" to me. > > But note that this example does not show any "refined" functionality. There are some underlines on this screenshot, maybe use underlines for refined diffs on tty?
bug-gnu-emacs <at> gnu.org:bug#33567; Package emacs.
(Wed, 26 Dec 2018 23:07:02 GMT) Full text and rfc822 format available.Message #160 received at 33567 <at> debbugs.gnu.org (full text, mbox):
From: Juri Linkov <juri <at> linkov.net> To: Dmitry Gutov <dgutov <at> yandex.ru> Cc: 33567 <at> debbugs.gnu.org Subject: Re: bug#33567: Syntactic fontification of diff hunks Date: Thu, 27 Dec 2018 00:59:37 +0200
>>> Your proposed new colors for added/removed are the same that are used >>> GitHub/GitLab, so this should be a good change. For refine-removed >>> better to use GitLab's color #ffcccc that is very close to the >>> color you proposed. But for refine-added GitLab made the same mistake >>> that GitHub already fixed. So the best color for refine-added is #bbffbb. >> >> After trying to use there colors, I see that their shade is too subtle. >> They might look better on large hunks, and I'm not sure why they look ok >> in the browser, but in Emacs refined colors for small changes are almost >> not noticeable. However, please change them if majority agrees. > > To my eyes, that's a surprising conclusion. > > I wasn't going to argue with your correction to refine-added, even though > I might prefer a slightly lighter variation (because I end up looking at > larger refined regions often). Are you now saying that #ffcccc for > refine-removed (or #d0ffd0, the difference is visible only on large > regions) and #bbffbb for refine-added are hard for you to notice on > smaller regions? I meant that added/removed #eeffee/#ffeeee are harder to notice, and really only on distant corners of the monitor. Also looking down at the LCD screen from a sharp angle can see the inverted colors: green instead of red, and red instead of green :) So color choice is not the exact sciences. > Before we get into deeper discussion (as well as discussing how one finds > out majority's opinion), I have to ask: did you make sure to use the new > refined colors with the new diff-added and diff-removed background colors? Since #eeffee/#ffeeee colors were tested on many users of GitHub/GitLab, please keep them in diff-mode and also install the accompanying change of refine-added/refine-removed. > This is commit a94ac604d8. We can also note that GitHub only refines > smaller chunks: GitHub only refines smaller chunks, but refining large chunks often helps to see real changes with code indentation, e.g. when a let-binding form is added and thus whitespace of indentation shifts the code block. > https://github.com/emacs-mirror/emacs/commit/a94ac604d8c9848b0414ade80a1920b345161656, > so its use of darker backgrounds is more justifiable. > > What do you think of the screenshots? Are the small refined regions hard > for you to see? Or do they look very different in your Emacs? I think this is fine, I have no problems with refine-added/refine-removed, please install refine-added/refine-removed as well.
bug-gnu-emacs <at> gnu.org:bug#33567; Package emacs.
(Wed, 26 Dec 2018 23:17:01 GMT) Full text and rfc822 format available.Message #163 received at 33567 <at> debbugs.gnu.org (full text, mbox):
From: Dmitry Gutov <dgutov <at> yandex.ru> To: Juri Linkov <juri <at> linkov.net> Cc: 33567 <at> debbugs.gnu.org Subject: Re: bug#33567: Syntactic fontification of diff hunks Date: Thu, 27 Dec 2018 01:16:09 +0200
On 27.12.2018 0:49, Juri Linkov wrote: > There are some underlines on this screenshot, maybe use underlines for > refined diffs on tty? You mean colored underlines for every line? Umm, not sure I'd like that. But I never use Emacs in tty, so maybe better ask on emacs-devel.
bug-gnu-emacs <at> gnu.org:bug#33567; Package emacs.
(Wed, 26 Dec 2018 23:57:02 GMT) Full text and rfc822 format available.Message #166 received at 33567 <at> debbugs.gnu.org (full text, mbox):
From: Dmitry Gutov <dgutov <at> yandex.ru> To: Juri Linkov <juri <at> linkov.net> Cc: 33567 <at> debbugs.gnu.org Subject: Re: bug#33567: Syntactic fontification of diff hunks Date: Thu, 27 Dec 2018 01:56:33 +0200
On 27.12.2018 0:59, Juri Linkov wrote:
> I meant that added/removed #eeffee/#ffeeee are harder to notice,
> and really only on distant corners of the monitor. Also looking down
> at the LCD screen from a sharp angle can see the inverted colors:
> green instead of red, and red instead of green :)
Oh, I see. IIRC, TN screens can exhibit this problem (poorer viewing
angles), especially the older ones.
> So color choice is not the exact sciences.
It's subjective, of course.
> Since #eeffee/#ffeeee colors were tested on many users of
> GitHub/GitLab, please keep them in diff-mode and also install
> the accompanying change of refine-added/refine-removed.
OK, installed. Thanks for testing!
> GitHub only refines smaller chunks, but refining large chunks often
> helps to see real changes with code indentation, e.g. when a let-binding
> form is added and thus whitespace of indentation shifts the code block.
Of course. It's definitely useful (though it can incur some noticeable
delay then a refined region is big enough).
What I'm saying is, reading text on a darker red background is not very
easy or enjoyable. So I think we want to have the diff-refine-* faces to
be as light as possible while still easy enough to discern from
diff-added and diff-removed.
But GitHub and GitLab don't have that urgency. That's why our colors are
lighter already.
With that in mind, I'm going to experiment with even lighter colors,
similar to what I originally proposed, in my private config:
(set-face-background 'diff-refine-added "#d0ffd0")
(set-face-background 'diff-refine-removed "#ffd6d6")
Maybe try them out sometime.
bug-gnu-emacs <at> gnu.org:bug#33567; Package emacs.
(Thu, 27 Dec 2018 00:22:01 GMT) Full text and rfc822 format available.Message #169 received at 33567 <at> debbugs.gnu.org (full text, mbox):
From: Juri Linkov <juri <at> linkov.net> To: Dmitry Gutov <dgutov <at> yandex.ru> Cc: 33567 <at> debbugs.gnu.org Subject: Re: bug#33567: Syntactic fontification of diff hunks Date: Thu, 27 Dec 2018 02:18:14 +0200
[Message part 1 (text/plain, inline)]
>> There are some underlines on this screenshot, maybe use underlines for >> refined diffs on tty? > > You mean colored underlines for every line? Umm, not sure I'd like that. Actually I meant underlines instead of inverse-video. Underlined refinement looks much better than in inverse-video. Also I propose to not use this feature on tty because there is no good solution how to combine diff foreground colors with syntax foreground colors:
[diff-mode.tty.patch (text/x-diff, inline)]
diff --git a/lisp/vc/diff-mode.el b/lisp/vc/diff-mode.el
index a4c0618c67..7ab64fa4aa 100644
--- a/lisp/vc/diff-mode.el
+++ b/lisp/vc/diff-mode.el
@@ -104,7 +104,7 @@ diff-font-lock-prettify
:version "27.1"
:type 'boolean)
-(defcustom diff-font-lock-syntax t
+(defcustom diff-font-lock-syntax (>= (display-color-cells) 88)
"If non-nil, diff hunk font-lock includes source language syntax highlighting.
This highlighting is the same as added by `font-lock-mode'
when corresponding source files are visited normally.
@@ -2019,7 +2019,7 @@ diff-refine-changed
:background "#ffff55")
(((class color) (min-colors 88) (background dark))
:background "#aaaa22")
- (t :inverse-video t))
+ (t :underline t))
"Face used for char-based changes shown by `diff-refine-hunk'.")
(defface diff-refine-removed
bug-gnu-emacs <at> gnu.org:bug#33567; Package emacs.
(Thu, 27 Dec 2018 00:47:04 GMT) Full text and rfc822 format available.Message #172 received at 33567 <at> debbugs.gnu.org (full text, mbox):
From: Dmitry Gutov <dgutov <at> yandex.ru> To: Juri Linkov <juri <at> linkov.net> Cc: 33567 <at> debbugs.gnu.org Subject: Re: bug#33567: Syntactic fontification of diff hunks Date: Thu, 27 Dec 2018 02:45:55 +0200
On 27.12.2018 2:18, Juri Linkov wrote: > Actually I meant underlines instead of inverse-video. Underlined refinement > looks much better than in inverse-video. OK. But I went to try it out, and looks like underlines don't work on tty. Or at least not on mine. > Also I propose to not use this feature on tty because there is no good > solution how to combine diff foreground colors with syntax foreground colors: *Shrug*, personally I'd leave the config as it is now. The current behavior might still be more useful than simply green or red lines.
bug-gnu-emacs <at> gnu.org:bug#33567; Package emacs.
(Thu, 27 Dec 2018 03:33:01 GMT) Full text and rfc822 format available.Message #175 received at 33567 <at> debbugs.gnu.org (full text, mbox):
From: Eli Zaretskii <eliz <at> gnu.org> To: Dmitry Gutov <dgutov <at> yandex.ru> Cc: 33567 <at> debbugs.gnu.org, juri <at> linkov.net Subject: Re: bug#33567: Syntactic fontification of diff hunks Date: Thu, 27 Dec 2018 05:32:31 +0200
> From: Dmitry Gutov <dgutov <at> yandex.ru> > Date: Thu, 27 Dec 2018 01:16:09 +0200 > Cc: 33567 <at> debbugs.gnu.org > > On 27.12.2018 0:49, Juri Linkov wrote: > > > There are some underlines on this screenshot, maybe use underlines for > > refined diffs on tty? > > You mean colored underlines for every line? Umm, not sure I'd like that. I _know_ I won't. It's ugly and, frankly, inappropriate for syntax highlighting.
bug-gnu-emacs <at> gnu.org:bug#33567; Package emacs.
(Thu, 27 Dec 2018 03:35:02 GMT) Full text and rfc822 format available.Message #178 received at 33567 <at> debbugs.gnu.org (full text, mbox):
From: Eli Zaretskii <eliz <at> gnu.org> To: Juri Linkov <juri <at> linkov.net> Cc: 33567 <at> debbugs.gnu.org, dgutov <at> yandex.ru Subject: Re: bug#33567: Syntactic fontification of diff hunks Date: Thu, 27 Dec 2018 05:34:37 +0200
> From: Juri Linkov <juri <at> linkov.net> > Date: Thu, 27 Dec 2018 02:18:14 +0200 > Cc: 33567 <at> debbugs.gnu.org > > > You mean colored underlines for every line? Umm, not sure I'd like that. > > Actually I meant underlines instead of inverse-video. Underlined refinement > looks much better than in inverse-video. > > Also I propose to not use this feature on tty because there is no good > solution how to combine diff foreground colors with syntax foreground colors: Please don't.
bug-gnu-emacs <at> gnu.org:bug#33567; Package emacs.
(Thu, 27 Dec 2018 20:48:02 GMT) Full text and rfc822 format available.Message #181 received at 33567 <at> debbugs.gnu.org (full text, mbox):
From: Juri Linkov <juri <at> linkov.net> To: Dmitry Gutov <dgutov <at> yandex.ru> Cc: 33567 <at> debbugs.gnu.org Subject: Re: bug#33567: Syntactic fontification of diff hunks Date: Thu, 27 Dec 2018 22:39:16 +0200
> With that in mind, I'm going to experiment with even lighter colors, > similar to what I originally proposed, in my private config: > > (set-face-background 'diff-refine-added "#d0ffd0") > (set-face-background 'diff-refine-removed "#ffd6d6") > > Maybe try them out sometime. These are better on large chunks, but harder to notice on smaller ones.
bug-gnu-emacs <at> gnu.org:bug#33567; Package emacs.
(Sat, 29 Dec 2018 23:25:02 GMT) Full text and rfc822 format available.Message #184 received at 33567 <at> debbugs.gnu.org (full text, mbox):
From: Juri Linkov <juri <at> linkov.net> To: Dmitry Gutov <dgutov <at> yandex.ru> Cc: 33567 <at> debbugs.gnu.org Subject: Re: bug#33567: Syntactic fontification of diff hunks Date: Sun, 30 Dec 2018 01:07:12 +0200
>> With that in mind, I'm going to experiment with even lighter colors, >> similar to what I originally proposed, in my private config: >> >> (set-face-background 'diff-refine-added "#d0ffd0") >> (set-face-background 'diff-refine-removed "#ffd6d6") >> >> Maybe try them out sometime. > > These are better on large chunks, but harder to notice on smaller ones. Or we could add more faces: lighter refine-added/removed for multi-line chunks, and darker refine-added/removed faces for one-line differences.
bug-gnu-emacs <at> gnu.org:bug#33567; Package emacs.
(Sun, 30 Dec 2018 23:08:02 GMT) Full text and rfc822 format available.Message #187 received at 33567 <at> debbugs.gnu.org (full text, mbox):
From: Dmitry Gutov <dgutov <at> yandex.ru> To: Juri Linkov <juri <at> linkov.net> Cc: 33567 <at> debbugs.gnu.org Subject: Re: bug#33567: Syntactic fontification of diff hunks Date: Mon, 31 Dec 2018 02:07:50 +0300
On 30.12.2018 2:07, Juri Linkov wrote: > Or we could add more faces: lighter refine-added/removed for multi-line chunks, > and darker refine-added/removed faces for one-line differences. It could work. But I'm not the best judge of the result since the lighter ones work fine for me in all contexts. Hopefully somebody else decides to join the discussion.
Debbugs Internal Request <help-debbugs <at> gnu.org>
to internal_control <at> debbugs.gnu.org.
(Mon, 28 Jan 2019 12:24:04 GMT) Full text and rfc822 format available.
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.