From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.bugs Subject: bug#33567: Syntactic fontification of diff hunks Date: Sun, 02 Dec 2018 08:56:44 +0200 Message-ID: <83a7lobemr.fsf@gnu.org> References: <878t18j4is.fsf@mail.linkov.net> NNTP-Posting-Host: blaine.gmane.org X-Trace: blaine.gmane.org 1543733767 27641 195.159.176.226 (2 Dec 2018 06:56:07 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Sun, 2 Dec 2018 06:56:07 +0000 (UTC) Cc: 33567@debbugs.gnu.org To: Juri Linkov Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Sun Dec 02 07:56:03 2018 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by blaine.gmane.org with esmtp (Exim 4.84_2) (envelope-from ) id 1gTLfe-000738-HJ for geb-bug-gnu-emacs@m.gmane.org; Sun, 02 Dec 2018 07:56:02 +0100 Original-Received: from localhost ([::1]:43625 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gTLhl-0007Cl-9F for geb-bug-gnu-emacs@m.gmane.org; Sun, 02 Dec 2018 01:58:13 -0500 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:34092) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gTLhf-0007CT-7Q for bug-gnu-emacs@gnu.org; Sun, 02 Dec 2018 01:58:08 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1gTLha-0005QJ-CL for bug-gnu-emacs@gnu.org; Sun, 02 Dec 2018 01:58:07 -0500 Original-Received: from debbugs.gnu.org ([208.118.235.43]:53222) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1gTLha-0005P7-8c for bug-gnu-emacs@gnu.org; Sun, 02 Dec 2018 01:58:02 -0500 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1gTLha-0008FX-47 for bug-gnu-emacs@gnu.org; Sun, 02 Dec 2018 01:58:02 -0500 X-Loop: help-debbugs@gnu.org Resent-From: Eli Zaretskii Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Sun, 02 Dec 2018 06:58:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 33567 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: patch Original-Received: via spool by 33567-submit@debbugs.gnu.org id=B33567.154373383231651 (code B ref 33567); Sun, 02 Dec 2018 06:58:02 +0000 Original-Received: (at 33567) by debbugs.gnu.org; 2 Dec 2018 06:57:12 +0000 Original-Received: from localhost ([127.0.0.1]:57480 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1gTLgk-0008EP-0z for submit@debbugs.gnu.org; Sun, 02 Dec 2018 01:57:11 -0500 Original-Received: from eggs.gnu.org ([208.118.235.92]:52556) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1gTLgi-0008EA-EO for 33567@debbugs.gnu.org; Sun, 02 Dec 2018 01:57:08 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1gTLgY-0004qM-6h for 33567@debbugs.gnu.org; Sun, 02 Dec 2018 01:57:03 -0500 Original-Received: from fencepost.gnu.org ([2001:4830:134:3::e]:47961) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gTLgY-0004qI-3j; Sun, 02 Dec 2018 01:56:58 -0500 Original-Received: from [176.228.60.248] (port=3681 helo=home-c4e4a596f7) by fencepost.gnu.org with esmtpsa (TLS1.2:RSA_AES_256_CBC_SHA1:256) (Exim 4.82) (envelope-from ) id 1gTLgX-0001El-O5; Sun, 02 Dec 2018 01:56:58 -0500 In-reply-to: <878t18j4is.fsf@mail.linkov.net> (message from Juri Linkov on Sat, 01 Dec 2018 23:55:40 +0200) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 208.118.235.43 X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Original-Sender: "bug-gnu-emacs" Xref: news.gmane.org gmane.emacs.bugs:152987 Archived-At: > From: Juri Linkov > 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.