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: Mon, 03 Dec 2018 08:49:15 +0200 Message-ID: <83pnuj9kb8.fsf@gnu.org> References: <878t18j4is.fsf@mail.linkov.net> <83a7lobemr.fsf@gnu.org> <87a7lnv6ex.fsf@mail.linkov.net> NNTP-Posting-Host: blaine.gmane.org X-Trace: blaine.gmane.org 1543819689 10577 195.159.176.226 (3 Dec 2018 06:48:09 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Mon, 3 Dec 2018 06:48:09 +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 Mon Dec 03 07:48:05 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 1gTi1T-0002bk-37 for geb-bug-gnu-emacs@m.gmane.org; Mon, 03 Dec 2018 07:48:03 +0100 Original-Received: from localhost ([::1]:47094 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gTi3Z-0005TL-AD for geb-bug-gnu-emacs@m.gmane.org; Mon, 03 Dec 2018 01:50:13 -0500 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:48259) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gTi3T-0005TE-0t for bug-gnu-emacs@gnu.org; Mon, 03 Dec 2018 01:50:08 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1gTi3P-0007LK-3Z for bug-gnu-emacs@gnu.org; Mon, 03 Dec 2018 01:50:06 -0500 Original-Received: from debbugs.gnu.org ([208.118.235.43]:54388) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1gTi3O-0007LA-RG for bug-gnu-emacs@gnu.org; Mon, 03 Dec 2018 01:50:03 -0500 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1gTi3O-0006Qv-81 for bug-gnu-emacs@gnu.org; Mon, 03 Dec 2018 01:50: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: Mon, 03 Dec 2018 06:50: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.154381977924695 (code B ref 33567); Mon, 03 Dec 2018 06:50:02 +0000 Original-Received: (at 33567) by debbugs.gnu.org; 3 Dec 2018 06:49:39 +0000 Original-Received: from localhost ([127.0.0.1]:58646 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1gTi30-0006QE-I7 for submit@debbugs.gnu.org; Mon, 03 Dec 2018 01:49:38 -0500 Original-Received: from eggs.gnu.org ([208.118.235.92]:38569) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1gTi2z-0006Q2-1C for 33567@debbugs.gnu.org; Mon, 03 Dec 2018 01:49:37 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1gTi2o-00071E-Ns for 33567@debbugs.gnu.org; Mon, 03 Dec 2018 01:49:31 -0500 Original-Received: from fencepost.gnu.org ([2001:4830:134:3::e]:47718) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gTi2o-000713-KR; Mon, 03 Dec 2018 01:49:26 -0500 Original-Received: from [176.228.60.248] (port=1280 helo=home-c4e4a596f7) by fencepost.gnu.org with esmtpsa (TLS1.2:RSA_AES_256_CBC_SHA1:256) (Exim 4.82) (envelope-from ) id 1gTi2o-0006h6-1F; Mon, 03 Dec 2018 01:49:26 -0500 In-reply-to: <87a7lnv6ex.fsf@mail.linkov.net> (message from Juri Linkov on Mon, 03 Dec 2018 02:34:14 +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:153017 Archived-At: > From: Juri Linkov > Cc: 33567@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.