[NonGNU ELPA] New package: eldoc-diffstat

[NonGNU ELPA] New package: eldoc-diffstat

[Johann Klähn]

I would like to submit a new package to NonGNU ELPA: eldoc-diffstat.

  https://github.com/kljohann/eldoc-diffstat/

It provides a way to display VCS diffstat information via eldoc.  I.e.,
if point rests on a commit in a magit, vc-annotate, or log-view buffer,
the Git or Mercurial diffstat summary will be shown in the echo area.
For example (though it uses ansi-colors and fontification, so you might
prefer to look at the screenshot in the repo):

  Johann Klähn <xyz@localhost>, Thu, 3 Oct 2024 15:45:10 +0200: Minor refactoring
   1 file changed, 93 insertions(+), 62 deletions(-)
   eldoc-diffstat.el | 155 ++++++++++++++++++++++++++++++++----------------------

It's adapted from Tassilo Horn's 2022 blog post “Using Eldoc with Magit
(asynchronously!)”: https://www.tsdh.org/posts/2022-07-20-using-eldoc-with-magit-async.html

Thanks
Johann

Re: [NonGNU ELPA] New package: eldoc-diffstat

[Tassilo Horn]

Johann Klähn <johann@jklaehn.de> writes:

Hi Johann,

> I would like to submit a new package to NonGNU ELPA: eldoc-diffstat.
>
>   https://github.com/kljohann/eldoc-diffstat/

I'm using it since a couple of days using package-vc and like it very
much, so I took the liberty to add it to NonGNU ELPA.

I think you could add a bit more configuration hints to the README.md,
e.g., where you say that one should call eldoc-diffstat-setup in the
desired buffer or mode hook, provide some typical examples like:

  (add-hook 'log-view-mode-hook #'eldoc-diffstat-setup)
  (add-hook 'vc-annotate-mode-hook #'eldoc-diffstat-setup)
  (add-hook 'magit-status-mode-hook #'eldoc-diffstat-setup)
  (add-hook 'magit-log-mode-hook #'eldoc-diffstat-setup)

Thanks for the package!
  Tassilo

Re: [NonGNU ELPA] New package: eldoc-diffstat

[Stefan Kangas]

Tassilo Horn <tsdh@gnu.org> writes:

>> I would like to submit a new package to NonGNU ELPA: eldoc-diffstat.
>>
>>   https://github.com/kljohann/eldoc-diffstat/
>
> I'm using it since a couple of days using package-vc and like it very
> much, so I took the liberty to add it to NonGNU ELPA.

Thanks, this seems pretty useful.  I can see myself wanting to have it
on sometimes, but not always, so I'd appreciate it being a minor mode
that I could toggle, instead of just a function.

When the mode is toggled on, it could detect automatically that it's in
a magit buffer, and do the `eldoc-add-command` setup for users
automatically.

Should we have something like this built-in?

Here are some more wishlist items:

- An option to use a maximum (or fixed) number of lines
- Caching the results for the current buffer to get instant results

> I think you could add a bit more configuration hints to the README.md,
> e.g., where you say that one should call eldoc-diffstat-setup in the
> desired buffer or mode hook, provide some typical examples like:
>
>   (add-hook 'log-view-mode-hook #'eldoc-diffstat-setup)
>   (add-hook 'vc-annotate-mode-hook #'eldoc-diffstat-setup)
>   (add-hook 'magit-status-mode-hook #'eldoc-diffstat-setup)
>   (add-hook 'magit-log-mode-hook #'eldoc-diffstat-setup)

+1

There could also be a global minor mode that would enable this in as
many places as possible.

Re: [NonGNU ELPA] New package: eldoc-diffstat

[Johann Klähn]

Thanks for your comments, everyone!


On Fri, Dec 13, 2024 at 21:23 +0100, Tassilo Horn wrote:
> I'm using it since a couple of days using package-vc and like it very
> much, so I took the liberty to add it to NonGNU ELPA.

Thanks!

> I think you could add a bit more configuration hints to the README.md,

I instead added the globalized minor mode proposed by Stefan, WDYT?


On Sat, Dec 14, 2024 at 13:24 +0000, Stefan Kangas wrote:
> Thanks, this seems pretty useful.  I can see myself wanting to have it
> on sometimes, but not always, so I'd appreciate it being a minor mode
> that I could toggle, instead of just a function.

Good idea, I added a minor mode plus the globalized minor mode you
suggested further below.

> When the mode is toggled on, it could detect automatically that it's in
> a magit buffer, and do the `eldoc-add-command` setup for users
> automatically.

I decided to unconditionally add these commands when the package is
loaded, since this (a) only has to be run once and (b) does not have an
effect unless eldoc is used in a magit buffer.  And in the latter case
the user will likely want these as triggers anyways.

> - An option to use a maximum (or fixed) number of lines

Now done, though eldoc by default strips trailing whitespace so echoing
a fixed number of lines required a kludge.  (But some people may prefer
the effect that the echo area size does not change between commits.)

> - Caching the results for the current buffer to get instant results

Do you mean eagerly compute diffstat?  Or just remember it indefinitely
after the first lookup of a commit?


On Sat, Dec 14, 2024 at 10:11 +0000, Philip Kaludercic wrote:
>    '((Git "git" "--no-pager" "show" "--color=always"
> -         "--format=format:%an <%ae>, %aD:%n%s" "--stat=80")
> +	 "--format=format:%an <%ae>, %aD:%n%s" "--stat=80")

Some lines look like spurious whitespace changes?  I hope I picked out
all functional changes.

> +  ;; Is it an issue that there is the slight possibility of a race
> +  ;; condition here?

I happened to have the same thought yesterday, also w.r.t. the
sentinel function.

>                                         The rewriting of
> `eldoc-diffstat--format-output-buffer' might be controversial, but I
> feel that using a regular expression to destruct the buffer feels more
> robust.

Yes, probably.  Though I decided to keep the original output if the
regex fails to match, as it might help users figure out what's happening.

Re: [NonGNU ELPA] New package: eldoc-diffstat

[Stefan Kangas]

Johann Klähn <johann@jklaehn.de> writes:

>> When the mode is toggled on, it could detect automatically that it's in
>> a magit buffer, and do the `eldoc-add-command` setup for users
>> automatically.
>
> I decided to unconditionally add these commands when the package is
> loaded, since this (a) only has to be run once and (b) does not have an
> effect unless eldoc is used in a magit buffer.  And in the latter case
> the user will likely want these as triggers anyways.

This is minor, but we usually recommend against changing behavior from
merely loading a file (as that can happen from merely saying `C-h f`),
so I'd move this to when the mode is loaded.

>> - Caching the results for the current buffer to get instant results
>
> Do you mean eagerly compute diffstat?  Or just remember it indefinitely
> after the first lookup of a commit?

I was thinking of computing it eagerly.  Either or should work, but
keeping it indefinitely might lead to its own issues, for example you
might want to eventually prune the list of commits for which you saved
the diffstat, and so on.

Re: [NonGNU ELPA] New package: eldoc-diffstat

[Philip Kaludercic]

Johann Klähn <johann@jklaehn.de> writes:

> Thanks for your comments, everyone!

[...]

> On Sat, Dec 14, 2024 at 10:11 +0000, Philip Kaludercic wrote:
>>    '((Git "git" "--no-pager" "show" "--color=always"
>> -         "--format=format:%an <%ae>, %aD:%n%s" "--stat=80")
>> +	 "--format=format:%an <%ae>, %aD:%n%s" "--stat=80")
>
> Some lines look like spurious whitespace changes?  I hope I picked out
> all functional changes.

That is true, my apologies.

>> +  ;; Is it an issue that there is the slight possibility of a race
>> +  ;; condition here?
>
> I happened to have the same thought yesterday, also w.r.t. the
> sentinel function.

As far as I see, there is no way of avoiding this without extending
`make-process' to accept a process plist (which wouldn't be backwards
compatible).  But if you don't insist on using the process plist, you
could capture the revision info in a closure and pass it on to the
sentinel.

>>                                         The rewriting of
>> `eldoc-diffstat--format-output-buffer' might be controversial, but I
>> feel that using a regular expression to destruct the buffer feels more
>> robust.
>
> Yes, probably.  Though I decided to keep the original output if the
> regex fails to match, as it might help users figure out what's happening.

That's fine as well of course!

Re: [NonGNU ELPA] New package: eldoc-diffstat

[Philip Kaludercic]

[-- Attachment #1: Type: text/plain, Size: 202 bytes --]

Johann Klähn <johann@jklaehn.de> writes:

> I would like to submit a new package to NonGNU ELPA: eldoc-diffstat.
>
>   https://github.com/kljohann/eldoc-diffstat/

I also have a few comments:


[-- Attachment #2: Type: text/plain, Size: 7786 bytes --]

diff --git a/eldoc-diffstat.el b/eldoc-diffstat.el
index 6433e3e..5bbcd33 100644
--- a/eldoc-diffstat.el
+++ b/eldoc-diffstat.el
@@ -55,10 +55,10 @@ a property to the process object.")
 
 (defconst eldoc-diffstat--commands
   '((Git "git" "--no-pager" "show" "--color=always"
-         "--format=format:%an <%ae>, %aD:%n%s" "--stat=80")
+	 "--format=format:%an <%ae>, %aD:%n%s" "--stat=80")
     (Hg "hg" "--pager=never" "log" "--color=always"
-        "--template" "{author}, {date|rfc822date}:\n{desc|firstline}\n"
-        "--stat" "--rev"))
+	"--template" "{author}, {date|rfc822date}:\n{desc|firstline}\n"
+	"--stat" "--rev"))
   "Alist mapping VCS backend to the command to use for computing the diffstat.")
 
 ;;;###autoload
@@ -66,7 +66,7 @@ a property to the process object.")
   "Configure eldoc buffer-locally to display diffstat for revision at point."
   (interactive)
   (add-hook 'eldoc-documentation-functions
-            #'eldoc-diffstat--docstring nil 'local)
+	    #'eldoc-diffstat--docstring nil 'local)
   (unless (bound-and-true-p eldoc-mode)
     (eldoc-mode)))
 
@@ -74,11 +74,11 @@ a property to the process object.")
   "Display diffstat for revision at point by calling CALLBACK.
 Intended for `eldoc-documentation-functions'."
   (when-let* ((info (eldoc-diffstat--get-revision-info))
-              (backend (car info))
-              (revision (cdr info))
-              (command (alist-get backend eldoc-diffstat--commands)))
-    (if-let ((result (eldoc-diffstat--get-cache info)))
-        (funcall callback result)
+	      (backend (car info))
+	      (revision (cdr info))
+	      (command (alist-get backend eldoc-diffstat--commands)))
+    (if-let* ((result (eldoc-diffstat--get-cache info)))
+	(funcall callback result)
       (eldoc-diffstat--docstring-1 (append command (list revision)) callback info))))
 
 (defun eldoc-diffstat--get-revision-info ()
@@ -87,34 +87,34 @@ The returned record should be a cons cell of the form (BACKEND . REVISION) where
 BACKEND is a symbol representing the version control system and REVISION is
 a string identifying the specific revision."
   (cond
-   ((when-let (((fboundp 'magit-stash-at-point))
-               (revision (magit-stash-at-point)))
+   ((when-let* (((fboundp 'magit-stash-at-point))
+		(revision (magit-stash-at-point)))
       (cons 'Git revision)))
-   ((when-let (((fboundp 'magit-commit-at-point))
-               (revision (magit-commit-at-point)))
+   ((when-let* (((fboundp 'magit-commit-at-point))
+		(revision (magit-commit-at-point)))
       (cons 'Git revision)))
    ((and (derived-mode-p 'git-rebase-mode)
-         (fboundp 'git-rebase-current-line)
-         (with-slots (action-type target)
-             (git-rebase-current-line)
-           (and (eq action-type 'commit)
-                (cons 'Git target)))))
+	 (fboundp 'git-rebase-current-line)
+	 (with-slots (action-type target)
+	     (git-rebase-current-line)
+	   (and (eq action-type 'commit)
+		(cons 'Git target)))))
    ((and (derived-mode-p 'vc-annotate-mode)
-         (boundp 'vc-annotate-backend)
-         (fboundp 'vc-annotate-extract-revision-at-line))
+	 (boundp 'vc-annotate-backend)
+	 (fboundp 'vc-annotate-extract-revision-at-line))
     (cons vc-annotate-backend
-          (car (vc-annotate-extract-revision-at-line))))
+	  (car (vc-annotate-extract-revision-at-line))))
    ((and (derived-mode-p 'log-view-mode)
-         (boundp 'log-view-vc-backend))
+	 (boundp 'log-view-vc-backend))
     (cons log-view-vc-backend
-          (log-view-current-tag)))))
+	  (log-view-current-tag)))))
 
 (defun eldoc-diffstat--get-cache (revision-info)
   "Retrieve cached diffstat result for REVISION-INFO if available."
   (when-let* ((proc eldoc-diffstat--process)
-              ((processp proc))
-              (cached-result (process-get proc :cached-result))
-              ((equal revision-info (car cached-result))))
+	      ((processp proc))
+	      (cached-result (process-get proc :cached-result))
+	      ((equal revision-info (car cached-result))))
     (cdr cached-result)))
 
 (defun eldoc-diffstat--docstring-1 (command callback revision-info)
@@ -126,7 +126,7 @@ caching the result, see `eldoc-diffstat--get-cache' for details."
   (when (processp eldoc-diffstat--process)
     (when (process-live-p eldoc-diffstat--process)
       (let (confirm-kill-processes)
-        (kill-process eldoc-diffstat--process)))
+	(kill-process eldoc-diffstat--process)))
     (kill-buffer (process-buffer eldoc-diffstat--process)))
 
   (setq
@@ -137,8 +137,9 @@ caching the result, see `eldoc-diffstat--get-cache' for details."
     :noquery t
     :command command
     :sentinel
-    (lambda (&rest args)
-      (apply #'eldoc-diffstat--sentinel callback args))))
+    (apply-partially #'eldoc-diffstat--sentinel callback)))
+  ;; Is it an issue that there is the slight possibility of a race
+  ;; condition here?
   (process-put eldoc-diffstat--process :revision-info revision-info)
 
   ;; Signal that the doc string is computed asynchronously.
@@ -150,41 +151,47 @@ caching the result, see `eldoc-diffstat--get-cache' for details."
     (with-current-buffer (process-buffer proc)
       (eldoc-diffstat--format-output-buffer)
       (let ((result (buffer-string))
-            (revision-info (process-get eldoc-diffstat--process :revision-info)))
-        (process-put eldoc-diffstat--process :cached-result
-                     (cons revision-info result))
-        (funcall callback result)))))
+	    (revision-info (process-get eldoc-diffstat--process :revision-info)))
+	(process-put eldoc-diffstat--process :cached-result
+		     (cons revision-info result))
+	(funcall callback result)))))
 
 (defun eldoc-diffstat--format-output-buffer ()
-  "Format the diffstat output."
+  "Format the diffstat output in the current buffer."
   ;; Translate color control sequences into text properties.
   (let ((ansi-color-apply-face-function
-         (lambda (beg end face)
-           (put-text-property beg end 'face face))))
+	 (lambda (beg end face)
+	   (put-text-property beg end 'face face))))
     (ansi-color-apply-on-region (point-min) (point-max)))
 
-  ;; Delete trailing blank lines.
-  (goto-char (point-max))
-  (delete-blank-lines)
-
-  ;; Make first line bold.
   (goto-char (point-min))
-  (put-text-property (point)
-                     (line-end-position)
-                     'face 'bold)
-
-  ;; Join second line.
-  (forward-line)
-  (join-line)
-
-  ;; Move summary to the top and make it italic.
-  (forward-line)
-  (reverse-region (point) (point-max))
-  (put-text-property (point)
-                     (line-end-position)
-                     'face 'italic)
-  (forward-line)
-  (reverse-region (point) (point-max)))
+  (unless (looking-at
+	   (rx buffer-start point
+	       ;; Commit author and date:
+	       (group (+ not-newline)) (group ?\n)
+	       ;; First lie of the commit message
+	       (group (+ not-newline)) (group ?\n)
+	       ;; File changes
+	       (group (+? anything))
+	       ;; Commit Summary
+	       (group (+ not-newline))
+	       (* space) buffer-end))
+    ;; If this occurs, then we know that the buffer doesn't look have
+    ;; the expected textual form!
+    (error "Malformed diffstat"))
+
+  (put-text-property			;make first line bold
+   (match-beginning 1) (match-end 1)
+   'face 'bold)
+  (replace-match " " nil nil nil 2)	;join the first two lines
+  (reverse-region			;reverse the diffstat order
+   (match-beginning 5) (match-end 5))
+  (let ((summary (delete-and-extract-region
+		  (match-beginning 6)
+		  (match-end 6))))
+    (replace-match			;move summary up and italicise
+     (concat "\n" (propertize summary 'face 'italic) "\n")
+     nil nil nil 4)))
 
 (provide 'eldoc-diffstat)
 ;;; eldoc-diffstat.el ends here

[-- Attachment #3: Type: text/plain, Size: 1189 bytes --]


To summarise, when-let/if-let have been deprecated in Emacs 31 so it is
best to stick to the star'ed variants.  The rewriting of
`eldoc-diffstat--format-output-buffer' might be controversial, but I
feel that using a regular expression to destruct the buffer feels more
robust.

It would also be nice if you could add a .elpaignore file to exclude the
screenshot from being bundled with the tarball.

> It provides a way to display VCS diffstat information via eldoc.  I.e.,
> if point rests on a commit in a magit, vc-annotate, or log-view buffer,
> the Git or Mercurial diffstat summary will be shown in the echo area.
> For example (though it uses ansi-colors and fontification, so you might
> prefer to look at the screenshot in the repo):
>
>   Johann Klähn <xyz@localhost>, Thu, 3 Oct 2024 15:45:10 +0200: Minor refactoring
>    1 file changed, 93 insertions(+), 62 deletions(-)
>    eldoc-diffstat.el | 155 ++++++++++++++++++++++++++++++++----------------------
>
> It's adapted from Tassilo Horn's 2022 blog post “Using Eldoc with Magit
> (asynchronously!)”: https://www.tsdh.org/posts/2022-07-20-using-eldoc-with-magit-async.html
>
> Thanks
> Johann