bug#73232: [PATCH] Allow vc-diff to suggest a default revision in vc-dir

bug#73232: [PATCH] Allow vc-diff to suggest a default revision in vc-dir

[Spencer Baugh via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Tags: patch


C-u M-x vc-root-diff will prompt for the old revision to use for the
diff.  The prompt will have a default calculated by
vc-diff-build-argument-list-internal.  The default is either the
working revision of the current fileset or the revision before that.

vc-diff-build-argument-list-internal contained a check (added in
c0d66cb21bac57f5ec0378e8a04aac8f35c3eb5c) which explicitly avoided
setting a default if the current fileset was a directory.  This check
was added in 1997 when vc only worked for single files.

This prevents a backend from choosing to return a non-nil value from
'working-revision when passed a directory.  (The vc-hg and vc-git
backends, at least, will do this)

Allow this by moving the file-directory-p check, so that we try
calling 'working-revision when the fileset is a single directory.  The
call is in inside ignore-errors, so if a backend errors when passed a
directory, we'll just get no default, as before.  (Most backends will
just return nil for a directory, rather than erroring)

Also, while we're here, explicitly pass the backend to
vc-working-revision rather than having vc-working-revision recompute
it.

Concretely this has the effect that for the vc-git and vc-hg backends,
running C-u M-x vc-root-diff in vc-dir will have the same behavior as
running C-u M-x vc-root-diff in a clean file: The "Previous revision:"
prompt's default will be the revision before HEAD.

* lisp/vc/vc.el (vc-diff-build-argument-list-internal): Move
file-directory-p check.

In GNU Emacs 29.2.50 (build 17, x86_64-pc-linux-gnu, X toolkit, cairo
 version 1.15.12, Xaw scroll bars) of 2024-09-06 built on
 igm-qws-u22796a
Repository revision: e6d04c06a7eb6ce932b52a346368d02b7a811a00
Repository branch: emacs-29
Windowing system distributor 'The X.Org Foundation', version 11.0.12011000
System Description: Rocky Linux 8.10 (Green Obsidian)

Configured using:
 'configure --with-x-toolkit=lucid --without-gpm --without-gconf
 --without-selinux --without-imagemagick --with-modules --with-gif=no
 --with-cairo --with-rsvg --without-compress-install
 --with-native-compilation=aot --with-tree-sitter
 PKG_CONFIG_PATH=/usr/local/home/garnish/libtree-sitter/0.22.6-1/lib/pkgconfig/'


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Allow-vc-diff-to-suggest-a-default-revision-in-vc-di.patch --]
[-- Type: text/patch, Size: 3069 bytes --]

From e57db4d91cf098d26c8c131d715ef3619466d159 Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh@janestreet.com>
Date: Fri, 13 Sep 2024 11:34:55 -0400
Subject: [PATCH] Allow vc-diff to suggest a default revision in vc-dir

C-u M-x vc-root-diff will prompt for the old revision to use for the
diff.  The prompt will have a default calculated by
vc-diff-build-argument-list-internal.  The default is either the
working revision of the current fileset or the revision before that.

vc-diff-build-argument-list-internal contained a check (added in
c0d66cb21bac57f5ec0378e8a04aac8f35c3eb5c) which explicitly avoided
setting a default if the current fileset was a single directory.  This
check was added in 1997 when vc only worked for single files.

This prevents a backend from choosing to return a non-nil value from
'working-revision when passed a directory.  (The vc-hg and vc-git
backends, at least, will do this)

Allow this by moving the file-directory-p check, so that we try
calling 'working-revision when the fileset is a single directory.  The
call is in inside ignore-errors, so if a backend errors when passed a
directory, we'll just get no default, as before.  (Most backends will
just return nil for a directory, rather than erroring)

Also, while we're here, explicitly pass the backend to
vc-working-revision rather than having vc-working-revision recompute
it.

Concretely this has the effect that for the vc-git and vc-hg backends,
running C-u M-x vc-root-diff in vc-dir will have the same behavior as
running C-u M-x vc-root-diff in a clean file: The "Previous revision:"
prompt's default will be the revision before HEAD.

* lisp/vc/vc.el (vc-diff-build-argument-list-internal): Move
file-directory-p check.
---
 lisp/vc/vc.el | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index 597a1622f5a..d88a655dc6b 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -2074,17 +2074,14 @@ vc-diff-build-argument-list-internal
      ;; filesets, but not yet.
      ((/= (length files) 1)
       nil)
-     ;; if it's a directory, don't supply any revision default
-     ((file-directory-p first)
-      nil)
      ;; if the file is not up-to-date, use working revision as older revision
-     ((not (vc-up-to-date-p first))
-      (setq rev1-default (vc-working-revision first)))
+     ((not (and (file-directory-p first) (vc-up-to-date-p first)))
+      (setq rev1-default (vc-working-revision first backend)))
      ;; if the file is not locked, use last revision and current source as defaults
      (t
       (setq rev1-default (ignore-errors ;If `previous-revision' doesn't work.
                            (vc-call-backend backend 'previous-revision first
-                                            (vc-working-revision first))))
+                                            (vc-working-revision first backend))))
       (when (string= rev1-default "") (setq rev1-default nil))))
     ;; construct argument list
     (let* ((rev1-prompt (format-prompt "Older revision" rev1-default))
-- 
2.39.3

bug#73232: [PATCH] Allow vc-diff to suggest a default revision in vc-dir

[Eli Zaretskii]

> Cc: Juri Linkov <juri@linkov.net>
> Date: Fri, 13 Sep 2024 11:51:47 -0400
> From:  Spencer Baugh via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> -     ;; if it's a directory, don't supply any revision default
> -     ((file-directory-p first)
> -      nil)
>       ;; if the file is not up-to-date, use working revision as older revision
> -     ((not (vc-up-to-date-p first))
> -      (setq rev1-default (vc-working-revision first)))
> +     ((not (and (file-directory-p first) (vc-up-to-date-p first)))
> +      (setq rev1-default (vc-working-revision first backend)))

Doesn't this change the conditions under which we use
vc-working-revision for regular files?  Did you perhaps mean

   ((and (not (file-directory-p first)) (vc-up-to-date-p first))

instead?

bug#73232: [PATCH] Allow vc-diff to suggest a default revision in vc-dir

[Spencer Baugh via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Eli Zaretskii <eliz@gnu.org> writes:
>> Cc: Juri Linkov <juri@linkov.net>
>> Date: Fri, 13 Sep 2024 11:51:47 -0400
>> From:  Spencer Baugh via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>> 
>> -     ;; if it's a directory, don't supply any revision default
>> -     ((file-directory-p first)
>> -      nil)
>>       ;; if the file is not up-to-date, use working revision as older revision
>> -     ((not (vc-up-to-date-p first))
>> -      (setq rev1-default (vc-working-revision first)))
>> +     ((not (and (file-directory-p first) (vc-up-to-date-p first)))
>> +      (setq rev1-default (vc-working-revision first backend)))
>
> Doesn't this change the conditions under which we use
> vc-working-revision for regular files?  Did you perhaps mean
>
>    ((and (not (file-directory-p first)) (vc-up-to-date-p first))
>
> instead?

Oops, yes, fixed.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Allow-vc-diff-to-suggest-a-default-revision-in-vc-di.patch --]
[-- Type: text/x-patch, Size: 3087 bytes --]

From f87b92ce5775d0558b7d9dd502687d1f460bcd2a Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh@janestreet.com>
Date: Fri, 13 Sep 2024 11:34:55 -0400
Subject: [PATCH] Allow vc-diff to suggest a default revision in vc-dir

C-u M-x vc-root-diff will prompt for the old revision to use for the
diff.  The prompt will have a default calculated by
vc-diff-build-argument-list-internal.  The default is either the
working revision of the current fileset or the revision before that.

vc-diff-build-argument-list-internal contained a check (added in
c0d66cb21bac57f5ec0378e8a04aac8f35c3eb5c) which explicitly avoided
setting a default if the current fileset was a single directory.  This
check was added in 1997 when vc only worked for single files.

This prevents a backend from choosing to return a non-nil value from
'working-revision when passed a directory.  (The vc-hg and vc-git
backends, at least, will do this)

Allow this by moving the file-directory-p check, so that we try
calling 'working-revision when the fileset is a single directory.  The
call is in inside ignore-errors, so if a backend errors when passed a
directory, we'll just get no default, as before.  (Most backends will
just return nil for a directory, rather than erroring)

Also, while we're here, explicitly pass the backend to
vc-working-revision rather than having vc-working-revision recompute
it.

Concretely this has the effect that for the vc-git and vc-hg backends,
running C-u M-x vc-root-diff in vc-dir will have the same behavior as
running C-u M-x vc-root-diff in a clean file: The "Previous revision:"
prompt's default will be the revision before HEAD.

* lisp/vc/vc.el (vc-diff-build-argument-list-internal): Move
file-directory-p check. (bug#73232)
---
 lisp/vc/vc.el | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/lisp/vc/vc.el b/lisp/vc/vc.el
index 597a1622f5a..7b1133565f9 100644
--- a/lisp/vc/vc.el
+++ b/lisp/vc/vc.el
@@ -2074,17 +2074,14 @@ vc-diff-build-argument-list-internal
      ;; filesets, but not yet.
      ((/= (length files) 1)
       nil)
-     ;; if it's a directory, don't supply any revision default
-     ((file-directory-p first)
-      nil)
      ;; if the file is not up-to-date, use working revision as older revision
-     ((not (vc-up-to-date-p first))
-      (setq rev1-default (vc-working-revision first)))
+     ((and (not (file-directory-p first)) (not (vc-up-to-date-p first)))
+      (setq rev1-default (vc-working-revision first backend)))
      ;; if the file is not locked, use last revision and current source as defaults
      (t
       (setq rev1-default (ignore-errors ;If `previous-revision' doesn't work.
                            (vc-call-backend backend 'previous-revision first
-                                            (vc-working-revision first))))
+                                            (vc-working-revision first backend))))
       (when (string= rev1-default "") (setq rev1-default nil))))
     ;; construct argument list
     (let* ((rev1-prompt (format-prompt "Older revision" rev1-default))
-- 
2.39.3

bug#73232: [PATCH] Allow vc-diff to suggest a default revision in vc-dir

[Dmitry Gutov]

Hi Spencer,

On 13/09/2024 19:25, Spencer Baugh via Bug reports for GNU Emacs, the 
Swiss army knife of text editors wrote:
> Concretely this has the effect that for the vc-git and vc-hg backends,
> running C-u M-x vc-root-diff in vc-dir will have the same behavior as
> running C-u M-x vc-root-diff in a clean file: The "Previous revision:"
> prompt's default will be the revision before HEAD.

Is this consistent with the current behavior with files?

I mean, if there are any uncommitted changes in a file, we suggest the 
current revision as the one to diff against.

But with a directory we can't so easily determine whether are 
uncommitted changes, but in all likelihood, most of the time when you're 
working on a new feature, there would be. So statistically speaking, 
shouldn't we default to the "file with changes" behavior, suggesting the 
HEAD revision?

I can see where you're coming from though -- that default isn't very 
useful, one might as well not press C-u.

Maybe we should switch to suggesting the previous revision in the prompt 
even when file has changes?

> * lisp/vc/vc.el (vc-diff-build-argument-list-internal): Move
> file-directory-p check. (bug#73232)

Maybe this should mention reusing the value of backend too, for 
completeness.

Also I wonder if it's okay to have a multi-paragraph description in the 
commit message. CONTRIBUTE seems to suggest one paragraph (if any) 
between the summary and the ChangeLog-style contents, but it doesn't 
suggest moving any parts of the description to the bug tracker. But IDK, 
for those who still read the changelog files (instead of using 
vc-print-root-log) this might be too much for a short change. The 
detailed description is great, though, so thanks.

Eli and others, what do you think?

bug#73232: [PATCH] Allow vc-diff to suggest a default revision in vc-dir

[Eli Zaretskii]

> Date: Sat, 14 Sep 2024 04:45:42 +0300
> Cc: 73232@debbugs.gnu.org, juri@linkov.net
> From: Dmitry Gutov <dmitry@gutov.dev>
> 
> Hi Spencer,
> 
> On 13/09/2024 19:25, Spencer Baugh via Bug reports for GNU Emacs, the 
> Swiss army knife of text editors wrote:
> > Concretely this has the effect that for the vc-git and vc-hg backends,
> > running C-u M-x vc-root-diff in vc-dir will have the same behavior as
> > running C-u M-x vc-root-diff in a clean file: The "Previous revision:"
> > prompt's default will be the revision before HEAD.
> 
> Is this consistent with the current behavior with files?
> 
> I mean, if there are any uncommitted changes in a file, we suggest the 
> current revision as the one to diff against.
> 
> But with a directory we can't so easily determine whether are 
> uncommitted changes, but in all likelihood, most of the time when you're 
> working on a new feature, there would be. So statistically speaking, 
> shouldn't we default to the "file with changes" behavior, suggesting the 
> HEAD revision?
> 
> I can see where you're coming from though -- that default isn't very 
> useful, one might as well not press C-u.
> 
> Maybe we should switch to suggesting the previous revision in the prompt 
> even when file has changes?
> 
> > * lisp/vc/vc.el (vc-diff-build-argument-list-internal): Move
> > file-directory-p check. (bug#73232)
> 
> Maybe this should mention reusing the value of backend too, for 
> completeness.
> 
> Also I wonder if it's okay to have a multi-paragraph description in the 
> commit message. CONTRIBUTE seems to suggest one paragraph (if any) 
> between the summary and the ChangeLog-style contents, but it doesn't 
> suggest moving any parts of the description to the bug tracker. But IDK, 
> for those who still read the changelog files (instead of using 
> vc-print-root-log) this might be too much for a short change. The 
> detailed description is great, though, so thanks.

I've modified CONTRIBUTE to not insist on "a paragraph".  (This is
exactly why I hate to codify stuff that is usually a no-brainer:
people tend to interpret each word and letter literally, as if they
were in a legally-binding document, and there's no end to complaints
and discussions about these unimportant details.)

My POV is that the description should be as clear and concise as
possible.  In most cases, there should be a bug report for the issue,
or some on-list discussion of it, and the gory details should be
described there, with only a reference left in the log message.  And,
of course, the important stuff that explains how the code works and
why it does something non-trivial should be in comments in the code.

IOW, I consider a long and very detailed description in the log
message a clear sign that something went wrong in the process: either
we didn't have the issue discussed in a place that can be referenced,
or the code needs to be cleaned-up or commented, or something else.
Git log is NOT the best place to document code changes.

> Eli and others, what do you think?

If you are asking about this bug report, then frankly I don't
understand the use case: when would a user want to invoke this command
with point on a directory?  Directories are never important items in
VC-related activities; files are.  When I'm looking at a tree with
changes, I'm never interested with directories, only with files.  It
is not a coincidence that "git status" will never say anything about
directories; e.g., if you create a directory, you will not see it in
the status, unlike a new file.

So I think I'd need to understand better the use case before I make up
my mind about this proposal.

bug#73232: [PATCH] Allow vc-diff to suggest a default revision in vc-dir

[Dmitry Gutov]

On 14/09/2024 10:04, Eli Zaretskii wrote:

>> Also I wonder if it's okay to have a multi-paragraph description in the
>> commit message. CONTRIBUTE seems to suggest one paragraph (if any)
>> between the summary and the ChangeLog-style contents, but it doesn't
>> suggest moving any parts of the description to the bug tracker. But IDK,
>> for those who still read the changelog files (instead of using
>> vc-print-root-log) this might be too much for a short change. The
>> detailed description is great, though, so thanks.
> 
> I've modified CONTRIBUTE to not insist on "a paragraph".  (This is
> exactly why I hate to codify stuff that is usually a no-brainer:
> people tend to interpret each word and letter literally, as if they
> were in a legally-binding document, and there's no end to complaints
> and discussions about these unimportant details.)
> 
> My POV is that the description should be as clear and concise as
> possible.  In most cases, there should be a bug report for the issue,
> or some on-list discussion of it, and the gory details should be
> described there, with only a reference left in the log message.  And,
> of course, the important stuff that explains how the code works and
> why it does something non-trivial should be in comments in the code.
> 
> IOW, I consider a long and very detailed description in the log
> message a clear sign that something went wrong in the process: either
> we didn't have the issue discussed in a place that can be referenced,
> or the code needs to be cleaned-up or commented, or something else.
> Git log is NOT the best place to document code changes.

Thanks. So if I understand this right, we're allowed to have multiple 
paragraphs in the commit message's description, but we'd rather not.

Perhaps we'd opt for this approach more often when there is no existing 
bug report.

>> Eli and others, what do you think?
> 
> If you are asking about this bug report, then frankly I don't
> understand the use case: when would a user want to invoke this command
> with point on a directory?

I was asking primarily about the documentation standard, so it's okay if 
you don't have a strong position on this. Thanks anyway.

> Directories are never important items in
> VC-related activities; files are.

IME it's pretty common to invoke vc-root-diff from the vc-dir buffer.

And we also added the capability to call vc-diff from Dired recently.

I don't do this myself often, TBH, but if someone is working in large 
repo, and the project is split into directories along subsystem lines, 
then one might want to make commits limited to subdirectories. Then it 
should be useful to call vc-diff on such a directory first.

bug#73232: [PATCH] Allow vc-diff to suggest a default revision in vc-dir

[Eli Zaretskii]

> Date: Sat, 14 Sep 2024 19:13:40 +0300
> Cc: sbaugh@janestreet.com, 73232@debbugs.gnu.org, juri@linkov.net
> From: Dmitry Gutov <dmitry@gutov.dev>
> 
> On 14/09/2024 10:04, Eli Zaretskii wrote:
> 
> > Directories are never important items in
> > VC-related activities; files are.
> 
> IME it's pretty common to invoke vc-root-diff from the vc-dir buffer.

Yes, but vc-dir shows files.  Directories are shown, but when there
are files with "unusual" status, they dominate the display.

> I don't do this myself often, TBH, but if someone is working in large 
> repo, and the project is split into directories along subsystem lines, 
> then one might want to make commits limited to subdirectories. Then it 
> should be useful to call vc-diff on such a directory first.

Does anyone really do such things nowadays?

bug#73232: [PATCH] Allow vc-diff to suggest a default revision in vc-dir

[Juri Linkov]

> And we also added the capability to call vc-diff from Dired recently.

Most of the time I'm using 'C-u C-x v =' on a file or directory
to select another branch as an "Older revision" to compare branches.
But no useful default is possible in such use cases.