GNU bug report logs - #33567
Syntactic fontification of diff hunks

Previous Next

Full log

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.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.