bug#33567: Syntactic fontification of diff hunks

[Eli Zaretskii]

> From: Juri Linkov <juri@linkov.net>
> 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.