bug#38044: 27.0.50; There should be an easier way to look at a specific vc commit

[Lars Ingebrigtsen]

Often when discussing code changes, people will send an email saying
something like "this was fixed by <id>", but Emacs doesn't have a
convenient way to display that.

For that case, Emacs should have a command that prompts for an ID
(defaulting to the ID under point), and then (unless default-directory
is already in a vc-controlled directory), prompts for the directory to
look for that ID, and then display the commit.

Stefan M also commented:

> It's worse: if the commit is not in the current branch it won't be
> listed at all, and `C-x v L` doesn't let you show the log corresponding
> to another branch.
>
> If `C-x v l` accepted a C-u to specify the "revision" for which to show
> the log, then it would solve both problems: you could get the log
> of other branches and you could see the commit message and the diff of
> a specific REVision with:
>
> C-u C-x v l <REV> RET
> followed by hitting `D` on the first element to see the diff

This would be a separate change, because it can also be useful to see a
commit in context.


In GNU Emacs 27.0.50 (build 6, x86_64-pc-linux-gnu, GTK+ Version 3.24.5)
 of 2019-11-01 built on marnie
Repository revision: eda98211e31ed969823c1048b3cde635e08eebe5
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12004000
System Description: Debian GNU/Linux 10 (buster)


--
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

[Stephen Berman]

On Sun, 03 Nov 2019 16:17:05 +0100 Lars Ingebrigtsen <[hidden email]> wrote:

> Often when discussing code changes, people will send an email saying
> something like "this was fixed by <id>", but Emacs doesn't have a
> convenient way to display that.
>
> For that case, Emacs should have a command that prompts for an ID
> (defaulting to the ID under point), and then (unless default-directory
> is already in a vc-controlled directory), prompts for the directory to
> look for that ID, and then display the commit.

I wrote such a command (appended below) for my own use, but it's
git-specific and I don't know the innards of VC well enough to adapt it,
which is why I haven't proposed it for inclusion in Emacs.  Maybe some
of it could be used for a VC command.

Steve Berman


(defun srb-git-log (&optional repo commit)
  "Check REPO for COMMIT and if it exists, display its commit message.
Interactively, prompt for REPO, defaulting to emacs-master, and
for COMMIT, defaulting to the commit hash at point.  If called
with a prefix argument `C-u', show the commit diff in addition to
the commit message."
  (interactive "P")
  (let* ((show (equal current-prefix-arg '(4)))
         (git-dir (if repo
                      (read-directory-name "Repo: " "~/src/emacs/"
                                           nil t "emacs-master")
                    "~/src/emacs/emacs-master"))
         (commit0 (substring-no-properties
                   (or commit
                       (read-string "Commit: " nil nil (word-at-point)))))
         (default-directory git-dir)
         (output-buffer (get-buffer-create "*git log*"))
         (args (split-string (mapconcat #'concat
                                        (if show
                                            `("show" ,commit0)
                                          `("log" "-1" ,commit0)) " ")))
         ;; FIXME: output of `git branch --contains' can be ambiguous (even
         ;; when `git log isn't, because one hash is for a commit, one for a
         ;; tree?).  Can use `git rev-parse --disambiguate=' to find matching
         ;; full hashes.
         (proc (progn
                 (with-current-buffer output-buffer (erase-buffer))
                 (call-process "git" nil output-buffer nil
                               "branch" "--contains" commit0))))
    (when proc
      (with-current-buffer output-buffer
        (goto-char (point-min))
        (unless (looking-at "[ *]")
          (user-error "%s is not on branch %s" commit0
                      (file-name-base git-dir)))
        (insert "Branches:\n")
        (goto-char (point-max))
        (apply #'call-process "git" nil output-buffer nil args)
        (when show
          (with-current-buffer output-buffer
            (diff-mode)))
        (pop-to-buffer output-buffer)))))

[Juri Linkov-2]

>> For that case, Emacs should have a command that prompts for an ID
>> (defaulting to the ID under point), and then (unless default-directory
>> is already in a vc-controlled directory), prompts for the directory to
>> look for that ID, and then display the commit.
>
> I wrote such a command (appended below) for my own use, but it's
> git-specific and I don't know the innards of VC well enough to adapt it,
> which is why I haven't proposed it for inclusion in Emacs.  Maybe some
> of it could be used for a VC command.

It should be easy to add a command properly integrated into the innards of VC
by just grepping for "log-search" in lisp/vc/vc*.el (there are only 7 matches),
and after copying to replace "log-search" with "log-show", and the new command
is ready.

[Juri Linkov-2]

>>> For that case, Emacs should have a command that prompts for an ID
>>> (defaulting to the ID under point), and then (unless default-directory
>>> is already in a vc-controlled directory), prompts for the directory to
>>> look for that ID, and then display the commit.
>>
>> I wrote such a command (appended below) for my own use, but it's
>> git-specific and I don't know the innards of VC well enough to adapt it,
>> which is why I haven't proposed it for inclusion in Emacs.  Maybe some
>> of it could be used for a VC command.
>
> It should be easy to add a command properly integrated into the innards of VC
> by just grepping for "log-search" in lisp/vc/vc*.el (there are only 7 matches),
> and after copying to replace "log-search" with "log-show", and the new command
> is ready.

Actually "log-show" is a bad name.  It's too git-specific OT1H,
and OTOH it's too general since its output varies that doesn't fit
to log-view-mode.

I hoped git would be able to search both sha and commit message
in one command, something like:

  git log -1 5761a1a393 --grep=5761a1a393

to output the log of sha 5761a1a393, and logs of matching commit messages,
but this is not possible in git.

[Juri Linkov-2]

> I hoped git would be able to search both sha and commit message
> in one command, something like:
>
>   git log -1 5761a1a393 --grep=5761a1a393
>
> to output the log of sha 5761a1a393, and logs of matching commit messages,
> but this is not possible in git.

We could support a special syntax for sha search, e.g.

  M-x vc-log-search RET sha:5761a1a393 RET

or using quoted strings as sha search instead of text search:

  M-x vc-log-search RET "5761a1a393" RET

or add a rule that if the search string matches only hex numbers
with a regexp like /[0-9A-F]+/, then run sha search:

  M-x vc-log-search RET 5761a1a393 RET

and any other text will run text search:

  M-x vc-log-search RET text RET

[Dmitry Gutov]

On 13.11.2019 23:03, Juri Linkov wrote:

> Actually "log-show" is a bad name.  It's too git-specific OT1H,
> and OTOH it's too general since its output varies that doesn't fit
> to log-view-mode.

Git's log output is flexible, isn't it? It can be changed using a template.

> I hoped git would be able to search both sha and commit message
> in one command, something like:
>
>    git log -1 5761a1a393 --grep=5761a1a393
>
> to output the log of sha 5761a1a393, and logs of matching commit messages,
> but this is not possible in git.

It's feasible: just call Git twice and concat the results.

But I don't think it's a good idea to mix the results of two different
searches. I think there should be two of them. But there could be an
extra -dwim- command that makes the choice based on the word under point
(and whether it's hexinumeric).

[Juri Linkov-2]

> But I don't think it's a good idea to mix the results of two different
> searches. I think there should be two of them. But there could be an
> extra -dwim- command

I agree than dwim would be better.  But what a hint to use for deciding
on showing a commit by sha instead of matching it in the commit messages?

> that makes the choice based on the word under point
> (and whether it's hexinumeric).

Making the choice whether the word is hexinumeric doesn't look too
unambiguous.  What if the user wants to search a hexinumeric word
in the text of commit messages?  For example, searching for "5761a1a393"
to find messages that contain "Revert commit 5761a1a393 ..."

[Dmitry Gutov]

On 17.11.2019 23:20, Juri Linkov wrote:
> Making the choice whether the word is hexinumeric doesn't look too
> unambiguous.  What if the user wants to search a hexinumeric word
> in the text of commit messages?  For example, searching for "5761a1a393"
> to find messages that contain "Revert commit 5761a1a393 ..."

Then they would call one of the non-dwim commands.

[Juri Linkov-2]

>> Making the choice whether the word is hexinumeric doesn't look too
>> unambiguous.  What if the user wants to search a hexinumeric word
>> in the text of commit messages?  For example, searching for "5761a1a393"
>> to find messages that contain "Revert commit 5761a1a393 ..."
>
> Then they would call one of the non-dwim commands.

Then no dwim command is needed.  We'll just add vc-log-revision and done.
The user will decide whether to use vc-log-search to search in commit
messages, or vc-log-revision to show the log message of one commit.

If adding a new command is not desirable, then add a prefix to existing.
'C-x v L' already uses a prefix arg to prompt for LIMIT, that can be 1.
Another prefix arg could ask for a revision, e.g. 'C-u C-u C-x L 5761a1a393'.

[Dmitry Gutov]

On 18.11.2019 0:36, Juri Linkov wrote:
> Then no dwim command is needed.  We'll just add vc-log-revision and done.
> The user will decide whether to use vc-log-search to search in commit
> messages, or vc-log-revision to show the log message of one commit.

I'm fine with having two commands. Having a '-dwim-' one could save on
key bindings, but we don't have one for vc-log-search anyway.

[Juri Linkov-2]

>> Then no dwim command is needed.  We'll just add vc-log-revision and done.
>> The user will decide whether to use vc-log-search to search in commit
>> messages, or vc-log-revision to show the log message of one commit.
>
> I'm fine with having two commands. Having a '-dwim-' one could save on key
> bindings, but we don't have one for vc-log-search anyway.

Ok, here is a new command vc-log-revision:


diff --git a/lisp/vc/vc-git.el b/lisp/vc/vc-git.el
index 5ab8e7ec53..f379c3d890 100644
--- a/lisp/vc/vc-git.el
+++ b/lisp/vc/vc-git.el
@@ -1189,7 +1189,7 @@ vc-git-log-incoming
  "@{upstream}"
       remote-location))))
 
-(defun vc-git-log-search (buffer pattern)
+(defun vc-git-log-search (buffer pattern &optional limit)
   "Search the log of changes for PATTERN and output results into BUFFER.
 
 PATTERN is a basic regular expression by default in Git.
@@ -1197,8 +1197,10 @@ vc-git-log-search
 Display all entries that match log messages in long format.
 With a prefix argument, ask for a command to run that will output
 log entries."
-  (let ((args `("log" "--no-color" "-i"
-                ,(format "--grep=%s" (or pattern "")))))
+  (let ((args (if limit
+                  `("log" "--no-color" "-n" "1" ,(or pattern ""))
+                `("log" "--no-color" "-i"
+                  ,(format "--grep=%s" (or pattern ""))))))
     (when current-prefix-arg
       (setq args (cdr (split-string
        (read-shell-command
diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index 0d29c80d02..92faa59502 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -2542,6 +2542,16 @@ vc-log-outgoing
     (vc-incoming-outgoing-internal backend (or remote-location "")
                                    "*vc-outgoing*" 'log-outgoing)))
 
+(defun vc-log-search-internal (backend buffer-name type pattern &optional limit)
+  (vc-log-internal-common
+   backend buffer-name nil type
+   (lambda (bk buf type-arg _files)
+     (vc-call-backend bk type-arg buf pattern limit))
+   (lambda (_bk _files-arg _ret) nil)
+   nil ;; Don't move point.
+   (lambda (_ignore-auto _noconfirm)
+     (vc-log-search-internal backend pattern buffer-name type))))
+
 ;;;###autoload
 (defun vc-log-search (pattern)
   "Search the log of changes for PATTERN.
@@ -2558,8 +2568,25 @@ vc-log-search
   (let ((backend (vc-deduce-backend)))
     (unless backend
       (error "Buffer is not version controlled"))
-    (vc-incoming-outgoing-internal backend pattern
-                                   "*vc-search-log*" 'log-search)))
+    (vc-log-search-internal backend "*vc-search-log*" 'log-search pattern)))
+
+;;;###autoload
+(defun vc-log-revision (revision)
+  "Search the log of changes for REVISION.
+Display the REVISION log entry in long format."
+  (interactive (list (unless current-prefix-arg
+                       (let ((default (thing-at-point 'word)))
+                         (vc-read-revision
+                          (if default
+                              (format "Revision to log (default %s): " default)
+                            "Revision to log: ")
+                          nil nil default)))))
+  (when (equal revision "")
+    (error "No revision specified"))
+  (let ((backend (vc-deduce-backend)))
+    (unless backend
+      (error "Buffer is not version controlled"))
+    (vc-log-search-internal backend "*vc-search-log*" 'log-search revision 1)))
 
 ;;;###autoload
 (defun vc-log-mergebase (_files rev1 rev2)

[Eli Zaretskii]

> From: Juri Linkov <[hidden email]>
> Date: Mon, 18 Nov 2019 23:31:50 +0200
> Cc: Lars Ingebrigtsen <[hidden email]>, Stephen Berman <[hidden email]>,
>  [hidden email]
>
> +(defun vc-log-revision (revision)
> +  "Search the log of changes for REVISION.
> +Display the REVISION log entry in long format."

The first sentence says that it "searches the log", but the second
says that it shows the log.  I think the first sentence is wrong and
the second is right: there's no "search" here, is there?

Even if the implementation does search, the doc string should describe
the result, not the implementation.

Thanks.

[Dmitry Gutov]

On 18.11.2019 23:31, Juri Linkov wrote:
> +(defun vc-git-log-search (buffer pattern &optional limit)
>
> +  (let ((args (if limit
> +                  `("log" "--no-color" "-n" "1" ,(or pattern ""))
> +                `("log" "--no-color" "-i"
> +                  ,(format "--grep=%s" (or pattern ""))))))

Why would we hardcode that if LIMIT is passed then PATTERN is a
revision, not a search string?

Let's please make it another backend command instead of piggy-backing on
'log-search'.

[Eli Zaretskii]

> Why would we hardcode that if LIMIT is passed then PATTERN is a
> revision, not a search string?
>
> Let's please make it another backend command instead of piggy-backing on
> 'log-search'.

I agree.

Stepping a notch back, wasn't the original request to have a command
that would display a specific commit?  If so, that's not a "log"
command, that's closer to a "diff" command.  And don't we already have
a "diff" command which shows diffs for a specific revision?  If not,
let's provide that.  The fact that Git can show diffs in some
sub-commands of 'log' shouldn't dupe us into thinking that this is
about "log" in any way.

A command that searches the log entries for a string/pattern should be
a separate command, which could be a "log" command.

Thanks.

[Dmitry Gutov]

On 19.11.2019 18:12, Eli Zaretskii wrote:
> If so, that's not a "log"
> command, that's closer to a "diff" command.

It's kind of both. 'git show HEAD' is the format that I'd personally
expect: some meta info, including the commit message, followed by the
diff contents.

> And don't we already have
> a "diff" command which shows diffs for a specific revision?

It *can* do that. And we also have a command that shows the revision log
message and stuff: vc-annotate-show-log-revision-at-line. We could reuse
its logic.

And either add a diff output at the botton (making it a different
command an dealing with major mode having to support both the headers
and the diff), or rely on log-view-mode's bindings (the user can press
'd' or 'D' there).

[Eli Zaretskii]

> Cc: [hidden email], [hidden email], [hidden email],
>  [hidden email]
> From: Dmitry Gutov <[hidden email]>
> Date: Tue, 19 Nov 2019 18:59:36 +0200
>
> On 19.11.2019 18:12, Eli Zaretskii wrote:
> > If so, that's not a "log"
> > command, that's closer to a "diff" command.
>
> It's kind of both. 'git show HEAD' is the format that I'd personally
> expect: some meta info, including the commit message, followed by the
> diff contents.

That's true, but I'd hesitate to introduce a new class of "show"
commands just because Git has it.  I see no problem showing the
meta-data with diffs (when Git is the back-end), perhaps with an
option to disable that.  My rationale is that other VCSes have a diff
command that shows only the diffs, and that command is used "to show"
a revision with those VCSes.

> > And don't we already have
> > a "diff" command which shows diffs for a specific revision?
>
> It *can* do that. And we also have a command that shows the revision log
> message and stuff: vc-annotate-show-log-revision-at-line. We could reuse
> its logic.
>
> And either add a diff output at the botton (making it a different
> command an dealing with major mode having to support both the headers
> and the diff), or rely on log-view-mode's bindings (the user can press
> 'd' or 'D' there).

I think we should disconnect this "show" command from "log".  It is
conceptually wrong to make them related.  Of course a vc-log buffer
could have (and already has, AFAIK) a binding to a command that shows
the revision at point's line.

[Juri Linkov-2]

>> Let's please make it another backend command instead of piggy-backing on
>> 'log-search'.
>
> I agree.
>
> Stepping a notch back, wasn't the original request to have a command
> that would display a specific commit?  If so, that's not a "log"
> command, that's closer to a "diff" command.  And don't we already have
> a "diff" command which shows diffs for a specific revision?

Then we could use something like the existing backend 'region-history'
that for vc-git uses 'git log', but displays both the log and diff
in one output buffer.

[Dmitry Gutov]

On 19.11.2019 19:43, Eli Zaretskii wrote:

>> It's kind of both. 'git show HEAD' is the format that I'd personally
>> expect: some meta info, including the commit message, followed by the
>> diff contents.
>
> That's true, but I'd hesitate to introduce a new class of "show"
> commands just because Git has it.  I see no problem showing the
> meta-data with diffs (when Git is the back-end), perhaps with an
> option to disable that.

These two sentences seem to contradict each other. To do the latter, we
need to "introduce a new class of show commands". Because vc-git-diff
won't print any metadata.

> My rationale is that other VCSes have a diff
> command that shows only the diffs, and that command is used "to show"
> a revision with those VCSes.

IMO the log message is more important because it describes and justifies
what happened. Showing the diff is good as well.

Maybe the other VCSes don't have a simple command to do the same, but
they can either be called twice, or use special formatting. For
instance, Hg can use this command:

hg log -r <REV> -p

>>> And don't we already have
>>> a "diff" command which shows diffs for a specific revision?
>>
>> It *can* do that. And we also have a command that shows the revision log
>> message and stuff: vc-annotate-show-log-revision-at-line. We could reuse
>> its logic.
>>
>> And either add a diff output at the botton (making it a different
>> command an dealing with major mode having to support both the headers
>> and the diff), or rely on log-view-mode's bindings (the user can press
>> 'd' or 'D' there).
>
> I think we should disconnect this "show" command from "log".  It is
> conceptually wrong to make them related.  Of course a vc-log buffer
> could have (and already has, AFAIK) a binding to a command that shows
> the revision at point's line.

Linking to a commit message from a diff buffer wouldn't really work. But
we can "link" to a diff buffer from a common message buffer.

Of course, it's better to just display both.

[Eli Zaretskii]

> Cc: [hidden email], [hidden email], [hidden email],
>  [hidden email]
> From: Dmitry Gutov <[hidden email]>
> Date: Wed, 20 Nov 2019 01:17:18 +0200
>
> > That's true, but I'd hesitate to introduce a new class of "show"
> > commands just because Git has it.  I see no problem showing the
> > meta-data with diffs (when Git is the back-end), perhaps with an
> > option to disable that.
>
> These two sentences seem to contradict each other. To do the latter, we
> need to "introduce a new class of show commands". Because vc-git-diff
> won't print any metadata.

I was talking about the VC level, not the vc-git level.  vc-git could
have a show command, but the user of VC would still invoke a variant
of vc-diff.

> > My rationale is that other VCSes have a diff
> > command that shows only the diffs, and that command is used "to show"
> > a revision with those VCSes.
>
> IMO the log message is more important because it describes and justifies
> what happened. Showing the diff is good as well.

That's not relevant to the issue at hand.  Like it or not, VCSes other
than Git describe a revision by the diffs alone.

> Maybe the other VCSes don't have a simple command to do the same, but
> they can either be called twice, or use special formatting. For
> instance, Hg can use this command:
>
> hg log -r <REV> -p

IMO, this is over-engineering.  If the VCS developers don't see the
need to have a commands which shows meta-data together with the diffs,
we should not force that on that VCS.  IOW, what Git does is not
necessarily the only game in town.

The VC level should be clean and uniform.  Any VCS-specific mindset
should not enter it too far, or else VC will not do its job of
insulating the user from the VCS differences well.

[Eli Zaretskii]

> From: Juri Linkov <[hidden email]>
> Cc: Dmitry Gutov <[hidden email]>,  [hidden email],
>   [hidden email],  [hidden email]
> Date: Wed, 20 Nov 2019 00:20:08 +0200
>
> > Stepping a notch back, wasn't the original request to have a command
> > that would display a specific commit?  If so, that's not a "log"
> > command, that's closer to a "diff" command.  And don't we already have
> > a "diff" command which shows diffs for a specific revision?
>
> Then we could use something like the existing backend 'region-history'
> that for vc-git uses 'git log', but displays both the log and diff
> in one output buffer.

region-history is slow, so I'm not sure it is a good starting point
for this feature.  A diff command is usually very fats with every VCS.