bug#59414: 29.0.50; Have vc-git-expanded-log-entry pass --stat

bug#59414: 29.0.50; Have vc-git-expanded-log-entry pass --stat

[Sean Whitton]

Hello,

I would like to add --stat to the list of options passed to git-log(1)
by vc-git-expanded-log-entry.  I think it makes VC root logs for Git
repositories much more informative with little disadvantage.  Any comments?

-- 
Sean Whitton

bug#59414: 29.0.50; Have vc-git-expanded-log-entry pass --stat

[Juri Linkov]

> I would like to add --stat to the list of options passed to git-log(1)
> by vc-git-expanded-log-entry.  I think it makes VC root logs for Git
> repositories much more informative with little disadvantage.  Any comments?

Not sure about adding --stat by default since often it produces too long
multi-line output.  But definitely the options should be customizable
to avoid adding such cruft to config files:

  (with-eval-after-load 'vc-git
    ;; OVERRIDDEN AND ADDED "--pretty=fuller"
    (defun vc-git-expanded-log-entry (revision)
      (with-temp-buffer
        (apply 'vc-git-command t nil nil (list "log" revision "-1" "--pretty=fuller" "--"))
        (goto-char (point-min))
        (unless (eobp)
          ;; Indent the expanded log entry.
          (while (re-search-forward "^  " nil t)
            (replace-match "")
            (forward-line))
          (buffer-string)))))

bug#59414: 29.0.50; Have vc-git-expanded-log-entry pass --stat

[Dmitry Gutov]

On 20.11.2022 19:52, Juri Linkov wrote:
>> I would like to add --stat to the list of options passed to git-log(1)
>> by vc-git-expanded-log-entry.  I think it makes VC root logs for Git
>> repositories much more informative with little disadvantage.  Any comments?
> Not sure about adding --stat by default since often it produces too long
> multi-line output.  But definitely the options should be customizable
> to avoid adding such cruft to config files:

Some new -switches variable could work. One should also ask themselves 
whether they want to apply the new switches to the "full" log as well -- 
one you get by pressing 'C-x v l'.

bug#59414: 29.0.50; Have vc-git-expanded-log-entry pass --stat

[Sean Whitton]

Hello,

On Sun 20 Nov 2022 at 07:52PM +02, Juri Linkov wrote:

>> I would like to add --stat to the list of options passed to git-log(1)
>> by vc-git-expanded-log-entry.  I think it makes VC root logs for Git
>> repositories much more informative with little disadvantage.  Any comments?
>
> Not sure about adding --stat by default since often it produces too long
> multi-line output.  But definitely the options should be customizable
> to avoid adding such cruft to config files:

We already have vc-git-log-switches.  git-log(1) gets called *without*
it in (at least) the following places:

- vc-git-log-outgoing
- vc-git-log-incoming
- vc-git-log-search
- vc-git-expanded-log-entry
- vc-git-region-history

I guess that the first three should probably use vc-git-log-switches if
anything?  And so we would want a separate option for
vc-git-expanded-log-entry.  Not sure about vc-git-region-history.

-- 
Sean Whitton

bug#59414: 29.0.50; Have vc-git-expanded-log-entry pass --stat

[Dmitry Gutov]

On 20.11.2022 23:58, Sean Whitton wrote:
> And so we would want a separate option for
> vc-git-expanded-log-entry.

Do we anticipate people using different switches for it from the first 
three? Like, do you want to add --stat only there and not in those other 
cases?

vc-git-region-history - no, probably not, since it comes with the diffs 
already.

bug#59414: 29.0.50; Have vc-git-expanded-log-entry pass --stat

[Sean Whitton]

Hello,

On Mon 21 Nov 2022 at 12:11AM +02, Dmitry Gutov wrote:

> On 20.11.2022 23:58, Sean Whitton wrote:
>> And so we would want a separate option for
>> vc-git-expanded-log-entry.
>
> Do we anticipate people using different switches for it from the first three?
> Like, do you want to add --stat only there and not in those other cases?

Yes: if you add --stat to vc-git-log-switches it simply breaks Log View.

-- 
Sean Whitton

bug#59414: 29.0.50; Have vc-git-expanded-log-entry pass --stat

[Sean Whitton]

Hello,

On Sun 20 Nov 2022 at 02:58PM -07, Sean Whitton wrote:

> We already have vc-git-log-switches.  git-log(1) gets called *without*
> it in (at least) the following places:
>
> - vc-git-log-outgoing
> - vc-git-log-incoming
> - vc-git-log-search
> - vc-git-expanded-log-entry
> - vc-git-region-history
>
> I guess that the first three should probably use vc-git-log-switches if
> anything?  And so we would want a separate option for
> vc-git-expanded-log-entry.  Not sure about vc-git-region-history.

I think that we actually need two defcustoms for the regular logs and
shortlogs:

- vc-git-print-log -- should choose which defcustom to include based on
                      its SHORTLOG parameter
- vc-git-log-outgoing -- vc-git-shortlog-switches
- vc-git-log-incoming -- vc-git-shortlog-switches
- vc-git-log-search -- vc-git-log-switches
- vc-git-expanded-log-entry -- vc-git-log-switches

This is because some options are incompatible with shortlogs, such as
--stat.  So, the proposed change is to add the new defcustom, change
vc-git-print-log to use both defcustoms, and change all the other
functions to use one of them.  How does    this sound?

-- 
Sean Whitton

bug#59414: 29.0.50; Have vc-git-expanded-log-entry pass --stat

[Juri Linkov]

> - vc-git-print-log -- should choose which defcustom to include based on
>                       its SHORTLOG parameter
> - vc-git-log-outgoing -- vc-git-shortlog-switches
> - vc-git-log-incoming -- vc-git-shortlog-switches
> - vc-git-log-search -- vc-git-log-switches
> - vc-git-expanded-log-entry -- vc-git-log-switches
>
> This is because some options are incompatible with shortlogs, such as
> --stat.  So, the proposed change is to add the new defcustom, change
> vc-git-print-log to use both defcustoms, and change all the other
> functions to use one of them.  How does    this sound?

Your classification correctly reflects the current situation
where two types of logs are in use by these commands.  So I think
two defcustoms as the minimal number of customization knobs
is a good idea.

bug#59414: 29.0.50; Have vc-git-expanded-log-entry pass --stat

[Sean Whitton]

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

Hello,

Thanks.  Here's a patch.

-- 
Sean Whitton

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Improve-passing-user-switches-to-Git-log-commands.patch --]
[-- Type: text/x-patch, Size: 5605 bytes --]

From 868141665504d689f8f02bc8c67391b63d8962b2 Mon Sep 17 00:00:00 2001
From: Sean Whitton <spwhitton@spwhitton.name>
Date: Sun, 4 Dec 2022 15:56:35 -0700
Subject: [PATCH] Improve passing user switches to Git log commands

* lisp/vc/vc-git.el (vc-git-log-switches): Revise docstring.
(vc-git-shortlog-switches): New defcustom.
(vc-git-print-log): Use vc-git-log-switches or
vc-git-shortlog-switches depending on whether printing a shortlog.
(vc-git-log-outgoing, vc-git-log-incoming): Use
vc-git-shortlog-switches.
(vc-git-log-search, vc-git-expanded-log-entry): Use
vc-git-log-switches.
* etc/NEWS: Document the new defcustom.
---
 etc/NEWS          |  7 ++++++
 lisp/vc/vc-git.el | 57 +++++++++++++++++++++++++++++------------------
 2 files changed, 42 insertions(+), 22 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 9b8edde5155..8f5b17fb4af 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -41,6 +41,13 @@ connection.
 \f
 * Changes in Specialized Modes and Packages in Emacs 30.1
 
+** VC
+
+---
+*** New user option 'vc-git-shortlog-switches'
+String or list of strings giving Git log switches for shortlogs, such
+as 'C-x v L'.  'vc-git-log-switches' is no longer used for shortlogs.
+
 \f
 * New Modes and Packages in Emacs 30.1
 
diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index 38e9d5f9c91..59dfb6c1252 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -136,12 +136,19 @@ vc-git-annotate-switches
 ;;;###autoload(put 'vc-git-annotate-switches 'safe-local-variable (lambda (switches) (equal switches "-w")))
 
 (defcustom vc-git-log-switches nil
-  "String or list of strings specifying switches for Git log under VC."
+  "String or list of strings giving Git log switches for non-shortlogs."
   :type '(choice (const :tag "None" nil)
                  (string :tag "Argument String")
                  (repeat :tag "Argument List" :value ("") string))
   :version "28.1")
 
+(defcustom vc-git-shortlog-switches nil
+  "String or list of strings giving Git log switches for shortlogs."
+  :type '(choice (const :tag "None" nil)
+                 (string :tag "Argument String")
+                 (repeat :tag "Argument List" :value ("") string))
+  :version "30.1")
+
 (defcustom vc-git-resolve-conflicts t
   "When non-nil, mark conflicted file as resolved upon saving.
 That is performed after all conflict markers in it have been
@@ -1325,7 +1332,8 @@ vc-git-print-log
                     ,(format "--pretty=tformat:%s"
                              (car vc-git-root-log-format))
                     "--abbrev-commit"))
-                (ensure-list vc-git-log-switches)
+                (ensure-list
+                 (if shortlog vc-git-shortlog-switches vc-git-log-switches))
                 (when (numberp limit)
                   (list "-n" (format "%s" limit)))
 		(when start-revision
@@ -1340,16 +1348,16 @@ vc-git-print-log
 
 (defun vc-git-log-outgoing (buffer remote-location)
   (vc-setup-buffer buffer)
-  (vc-git-command
-   buffer 'async nil
-   "log"
-   "--no-color" "--graph" "--decorate" "--date=short"
-   (format "--pretty=tformat:%s" (car vc-git-root-log-format))
-   "--abbrev-commit"
-   (concat (if (string= remote-location "")
-	       "@{upstream}"
-	     remote-location)
-	   "..HEAD")))
+  (apply #'vc-git-command buffer 'async nil
+         `("log"
+           "--no-color" "--graph" "--decorate" "--date=short"
+           ,(format "--pretty=tformat:%s" (car vc-git-root-log-format))
+           "--abbrev-commit"
+           ,@(ensure-list vc-git-shortlog-switches)
+           ,(concat (if (string= remote-location "")
+	                "@{upstream}"
+	              remote-location)
+	            "..HEAD"))))
 
 (defun vc-git-log-incoming (buffer remote-location)
   (vc-setup-buffer buffer)
@@ -1359,15 +1367,15 @@ vc-git-log-incoming
                     ;; so remove everything except a repository name.
                     (replace-regexp-in-string
                      "/.*" "" remote-location)))
-  (vc-git-command
-   buffer 'async nil
-   "log"
-   "--no-color" "--graph" "--decorate" "--date=short"
-   (format "--pretty=tformat:%s" (car vc-git-root-log-format))
-   "--abbrev-commit"
-   (concat "HEAD.." (if (string= remote-location "")
-			"@{upstream}"
-		      remote-location))))
+  (apply #'vc-git-command buffer 'async nil
+         `("log"
+           "--no-color" "--graph" "--decorate" "--date=short"
+           ,(format "--pretty=tformat:%s" (car vc-git-root-log-format))
+           "--abbrev-commit"
+           ,@(ensure-list vc-git-shortlog-switches)
+           ,(concat "HEAD.." (if (string= remote-location "")
+			         "@{upstream}"
+		               remote-location)))))
 
 (defun vc-git-log-search (buffer pattern)
   "Search the log of changes for PATTERN and output results into BUFFER.
@@ -1378,6 +1386,7 @@ vc-git-log-search
 With a prefix argument, ask for a command to run that will output
 log entries."
   (let ((args `("log" "--no-color" "-i"
+                ,@(ensure-list vc-git-log-switches)
                 ,(format "--grep=%s" (or pattern "")))))
     (when current-prefix-arg
       (setq args (cdr (split-string
@@ -1462,7 +1471,11 @@ vc-git-show-log-entry
 
 (defun vc-git-expanded-log-entry (revision)
   (with-temp-buffer
-    (apply #'vc-git-command t nil nil (list "log" revision "-1"  "--no-color" "--"))
+    (apply #'vc-git-command t nil nil
+           `("log"
+             ,revision
+             "-1"  "--no-color" ,@(ensure-list vc-git-log-switches)
+             "--"))
     (goto-char (point-min))
     (unless (eobp)
       ;; Indent the expanded log entry.
-- 
2.30.2

bug#59414: 29.0.50; Have vc-git-expanded-log-entry pass --stat

[Dmitry Gutov]

On 05/12/2022 00:57, Sean Whitton wrote:
> Hello,
> 
> Thanks.  Here's a patch.

Thank you.

Looks good, please install.

bug#59414: 29.0.50; Have vc-git-expanded-log-entry pass --stat

[Sean Whitton]

Hello,

Thanks for looking it over.  Installed.

-- 
Sean Whitton

bug#59414: 29.0.50; Have vc-git-expanded-log-entry pass --stat

[Eli Zaretskii]

> From: Sean Whitton <spwhitton@spwhitton.name>
> Date: Sun, 04 Dec 2022 15:57:52 -0700
> 
> @@ -1340,16 +1348,16 @@ vc-git-print-log
>  
>  (defun vc-git-log-outgoing (buffer remote-location)
>    (vc-setup-buffer buffer)
> -  (vc-git-command
> -   buffer 'async nil
> -   "log"
> -   "--no-color" "--graph" "--decorate" "--date=short"
> -   (format "--pretty=tformat:%s" (car vc-git-root-log-format))
> -   "--abbrev-commit"
> -   (concat (if (string= remote-location "")
> -	       "@{upstream}"
> -	     remote-location)
> -	   "..HEAD")))
> +  (apply #'vc-git-command buffer 'async nil
> +         `("log"
> +           "--no-color" "--graph" "--decorate" "--date=short"
> +           ,(format "--pretty=tformat:%s" (car vc-git-root-log-format))
> +           "--abbrev-commit"
> +           ,@(ensure-list vc-git-shortlog-switches)
> +           ,(concat (if (string= remote-location "")
> +	                "@{upstream}"
> +	              remote-location)
> +	            "..HEAD"))))

Why the change from vc-git-command to 'apply'?  The former took care for
setting up the I/O encoding for the Git command, while the latter just uses
the process defaults, which are not necessarily right for the underlying
system and locale.

In general, I'd prefer that invocations of all the Git commands went through
a single function, so that we could make sure the encoding/decoding stuff,
which is entirely non-trivial with Git, is done correctly in a single place
that is easy to audit and maintain.  I know that not all the commands are
invoked through there, but making more of them do so is going in the
direction that is 180° opposite to what we should strive to.

Thanks.

bug#59414: 29.0.50; Have vc-git-expanded-log-entry pass --stat

[Dmitry Gutov]

On 05/12/2022 14:29, Eli Zaretskii wrote:
> Why the change from vc-git-command to 'apply'?  The former took care for
> setting up the I/O encoding for the Git command, while the latter just uses
> the process defaults, which are not necessarily right for the underlying
> system and locale.
> 
> In general, I'd prefer that invocations of all the Git commands went through
> a single function, so that we could make sure the encoding/decoding stuff,
> which is entirely non-trivial with Git, is done correctly in a single place
> that is easy to audit and maintain.  I know that not all the commands are
> invoked through there, but making more of them do so is going in the
> direction that is 180° opposite to what we should strive to.

Both cases use 'vc-git-command', don't they?

'apply' is just about how the arguments are passed.

bug#59414: 29.0.50; Have vc-git-expanded-log-entry pass --stat

[Eli Zaretskii]

> Date: Mon, 5 Dec 2022 14:40:39 +0200
> Cc: 59414@debbugs.gnu.org, juri@linkov.net
> From: Dmitry Gutov <dgutov@yandex.ru>
> 
> On 05/12/2022 14:29, Eli Zaretskii wrote:
> > Why the change from vc-git-command to 'apply'?  The former took care for
> > setting up the I/O encoding for the Git command, while the latter just uses
> > the process defaults, which are not necessarily right for the underlying
> > system and locale.
> > 
> > In general, I'd prefer that invocations of all the Git commands went through
> > a single function, so that we could make sure the encoding/decoding stuff,
> > which is entirely non-trivial with Git, is done correctly in a single place
> > that is easy to audit and maintain.  I know that not all the commands are
> > invoked through there, but making more of them do so is going in the
> > direction that is 180° opposite to what we should strive to.
> 
> Both cases use 'vc-git-command', don't they?
> 
> 'apply' is just about how the arguments are passed.

Sorry, too little coffee, I guess.